Closed
Bug 1467752
Opened 5 years ago
Closed 5 years ago
Memory leak with OOM in CloneBufferObject::getCloneBufferAsArrayBuffer_impl(JSContext*, JS::CallArgs const&)
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: anba, Assigned: Waldo)
Details
Attachments
(1 file)
1.52 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
|JS_NewArrayBufferWithContents(...)| doesn't take ownership of the passed void* on OOM, which means releasing the UniquePtr in |buffer.release()| can lead to a leak [1]. [1] https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/js/src/builtin/TestingFunctions.cpp#2989 Test case: --- var cb = serialize([]); oomAtAllocation(2); print(cb.arraybuffer); --- 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 /tmp/t.js Output: --- ==16215== Memcheck, a memory error detector ==16215== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al. ==16215== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info ==16215== Command: /home/andre/hg/mozilla-inbound/js/src/build-valgrind-debug-obj/dist/bin/js /tmp/t.js ==16215== ==16215== Warning: set address range perms: large range [0x2941a4a6d000, 0x2941e4a6d000) (noaccess) uncaught exception: out of memory (Unable to print stack trace) ==16215== Warning: set address range perms: large range [0x2941a4a6d000, 0x2941e4a6d000) (noaccess) ==16215== ==16215== HEAP SUMMARY: ==16215== in use at exit: 72,803 bytes in 4 blocks ==16215== total heap usage: 8,935 allocs, 8,931 frees, 6,041,583 bytes allocated ==16215== ==16215== 24 bytes in 1 blocks are definitely lost in loss record 1 of 4 ==16215== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==16215== by 0x492024: SystemMalloc::malloc(unsigned long) (malloc_decls.h:37) ==16215== by 0x491F58: DummyArenaAllocator<SystemMalloc>::moz_arena_malloc(unsigned long, unsigned long) (malloc_decls.h:37) ==16215== by 0x491E4C: moz_arena_malloc (malloc_decls.h:115) ==16215== by 0xBC0572: js_malloc(unsigned long) (Utility.h:387) ==16215== by 0xC1442D: CloneBufferObject::getCloneBufferAsArrayBuffer_impl(JSContext*, JS::CallArgs const&) (TestingFunctions.cpp:2989) ==16215== by 0xC142CA: bool JS::CallNonGenericMethod<&CloneBufferObject::is, &CloneBufferObject::getCloneBufferAsArrayBuffer_impl>(JSContext*, JS::CallArgs const&) (CallNonGenericMethod.h:100) ==16215== by 0xC03D43: CloneBufferObject::getCloneBufferAsArrayBuffer(JSContext*, unsigned int, JS::Value*) (in /home/andre/hg/mozilla-inbound/js/src/build-valgrind-debug-obj/dist/bin/js) ==16215== by 0x6720B3: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (JSContext-inl.h:274) ==16215== by 0x660BCE: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:471) ==16215== by 0x66119F: InternalCall(JSContext*, js::AnyInvokeArgs const&) (Interpreter.cpp:520) ==16215== by 0x661215: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) (Interpreter.cpp:539) ==16215== ==16215== LEAK SUMMARY: ==16215== definitely lost: 24 bytes in 1 blocks ==16215== indirectly lost: 0 bytes in 0 blocks ==16215== possibly lost: 0 bytes in 0 blocks ==16215== still reachable: 72,779 bytes in 3 blocks ==16215== suppressed: 0 bytes in 0 blocks ==16215== Reachable blocks (those to which a pointer was found) are not shown. ==16215== To see them, rerun with: --leak-check=full --show-leak-kinds=all ==16215== ==16215== For counts of detected and suppressed errors, rerun with: -v ==16215== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0) ---
Assignee | ||
Comment 1•5 years ago
|
||
Attachment #8985423 -
Flags: review?(andrebargull)
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Reporter | ||
Comment 2•5 years ago
|
||
Comment on attachment 8985423 [details] [diff] [review] Patch Review of attachment 8985423 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/TestingFunctions.cpp @@ +3002,2 @@ > return false; > + } I think we more often use |mozilla::Unused| when giving up ownership of a UniquePtr after a successful operation, that means: --- JSObject* arrayBuffer = JS_NewArrayBufferWithContents(cx, size, buffer.get()); if (!arrayBuffer) return false; mozilla::Unused << buffer.release(); --- The |mozilla::Unused| approach seems to be more predominant [1] when compared to releasing the UniquePtr to a temporary variable [2], but whether that means it's the better resp. more clearer approach to represent the ownership is probably up to discussion... [1] https://searchfox.org/mozilla-central/search?q=%3C%3C+%5Cw%2B%5C.release%5C(%5C)&case=true®exp=true&path= [2] https://searchfox.org/mozilla-central/search?q=%5Cw%2B(%5C*%3F%5Cs%2B%7C%5Cs%2B%5C*%3F)%5Cw%2B+%3D+%5Cw%2B%5C.release%5C(%5C)&case=true®exp=true&path=
Attachment #8985423 -
Flags: review?(andrebargull) → review+
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/a194fdbd87f9 Don't leak in case of OOM in getCloneBufferAsArrayBuffer_impl when JS_NewArrayBufferWithContents fails. r=anba
Comment 4•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a194fdbd87f9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•