Memory leak in |JS_dtobasestr| (jsdtoa.c)

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Kenneth Herron, Assigned: Ryan Jones)

Tracking

({coverity, fixed1.8.0.10, fixed1.8.1.2})

Trunk
coverity, fixed1.8.0.10, fixed1.8.1.2
Points:
---
Bug Flags:
blocking1.8.1.2 +
blocking1.8.0.10 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Updated

11 years ago
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

11 years ago
NSPR's prdtoa.c doesn't have the equivalent of the
JS_dtobasestr function.
(Assignee)

Comment 3

11 years ago
Created attachment 242217 [details] [diff] [review]
Patch v1

Patch v1.

Simply use |free(buffer)| before the function returns.
Assignee: general → sciguyryan+bugzilla
Status: NEW → ASSIGNED
Attachment #242217 - Flags: superreview?(brendan)
Attachment #242217 - Flags: review?(brendan)

Comment 4

11 years ago
Same leak around line 3007-ish?
(Assignee)

Comment 5

11 years ago
(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 :)
(Assignee)

Comment 6

11 years ago
Created attachment 242222 [details] [diff] [review]
Patch v1.1

Updated to add a free for the same leak spotted by Brian Crowder.
Attachment #242217 - Attachment is obsolete: true
Attachment #242222 - Flags: superreview?(brendan)
Attachment #242222 - Flags: review?(brendan)
Attachment #242217 - Flags: superreview?(brendan)
Attachment #242217 - Flags: review?(brendan)
(Assignee)

Updated

11 years ago
Attachment #242222 - Flags: superreview?(brendan)
Attachment #242222 - Flags: review?(igor)
Attachment #242222 - Flags: review?(brendan)

Comment 7

11 years ago
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.
Attachment #242222 - Flags: review?(igor)
(Assignee)

Comment 8

11 years ago
Created attachment 251760 [details] [diff] [review]
Patch v2.0

Patch v2.0

* Updated to latest trunk build.
Attachment #242222 - Attachment is obsolete: true
Attachment #251760 - Flags: review?(igor)

Comment 9

11 years ago
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.
(Assignee)

Comment 10

11 years ago
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.
Attachment #251760 - Attachment is obsolete: true
Attachment #251764 - Flags: review?(igor)
Attachment #251760 - Flags: review?(igor)

Updated

11 years ago
Attachment #251764 - Flags: review?(igor) → review+
(Assignee)

Updated

11 years ago
Whiteboard: [checkin needed]

Comment 11

11 years ago
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. 
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.10?

Updated

11 years ago
Attachment #251764 - Flags: approval1.8.1.2?
Attachment #251764 - Flags: approval1.8.0.10?
Whiteboard: [checkin needed]

Comment 12

11 years ago
Comment on attachment 251764 [details] [diff] [review]
Patch v2.1

Approved for both branches, a=jay for drivers.
Attachment #251764 - Flags: approval1.8.1.2?
Attachment #251764 - Flags: approval1.8.1.2+
Attachment #251764 - Flags: approval1.8.0.10?
Attachment #251764 - Flags: approval1.8.0.10+

Updated

11 years ago
Flags: blocking1.8.1.2?
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10?
Flags: blocking1.8.0.10+

Updated

11 years ago
Depends on: 357392

Comment 13

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.0.10
Resolution: --- → FIXED

Comment 14

11 years ago
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.
Keywords: fixed1.8.1.2

Updated

11 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.