Closed Bug 1467752 Opened 5 years ago Closed 5 years ago

Memory leak with OOM in CloneBufferObject::getCloneBufferAsArrayBuffer_impl(JSContext*, JS::CallArgs const&)


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox62 --- fixed


(Reporter: anba, Assigned: Waldo)



(1 file)

|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].


Test case:
var cb = serialize([]);


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

==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== 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== 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== 24 bytes in 1 blocks are definitely lost in loss record 1 of 4
==16215==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/
==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== 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== For counts of detected and suppressed errors, rerun with: -v
==16215== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
Attached patch PatchSplinter Review
Attachment #8985423 - Flags: review?(andrebargull)
Assignee: nobody → jwalden+bmo
Comment on attachment 8985423 [details] [diff] [review]

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...

Attachment #8985423 - Flags: review?(andrebargull) → review+
Pushed by
Don't leak in case of OOM in getCloneBufferAsArrayBuffer_impl when JS_NewArrayBufferWithContents fails.  r=anba
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.