Closed Bug 333704 Opened 14 years ago Closed 13 years ago

We leak key in txNodeSorter::addSortElement

Categories

(Core :: XSLT, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: jwatt, Assigned: sciguyryan)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, memory-leak)

Attachments

(1 file, 8 obsolete files)

We really can't use the NS_ENSURE_ macros in txNodeSorter::addSortElement because we'll leak the SortKey.
We should really stick to early returns, but use an nsAutoPtr for the key up
to the point where we actually pass the ownership over to mSortKeys.
Attached patch Patch v1 (obsolete) — Splinter Review
Patch v1. Swaps the |NS_ENSURE_SUCCESS| in return for early returns.
Assignee: xslt → sciguyryan+bugzilla
Status: NEW → ASSIGNED
Attachment #242432 - Flags: superreview?(peterv)
Attachment #242432 - Flags: review?(peterv)
Comment on attachment 242432 [details] [diff] [review]
Patch v1

Make key an nsAutoPtr instead (use forget when adding it to mSortKeys).
Attachment #242432 - Flags: superreview?(peterv)
Attachment #242432 - Flags: superreview-
Attachment #242432 - Flags: review?(peterv)
Attachment #242432 - Flags: review-
(In reply to comment #3)
> (From update of attachment 242432 [details] [diff] [review] [edit])
> Make key an nsAutoPtr instead (use forget when adding it to mSortKeys).
> 

I was under the impression that XSLT could be built standalone. Wouldn't a reliance on XPCOM for nsAutoPtr break that?
the standalone engine relies on XPCOM, just not the rest of gecko.
(In reply to comment #5)
> the standalone engine relies on XPCOM, just not the rest of gecko.
> 

Ah. I see. In that case I'll make a new patch using nsAutoPtr :)
Attached patch Patch v2 (obsolete) — Splinter Review
Patch v2

* Return |rv| from a method call where failure is detected.
* Switch too |nsAutoPtr|.
* Favour early return rather than use of |NS_ENSURE_TRUE|.
Attachment #242432 - Attachment is obsolete: true
Attachment #242511 - Flags: superreview?(peterv)
Attachment #242511 - Flags: review?(peterv)
Comment on attachment 242511 [details] [diff] [review]
Patch v2

>-        NS_ENSURE_SUCCESS(rv, rv);
>+        if (NS_FAILED(rv)) {
>+            return rv;
>+        }

No, don't make this change. The two things mean the same thing, but your version bloats the source more. In release builds the NS_ENSURE_SUCCESS macro expands to the exact same thing.
Attachment #242511 - Flags: review?(peterv) → review-
Attached patch Patch v3 (obsolete) — Splinter Review
(In reply to comment #8)
> No, don't make this change. The two things mean the same thing, but your
> version bloats the source more. In release builds the NS_ENSURE_SUCCESS macro
> expands to the exact same thing.
> 

Updated the patch, no removal of the |NS_ENSURE_TRUE| calls.
Attachment #242511 - Attachment is obsolete: true
Attachment #242520 - Flags: superreview?(peterv)
Attachment #242520 - Flags: review?(bugmail)
Attachment #242511 - Flags: superreview?(peterv)
Comment on attachment 242520 [details] [diff] [review]
Patch v3

>-    mSortKeys.add(key);
>+    // mSortKeys owns key now.
>+    mSortKeys.add(key.forget());

This is wrong in the current code too, but please fix this call too since it too can cause a leak. mSortKeys.add can fail (it returns an nsresult), so only call key.forget() once you've checked the return value of the add-call.
Attachment #242520 - Flags: review?(bugmail) → review-
Attached patch Patch v3.1 (obsolete) — Splinter Review
Patched to Jonas Sicking - appear to have missed that one sorry.
Attachment #242520 - Attachment is obsolete: true
Attachment #242531 - Flags: review?(bugmail)
Attachment #242520 - Flags: superreview?(peterv)
Comment on attachment 242531 [details] [diff] [review]
Patch v3.1

>Index: txNodeSorter.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xslt/src/xslt/txNodeSorter.cpp,v
>retrieving revision 1.21
>diff -u -8 -p -r1.21 txNodeSorter.cpp
>--- txNodeSorter.cpp	1 Apr 2006 02:12:15 -0000	1.21
>+++ txNodeSorter.cpp	17 Oct 2006 18:33:49 -0000
>@@ -65,17 +65,17 @@ txNodeSorter::~txNodeSorter()
>     }
> }
> 
> nsresult
> txNodeSorter::addSortElement(Expr* aSelectExpr, Expr* aLangExpr,
>                              Expr* aDataTypeExpr, Expr* aOrderExpr,
>                              Expr* aCaseOrderExpr, txIEvalContext* aContext)
> {
>-    SortKey* key = new SortKey;
>+    nsAutoPtr<SortKey> key(new SortKey);

Nit: I prefer this to be |nsAutoPtr<SortKey> key = new SortKey;| since that's IMHO more readable.

>-    mSortKeys.add(key);
>+    // mSortKeys owns key now. 
>+    nsresult success = mSortKeys.add(key.get());
>     mNKeys++;
> 
>+    if (NS_SUCCEEDED(success)) {
>+        key.forget();
>+    }
>+
>     return NS_OK;

This looses the error code if one is return, i.e. you want that error to be returned and not just forgotten. So simply do what all other callsites do in this function and use

rv = ...
NS_ENSURE_SUCCESS(rv, rv);
Attachment #242531 - Flags: review?(bugmail) → review-
Attached patch patch v3.2 (obsolete) — Splinter Review
Addressing nits!
Attachment #242531 - Attachment is obsolete: true
Attachment #242535 - Flags: review?(bugmail)
> >+    nsAutoPtr<SortKey> key(new SortKey);
> 
> Nit: I prefer this to be |nsAutoPtr<SortKey> key = new SortKey;| since that's
> IMHO more readable.

I prefer this too, but I think some compilers won't accept it. So it should be |nsAutoPtr<SortKey> key(new SortKey);| after all.
Attached patch Patch v3.3 (obsolete) — Splinter Review
Changed back too |nsAutoPtr<SortKey> key(new SortKey);| Peter's comment :)
Attachment #242535 - Attachment is obsolete: true
Attachment #242540 - Flags: superreview?(peterv)
Attachment #242540 - Flags: review?(bugmail)
Attachment #242535 - Flags: review?(bugmail)
should you really increase mNKeys when add failed?
Comment on attachment 242540 [details] [diff] [review]
Patch v3.3

>-    mSortKeys.add(key);
>+    // mSortKeys owns key now. 
>+    nsresult success = mSortKeys.add(key.get());
>     mNKeys++;

You never check the |success| variable. Why not use |rv|? Additionally you should check if the call succeeded before doing anything else. And no need to call .get() expclicitly, that conversion is done automatically.
Attachment #242540 - Flags: superreview?(peterv)
Attachment #242540 - Flags: review?(bugmail)
Attachment #242540 - Flags: review-
Attached patch Patch v3.4 (obsolete) — Splinter Review
Hopefully I didn't make any more mistakes!

Moved |mNKeys++| after the |NS_ENSURE_SUCCESS| (stupid error).
Changed |success| too |rv| which is now checked (another stupid error).
Removed the explicit use of |key.get()| in favour of auto-casting.
Attachment #242540 - Attachment is obsolete: true
Attachment #242546 - Flags: superreview?(peterv)
Attachment #242546 - Flags: review?(bugmail)
Comment on attachment 242546 [details] [diff] [review]
Patch v3.4

Wrong one sorry.
Attachment #242546 - Attachment is obsolete: true
Attachment #242546 - Flags: superreview?(peterv)
Attachment #242546 - Flags: review?(bugmail)
Attached patch Correct patch! (obsolete) — Splinter Review
Uploaded the wrong patch last time. Sorry for the bugspam.
Attachment #242548 - Flags: superreview?(peterv)
Attachment #242548 - Flags: review?(bugmail)
Comment on attachment 242548 [details] [diff] [review]
Correct patch!

>-    mSortKeys.add(key);
>+    // mSortKeys owns key now. 
>+    rv = mSortKeys.add(key);
>+
>+    NS_ENSURE_SUCCESS(rv, rv);

Nit: Put the empty line after the NS_ENSURE_SUCCESS, as the ENSURE belongs together with the call.

>     mNKeys++;
>+    key.forget();

Nit: Put the call to forget() before the call to mNKeys++ so that we don't accidentally put more code in later that can cause other early returns.

r/sr=sicking with that.
Attachment #242548 - Flags: superreview?(peterv)
Attachment #242548 - Flags: superreview+
Attachment #242548 - Flags: review?(bugmail)
Attachment #242548 - Flags: review+
Attached patch Nits-be-goneSplinter Review
Final patch to Jonas' nits! Thanks for all the reviews and comments (and your patience with a n00b).
Attachment #242548 - Attachment is obsolete: true
Attachment #242554 - Flags: superreview?(bugmail)
Attachment #242554 - Flags: review?(bugmail)
Attachment #242554 - Flags: superreview?(bugmail)
Attachment #242554 - Flags: superreview+
Attachment #242554 - Flags: review?(bugmail)
Attachment #242554 - Flags: review+
Whiteboard: [checkin needed]
mozilla/content/xslt/src/xslt/txNodeSorter.cpp 	1.22
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
You need to log in before you can comment on or make changes to this bug.