Memory leak when copying JSErrorReport

RESOLVED FIXED in Firefox 62

Status

()

defect
RESOLVED FIXED
Last year
Last year

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

js::ErrorToException(...) [1] copies the JSErrorReport including its JSErrorNotes, but stores the result in a js::ScopedJSFreePtr, so when ScopedJSFreePtr's destructor runs, the result is only freed, which means JSErrorReport's destructor isn't executed and the copied JSErrorNotes will leak.

Similar issues are probably present in js::CopyErrorObject(...) [2] and JS::CreateError(...) [3].

[1] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/js/src/jsexn.cpp#689
[2] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/js/src/jsexn.cpp#993-998
[3] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/js/src/jsexn.cpp#1026-1028


Test case:
---
oomAfterAllocations(140);

Function("let a; let a;");
---

Configure flags: --enable-debug --disable-optimize --disable-tests --enable-valgrind --disable-jemalloc

Run with: valgrind --tool=memcheck --leak-check=yes ~/hg/mozilla-inbound/js/src/build-debug-valgrind-obj/dist/bin/js /tmp/q.js


Output:
---
==9105== Memcheck, a memory error detector
==9105== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==9105== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==9105== Command: /home/andre/hg/mozilla-inbound/js/src/build-debug-valgrind-obj/dist/bin/js /tmp/q.js
==9105== 
==9105== Warning: set address range perms: large range [0x26979c595000, 0x2697dc595000) (noaccess)
out of memory initializing ErrorReport
==9105== Warning: set address range perms: large range [0x26979c595000, 0x2697dc595000) (noaccess)
==9105== 
==9105== HEAP SUMMARY:
==9105==     in use at exit: 72,909 bytes in 5 blocks
==9105==   total heap usage: 8,898 allocs, 8,893 frees, 6,001,222 bytes allocated
==9105== 
==9105== 130 (48 direct, 82 indirect) bytes in 1 blocks are definitely lost in loss record 4 of 5
==9105==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9105==    by 0x492044: SystemMalloc::malloc(unsigned long) (malloc_decls.h:37)
==9105==    by 0x491F78: DummyArenaAllocator<SystemMalloc>::moz_arena_malloc(unsigned long, unsigned long) (malloc_decls.h:37)
==9105==    by 0x491E6C: moz_arena_malloc (malloc_decls.h:115)
==9105==    by 0x5EA512: js_malloc(unsigned long) (Utility.h:388)
==9105==    by 0x5EBAA3: JSErrorNotes* js_new<JSErrorNotes>() (Utility.h:504)
==9105==    by 0x60DE57: js::detail::UniqueSelector<JSErrorNotes>::SingleObject js::MakeUnique<JSErrorNotes>() (in /home/andre/hg/mozilla-inbound/js/src/build-debug-valgrind-obj/dist/bin/js)
==9105==    by 0xD84D61: JSErrorNotes::copy(JSContext*) (jsapi.cpp:7159)
==9105==    by 0xD8A5A6: CopyExtraData(JSContext*, unsigned char**, JSErrorReport*, JSErrorReport*) (jsexn.cpp:252)
==9105==    by 0xD8ACBA: JSErrorReport* CopyErrorHelper<JSErrorReport>(JSContext*, JSErrorReport*) (jsexn.cpp:316)
==9105==    by 0xD8AABC: js::CopyErrorReport(JSContext*, JSErrorReport*) (jsexn.cpp:341)
==9105==    by 0xD8BCED: js::ErrorToException(JSContext*, JSErrorReport*, JSErrorFormatString const* (*)(void*, unsigned int), void*) (jsexn.cpp:689)
==9105== 
==9105== LEAK SUMMARY:
==9105==    definitely lost: 48 bytes in 1 blocks
==9105==    indirectly lost: 82 bytes in 1 blocks
==9105==      possibly lost: 0 bytes in 0 blocks
==9105==    still reachable: 72,779 bytes in 3 blocks
==9105==         suppressed: 0 bytes in 0 blocks
==9105== Reachable blocks (those to which a pointer was found) are not shown.
==9105== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==9105== 
==9105== For counts of detected and suppressed errors, rerun with: -v
==9105== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
---
Posted patch bug1465350.patch (obsolete) — Splinter Review
I took the easy way out and simply switched the ScopedJSFreePtr to a UniquePtr to ensure the destructor runs. 

And then I've also changed the parameter to pass the UniquePtr by value instead of using a pointer to more clearly express the ownership and fixed a missing error detection in JS::CreateError(...).
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8984173 - Flags: review?(jwalden+bmo)
anba++
Comment on attachment 8984173 [details] [diff] [review]
bug1465350.patch

Review of attachment 8984173 [details] [diff] [review]:
-----------------------------------------------------------------

If I'm not mistaken, you've touched every call of CopyErrorReport, but you've kept it returning a dumb pointer.  I'm moderately worried about returning a raw pointer and implicitly assuming a deletion-policy by assigning directly into UniquePtr with no policy specified, particularly when in some contexts UniquePtr has one policy, and in other context it has another.  It doesn't look like there's any reason it couldn't return UniquePtr, right?  Please make that change while you're doing this.
Attachment #8984173 - Flags: review?(jwalden+bmo) → review-
Updated patch to return a UniquePtr from CopyErrorHelper per comment #3.
Attachment #8984173 - Attachment is obsolete: true
Attachment #8985194 - Flags: review?(jwalden+bmo)
Attachment #8985194 - Flags: review?(jwalden+bmo) → review+
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6350b1a6097e
Use UniquePtr instead of ScopedJSFreePtr for JSErrorReporter. r=Waldo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6350b1a6097e
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.