Closed Bug 1467275 Opened 2 years ago Closed 2 years ago

Memory leak with OOM in JS_BufferIsCompilableUnit(JSContext*, JS::Handle<JSObject*>, char const*, unsigned long)

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: anba, Assigned: Waldo)

Details

Attachments

(1 file)

|chars| is not freed when |frontend::UsedNameTracker::init()| resp. |frontend::CreateScriptSourceObject(...)| in these lines [1] return false.


[1] https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/js/src/jsapi.cpp#4437,4447,4452


Test case:
---
1. Start js with valgrind
2. Enter: oomAtAllocation(3);
3. Enter: function f(){}
4. Enter: quit()
---


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-valgrind-debug-obj/dist/bin/js 


Output:
---
==5448== Memcheck, a memory error detector
==5448== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==5448== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==5448== Command: /home/andre/hg/mozilla-inbound/js/src/build-valgrind-debug-obj/dist/bin/js
==5448== 
==5448== Warning: set address range perms: large range [0x3f48bc49b000, 0x3f48fc49b000) (noaccess)
js> oomAtAllocation(3);      
js> function f(){}

js> quit()


==5448== Warning: set address range perms: large range [0x3f48bc49b000, 0x3f48fc49b000) (noaccess)
==5448== 
==5448== HEAP SUMMARY:
==5448==     in use at exit: 72,960 bytes in 12 blocks
==5448==   total heap usage: 8,956 allocs, 8,944 frees, 6,051,446 bytes allocated
==5448== 
==5448== 32 bytes in 1 blocks are definitely lost in loss record 2 of 7
==5448==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5448==    by 0x491E24: SystemMalloc::malloc(unsigned long) (malloc_decls.h:37)
==5448==    by 0x491D58: DummyArenaAllocator<SystemMalloc>::moz_arena_malloc(unsigned long, unsigned long) (malloc_decls.h:37)
==5448==    by 0x491C4C: moz_arena_malloc (malloc_decls.h:115)
==5448==    by 0x41E632: js_malloc(unsigned long) (Utility.h:388)
==5448==    by 0x4293E1: char16_t* js_pod_malloc<char16_t>(unsigned long) (Utility.h:578)
==5448==    by 0x477A13: char16_t* js::MallocProvider<JSContext>::maybe_pod_malloc<char16_t>(unsigned long) (MallocProvider.h:54)
==5448==    by 0x4773CE: char16_t* js::MallocProvider<JSContext>::pod_malloc<char16_t>(unsigned long) (MallocProvider.h:87)
==5448==    by 0xE4CA0F: JS::TwoByteCharsZ InflateUTF8StringHelper<(InflateUTF8Action)0, JS::TwoByteCharsZ, JSContext>(JSContext*, JS::UTF8Chars, unsigned long*) (CharacterEncoding.cpp:402)
==5448==    by 0xE4C91F: JS::UTF8CharsToNewTwoByteCharsZ(JSContext*, JS::UTF8Chars, unsigned long*) (CharacterEncoding.cpp:425)
==5448==    by 0xD7B4D7: JS_BufferIsCompilableUnit(JSContext*, JS::Handle<JSObject*>, char const*, unsigned long) (jsapi.cpp:4450)
==5448==    by 0x4478D4: ReadEvalPrintLoop(JSContext*, _IO_FILE*, bool) (js.cpp:1238)
==5448== 
==5448== 43 bytes in 4 blocks are definitely lost in loss record 5 of 7
==5448==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5448==    by 0x5DA1489: strdup (strdup.c:42)
==5448==    by 0x4EB898: readline (editline.c:1000)
==5448==    by 0x447F6C: GetLine(_IO_FILE*, char const*) (js.cpp:660)
==5448==    by 0x447728: ReadEvalPrintLoop(JSContext*, _IO_FILE*, bool) (js.cpp:1216)
==5448==    by 0x446C3A: Process(JSContext*, char const*, bool, FileKind) (js.cpp:1334)
==5448==    by 0x42B661: ProcessArgs(JSContext*, js::cli::OptionParser*) (js.cpp:8435)
==5448==    by 0x42453D: Shell(JSContext*, js::cli::OptionParser*, char**) (js.cpp:8889)
==5448==    by 0x420E5B: main (js.cpp:9362)
==5448== 
==5448== LEAK SUMMARY:
==5448==    definitely lost: 75 bytes in 5 blocks
==5448==    indirectly lost: 0 bytes in 0 blocks
==5448==      possibly lost: 0 bytes in 0 blocks
==5448==    still reachable: 72,885 bytes in 7 blocks
==5448==         suppressed: 0 bytes in 0 blocks
==5448== Reachable blocks (those to which a pointer was found) are not shown.
==5448== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==5448== 
==5448== For counts of detected and suppressed errors, rerun with: -v
==5448== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)
---
Attached patch PatchSplinter Review
Attachment #8985426 - Flags: review?(andrebargull)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Comment on attachment 8985426 [details] [diff] [review]
Patch

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

::: js/src/jsapi.cpp
@@ +4387,5 @@
>  
>      cx->clearPendingException();
>  
> +    UniquePtr<char16_t> chars
> +        { JS::UTF8CharsToNewTwoByteCharsZ(cx, JS::UTF8Chars(utf8, length), &length).get() };

Just to make sure I don't miss something here, the curly braces in this case don't make any semantic difference when compared to normal parentheses, right?
Attachment #8985426 - Flags: review?(andrebargull) → review+
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/845878e00f84
Don't leak |chars| when an error occurs after its allocation in JS_BufferIsCompilableUnit.  r=anba
https://hg.mozilla.org/mozilla-central/rev/845878e00f84
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.