Closed
Bug 299151
Opened 19 years ago
Closed 19 years ago
Crash in nsRDFXMLSerializer - NULL pointer gRDFC
Categories
(Core Graveyard :: RDF, defect)
Core Graveyard
RDF
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bambas, Assigned: axel)
Details
Attachments
(2 files, 1 obsolete file)
476 bytes,
application/x-javascript
|
Details | |
1.25 KB,
patch
|
benjamin
:
review+
shaver
:
superreview+
benjamin
:
approval1.8b3+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•19 years ago
|
||
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•19 years ago
|
||
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.
Assignee | ||
Comment 3•19 years ago
|
||
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.
Reporter | ||
Comment 4•19 years ago
|
||
(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.
Assignee | ||
Comment 5•19 years ago
|
||
CreateInstance calls into Create, see http://lxr.mozilla.org/seamonkey/source/rdf/build/nsRDFModule.cpp#204. Thus, #2 (and #3) still stand.
Reporter | ||
Comment 6•19 years ago
|
||
(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.
Assignee | ||
Comment 7•19 years ago
|
||
Assignee: nobody → axel
Status: UNCONFIRMED → ASSIGNED
Assignee | ||
Comment 8•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Attachment #187772 -
Flags: superreview?(shaver)
Attachment #187772 -
Flags: review?(benjamin)
Comment 9•19 years ago
|
||
Comment on attachment 187772 [details] [diff] [review] first, addref, then check rv That's a bit icky, but ok.
Attachment #187772 -
Flags: review?(benjamin) → review+
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.
Attachment #187772 -
Flags: superreview?(shaver) → superreview-
Assignee | ||
Comment 11•19 years ago
|
||
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.
Attachment #187772 -
Attachment is obsolete: true
Attachment #187796 -
Flags: superreview?(shaver)
Attachment #187796 -
Flags: review?(benjamin)
Comment on attachment 187796 [details] [diff] [review] use nsCOMPtr instead of manually addref, that's easier to track That's my man. sr=shaver.
Attachment #187796 -
Flags: superreview?(shaver) → superreview+
Updated•19 years ago
|
Attachment #187796 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 13•19 years ago
|
||
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
Attachment #187796 -
Flags: approval1.8b3?
Reporter | ||
Comment 14•19 years ago
|
||
(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•19 years ago
|
||
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
Attachment #187796 -
Flags: approval1.8b3? → approval1.8b3+
Assignee | ||
Comment 16•19 years ago
|
||
Checked in a few hours ago.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•