299151 - Crash in nsRDFXMLSerializer - NULL pointer gRDFC

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect)

Description

[<<INACTIVE>>]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050610 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050610 Firefox/1.0+

There is invalid reference counting (gRefCnt) in the nsRDFXMLSerializer class
related to the gRDFC static variable pointer. This may result to null pointer
dereference.

Reproducible: Always

Steps to Reproduce:
1. Instatiate nsRDFXMLSerializer - i.e. create instace of
@mozilla.org/rdf/xml-serializer;1 component.
2. Dereference this instance immediately.
3. Let this class be used by mozilla - crash occures in IsContainerProperty
method of the same class on the pointer dereference.

Cause:
in destructor of nsRDFXMLSerializer implementation class there is check for
--gRefCnt == 0. When entering the destructor, value of gRefCnt may be already
zero (0) because gRefCnt is incremented in method Create(). When this method is
not called during the object lifetime, gRefCnt is in the destructor decremented
to value -1. During next call of Create() its value is tested oposite to 0 but
actual value of the ref counter is -1. So, the gRDFC is not initialized and
crash occures.

Comment 1

[<<INACTIVE>>]

This patch resolves the bug. gRefCnt is decremented and release code may be
called only if value of the counter is greater then zero.

Index: base/src/nsRDFXMLSerializer.cpp
===================================================================
RCS file: /cvsroot/mozilla/rdf/base/src/nsRDFXMLSerializer.cpp,v
retrieving revision 1.25
diff -u -8 -r1.25 nsRDFXMLSerializer.cpp
--- base/src/nsRDFXMLSerializer.cpp	14 Feb 2005 20:45:37 -0000	1.25
+++ base/src/nsRDFXMLSerializer.cpp	29 Jun 2005 15:38:39 -0000
@@ -125,17 +125,17 @@
 {
     MOZ_COUNT_CTOR(nsRDFXMLSerializer);
 }
 
 nsRDFXMLSerializer::~nsRDFXMLSerializer()
 {
     MOZ_COUNT_DTOR(nsRDFXMLSerializer);
 
-    if (--gRefCnt == 0) {
+    if (gRefCnt > 0 && --gRefCnt == 0) {
         NS_IF_RELEASE(kRDF_Bag);
         NS_IF_RELEASE(kRDF_Seq);
         NS_IF_RELEASE(kRDF_Alt);
         NS_IF_RELEASE(kRDF_instanceOf);
         NS_IF_RELEASE(kRDF_type);
         NS_IF_RELEASE(kRDF_nextVal);
         NS_IF_RELEASE(gRDFC);
     }

Comment 2

[Benjamin Smedberg]

That patch is not acceptable mainly because refcnt should never go below 0.
There's something else screwy going on when the Create method is never called.

Comment 3

[Axel Hecht]

Please submit a testcase, maybe even for xpcshell.

Create seems to not increment the reference count if you request a bad interface.
Maybe it's that.

Comment 4

[<<INACTIVE>>]

(In reply to comment #2)
> That patch is not acceptable mainly because refcnt should never go below 0.
> There's something else screwy going on when the Create method is never called.

The bug is just in the condition: if (--gRefCnt == 0). If you create the
instance and do not call Create method it is there (in the bad condition) where
the counter drops under zero. 

In the creation time the counter is zero. Same it is in the destructor call. If
you execute this condition (--gRefCnt == 0) the value if gRefCnt is decreased
and returned value is -1. So, the code below the condition is not executed -
nothing wrong happens. Until you next time create an instance, call Create
method, where the gRefCnt is rised back to the zero (but no initialization is
made due to "if (gRefCnt++ == 0)" - result of gRefCnt++ is -1. Finally call
IsContainerProperty method - the crash occures.

Comment 5

[Axel Hecht]

CreateInstance calls into Create, see
http://lxr.mozilla.org/seamonkey/source/rdf/build/nsRDFModule.cpp#204. Thus, #2
(and #3) still stand.

Comment 6

[<<INACTIVE>>]

(In reply to comment #5)
> CreateInstance calls into Create, see
> http://lxr.mozilla.org/seamonkey/source/rdf/build/nsRDFModule.cpp#204. Thus, #2
> (and #3) still stand.

Now I understand! This bug occures in case I query interface what the
nsRDFXMLSerializer does not implement, ie. nsCOMPtr<nsISomeInterface> ptr =
do_CreateInstance("@mozilla.org/rdf/xml-serializer;1");

The condition "if (NS_SUCCEEDED(rv) && (gRefCnt++ == 0)) do {..." (line 87) is
beyond query interface - rv is failure and reference counter is not rised.

And then counter is decreased to -1 in the destructor code.

Comment 7

[Axel Hecht]

Attachment: testcase, crash xpcshell

Comment 8

[Axel Hecht]

Attachment: first, addref, then check rv

I need to addref gRefCnt there independently of rv, as the RELEASE later on
calls
the destructor on the rdfxml serializer object result, if the QI failed.

Tested this patch with my testcase, fixes the crash and serializes nicely.

Comment 9

[Benjamin Smedberg]

Comment on attachment 187772 [details] [diff] [review]
first, addref, then check rv

That's a bit icky, but ok.

Comment 10

[Mike Shaver (:shaver emeritus)]

Comment on attachment 187772 [details] [diff] [review]
first, addref, then check rv

How about instead making it clearer, by incrementing the refcount and then
testing it in separate statements, rather than relying on the precise ordering
of postfix increment, && short-circuit, and the moon being in the house of
Jupiter?

No More RDF Refcounting Cleverness!  Chant it with me; it's very empowering.

Comment 11

[Axel Hecht]

Attachment: use nsCOMPtr instead of manually addref, that's easier to track

I moved the result into an nsCOMPtr, then I can just return when I want to.
I also check for gRefCnt == 1 now, which may be a bit uncommon, but doesn't
rely
on post increment.

Comment 12

[Mike Shaver (:shaver emeritus)]

Comment on attachment 187796 [details] [diff] [review]
use nsCOMPtr instead of manually addref, that's easier to track

That's my man. sr=shaver.

Comment 13

[Axel Hecht]

Comment on attachment 187796 [details] [diff] [review]
use nsCOMPtr instead of manually addref, that's easier to track

low risk crash fix, trying to land that

Comment 14

[<<INACTIVE>>]

(In reply to comment #13)
> (From update of attachment 187796 [details] [diff] [review] [edit])
> low risk crash fix, trying to land that
> 

I succesfully tested this patch. All works fine.

Comment 15

[Benjamin Smedberg]

Comment on attachment 187796 [details] [diff] [review]
use nsCOMPtr instead of manually addref, that's easier to track

a=bsmedberg for checkin on 6/30 only

Comment 16

[Axel Hecht]

Checked in a few hours ago.