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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

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

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Comment 2

[André Bargull [:anba]]

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&regexp=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&regexp=true&path=

Comment 3

[Pulsebot]

Pushed by [e-mail]:
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

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/a194fdbd87f9