Last Comment Bug 351739 - Memory leak in |JS_dtobasestr| (jsdtoa.c)
: Memory leak in |JS_dtobasestr| (jsdtoa.c)
Status: RESOLVED FIXED
: coverity, fixed1.8.0.10, fixed1.8.1.2
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Ryan Jones
:
Mentors:
http://bonsai.mozilla.org/cvsblame.cg...
Depends on: 357392
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-07 16:07 PDT by Kenneth Herron
Modified: 2007-01-18 16:26 PST (History)
4 users (show)
jaymoz: blocking1.8.1.2+
jaymoz: blocking1.8.0.10+
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (885 bytes, patch)
2006-10-13 12:41 PDT, Ryan Jones
no flags Details | Diff | Splinter Review
Patch v1.1 (1.47 KB, patch)
2006-10-13 13:19 PDT, Ryan Jones
no flags Details | Diff | Splinter Review
Patch v2.0 (905 bytes, patch)
2007-01-17 03:43 PST, Ryan Jones
no flags Details | Diff | Splinter Review
Patch v2.1 (982 bytes, patch)
2007-01-17 04:07 PST, Ryan Jones
igor: review+
jaymoz: approval1.8.1.2+
jaymoz: approval1.8.0.10+
Details | Diff | Splinter Review

Description Kenneth Herron 2006-09-07 16:07:56 PDT
This is coverity ID 68. Please see the sample URL. The allocation at line 2928 is leaked if the call to |d2b| or |lshift| return null.
Comment 1 Brendan Eich [:brendan] 2006-09-07 16:36:53 PDT
Check NSPR's prdtoa.c.  The JS version forked long ago from David M. Gay's netlib hosted dtoa.c, and I think NSPR refreshed more recently.

/be
Comment 2 Wan-Teh Chang 2006-09-07 16:55:06 PDT
NSPR's prdtoa.c doesn't have the equivalent of the
JS_dtobasestr function.
Comment 3 Ryan Jones 2006-10-13 12:41:41 PDT
Created attachment 242217 [details] [diff] [review]
Patch v1

Patch v1.

Simply use |free(buffer)| before the function returns.
Comment 4 Brian Crowder 2006-10-13 12:49:27 PDT
Same leak around line 3007-ish?
Comment 5 Ryan Jones 2006-10-13 13:16:43 PDT
(In reply to comment #4)
> Same leak around line 3007-ish?
> 

Certainly looks like it. |buffer| isn't freed before then so I guess your right. I'll update the patch! Thanks for pointing that out :)
Comment 6 Ryan Jones 2006-10-13 13:19:43 PDT
Created attachment 242222 [details] [diff] [review]
Patch v1.1

Updated to add a free for the same leak spotted by Brian Crowder.
Comment 7 Igor Bukanov 2007-01-17 01:44:53 PST
Comment on attachment 242222 [details] [diff] [review]
Patch v1.1

This is not a pacth against  the trunk. Please update it  to reflect bug 357392.
Comment 8 Ryan Jones 2007-01-17 03:43:20 PST
Created attachment 251760 [details] [diff] [review]
Patch v2.0

Patch v2.0

* Updated to latest trunk build.
Comment 9 Igor Bukanov 2007-01-17 03:49:15 PST
Comment on attachment 251760 [details] [diff] [review]
Patch v2.0

I will review=+ for the patch with changed order of free and RELEASE_DTOA_LOCK calls to follow LIFO pattern.
Comment 10 Ryan Jones 2007-01-17 04:07:54 PST
Created attachment 251764 [details] [diff] [review]
Patch v2.1

Patch v2.1

* Same as patch v2.0 but updated to follow the LIFO pattern as suggested by Igor Bukanov.
Comment 11 Igor Bukanov 2007-01-17 04:29:38 PST
I committed the patch from comment 10 to the trunk:

Checking in jsdtoa.c;
/cvsroot/mozilla/js/src/jsdtoa.c,v  <--  jsdtoa.c
new revision: 3.39; previous revision: 3.38
done

Please mark the bug fixed after verifying that the committed code is the right one. 
Comment 12 Jay Patel [:jay] 2007-01-17 15:07:02 PST
Comment on attachment 251764 [details] [diff] [review]
Patch v2.1

Approved for both branches, a=jay for drivers.
Comment 13 Igor Bukanov 2007-01-18 15:01:07 PST
I committed the patch from comment 10 to MOZILLA_1_8_0_BRANCH:

Checking in jsdtoa.c;
/cvsroot/mozilla/js/src/jsdtoa.c,v  <--  jsdtoa.c
new revision: 3.33.10.3; previous revision: 3.33.10.2
done
Comment 14 Igor Bukanov 2007-01-18 15:06:33 PST
I also committed yesterday the patch from comment 10 to MOZILLA_1_8_BRANCH but forgot to update the bug. The committed version was 3.33.2.4.

Note You need to log in before you can comment on or make changes to this bug.