Closed Bug 1467753 Opened Last year Closed Last year

Memory leak with OOM in CopyStringPure(JSContext*, JSString*)

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)

|NewString<CanGC>(...)| [1] resp. |NewStringDontDeflate<CanGC>(...)| [2] don't take ownership of the passed char/char16_t* on OOM, so directly releasing the scoped pointer (or soon unique pointer - bug 1467438) in the call can lead to a leak.


[1] https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/js/src/vm/JSCompartment.cpp#286
[2] https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/js/src/vm/JSCompartment.cpp#293


Test case 1:
---
var g = newGlobal();

var s = String.fromCharCode(0xff);
var r = "";
for (var j = 0; j < 100; ++j) {
    r = s + r;
}

g.String.prototype.toString.call("");

oomAtAllocation(71);
g.String.prototype.toString.call(r);
---


Test case 2:
---
var g = newGlobal();

var s = String.fromCharCode(0x100);
var r = "";
for (var j = 0; j < 100; ++j) {
    r = s + r;
}

g.String.prototype.toString.call("");

oomAtAllocation(83);
g.String.prototype.toString.call(r);
---



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 --no-baseline /tmp/t.js


Output 1:
---
==16372== Memcheck, a memory error detector
==16372== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==16372== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==16372== Command: /home/andre/hg/mozilla-inbound/js/src/build-valgrind-debug-obj/dist/bin/js --no-baseline /tmp/t.js
==16372== 
==16372== Warning: set address range perms: large range [0x35ffd92f1000, 0x3600192f1000) (noaccess)
uncaught exception: out of memory
(Unable to print stack trace)
==16372== Warning: set address range perms: large range [0x35ffd92f1000, 0x3600192f1000) (noaccess)
==16372== 
==16372== HEAP SUMMARY:
==16372==     in use at exit: 72,880 bytes in 4 blocks
==16372==   total heap usage: 9,324 allocs, 9,320 frees, 6,185,133 bytes allocated
==16372== 
==16372== 101 bytes in 1 blocks are definitely lost in loss record 3 of 4
==16372==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==16372==    by 0x492024: SystemMalloc::malloc(unsigned long) (malloc_decls.h:37)
==16372==    by 0x491F58: DummyArenaAllocator<SystemMalloc>::moz_arena_malloc(unsigned long, unsigned long) (malloc_decls.h:37)
==16372==    by 0x491E4C: moz_arena_malloc (malloc_decls.h:115)
==16372==    by 0x41E7D2: js_malloc(unsigned long) (Utility.h:387)
==16372==    by 0x429401: unsigned char* js_pod_malloc<unsigned char>(unsigned long) (Utility.h:584)
==16372==    by 0x45C223: unsigned char* js::MallocProvider<JSContext>::maybe_pod_malloc<unsigned char>(unsigned long) (MallocProvider.h:54)
==16372==    by 0x45C0DE: unsigned char* js::MallocProvider<JSContext>::pod_malloc<unsigned char>(unsigned long) (MallocProvider.h:87)
==16372==    by 0x11426AF: mozilla::UniquePtr<unsigned char [], JS::FreePolicy> JSRope::copyCharsInternal<unsigned char>(JSContext*, bool) const (StringType.cpp:318)
==16372==    by 0x1127608: JSRope::copyLatin1CharsZ(JSContext*) const (StringType.cpp:285)
==16372==    by 0xF7F91F: CopyStringPure(JSContext*, JSString*) (JSCompartment.cpp:284)
==16372==    by 0xF7F58E: JS::Compartment::wrap(JSContext*, JS::MutableHandle<JSString*>) (JSCompartment.cpp:325)
==16372== 
==16372== LEAK SUMMARY:
==16372==    definitely lost: 101 bytes in 1 blocks
==16372==    indirectly lost: 0 bytes in 0 blocks
==16372==      possibly lost: 0 bytes in 0 blocks
==16372==    still reachable: 72,779 bytes in 3 blocks
==16372==         suppressed: 0 bytes in 0 blocks
==16372== Reachable blocks (those to which a pointer was found) are not shown.
==16372== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==16372== 
==16372== For counts of detected and suppressed errors, rerun with: -v
==16372== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
---


Output 2:
---
==16443== Memcheck, a memory error detector
==16443== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==16443== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==16443== Command: /home/andre/hg/mozilla-inbound/js/src/build-valgrind-debug-obj/dist/bin/js --no-baseline /tmp/t.js
==16443== 
==16443== Warning: set address range perms: large range [0x1ef126a80000, 0x1ef166a80000) (noaccess)
uncaught exception: out of memory
(Unable to print stack trace)
==16443== Warning: set address range perms: large range [0x1ef126a80000, 0x1ef166a80000) (noaccess)
==16443== 
==16443== HEAP SUMMARY:
==16443==     in use at exit: 72,981 bytes in 4 blocks
==16443==   total heap usage: 9,324 allocs, 9,320 frees, 6,185,238 bytes allocated
==16443== 
==16443== 202 bytes in 1 blocks are definitely lost in loss record 3 of 4
==16443==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==16443==    by 0x492024: SystemMalloc::malloc(unsigned long) (malloc_decls.h:37)
==16443==    by 0x491F58: DummyArenaAllocator<SystemMalloc>::moz_arena_malloc(unsigned long, unsigned long) (malloc_decls.h:37)
==16443==    by 0x491E4C: moz_arena_malloc (malloc_decls.h:115)
==16443==    by 0x41E7D2: js_malloc(unsigned long) (Utility.h:387)
==16443==    by 0x429621: char16_t* js_pod_malloc<char16_t>(unsigned long) (Utility.h:584)
==16443==    by 0x477D63: char16_t* js::MallocProvider<JSContext>::maybe_pod_malloc<char16_t>(unsigned long) (MallocProvider.h:54)
==16443==    by 0x47771E: char16_t* js::MallocProvider<JSContext>::pod_malloc<char16_t>(unsigned long) (MallocProvider.h:87)
==16443==    by 0x11429BF: mozilla::UniquePtr<char16_t [], JS::FreePolicy> JSRope::copyCharsInternal<char16_t>(JSContext*, bool) const (StringType.cpp:318)
==16443==    by 0x1127648: JSRope::copyTwoByteCharsZ(JSContext*) const (StringType.cpp:291)
==16443==    by 0xF7F9AF: CopyStringPure(JSContext*, JSString*) (JSCompartment.cpp:291)
==16443==    by 0xF7F58E: JS::Compartment::wrap(JSContext*, JS::MutableHandle<JSString*>) (JSCompartment.cpp:325)
==16443== 
==16443== LEAK SUMMARY:
==16443==    definitely lost: 202 bytes in 1 blocks
==16443==    indirectly lost: 0 bytes in 0 blocks
==16443==      possibly lost: 0 bytes in 0 blocks
==16443==    still reachable: 72,779 bytes in 3 blocks
==16443==         suppressed: 0 bytes in 0 blocks
==16443== Reachable blocks (those to which a pointer was found) are not shown.
==16443== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==16443== 
==16443== For counts of detected and suppressed errors, rerun with: -v
==16443== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
---
Attached patch PatchSplinter Review
Attachment #8985437 - Flags: review?(andrebargull)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
s/release/forget/g
Comment on attachment 8985437 [details] [diff] [review]
Patch

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

::: js/src/vm/Compartment.cpp
@@ +133,5 @@
>  
> +        auto* rawCopiedChars = copiedChars.release();
> +        auto* result = NewString<CanGC>(cx, rawCopiedChars, len);
> +        if (!result)
> +            js_free(rawCopiedChars);

Same remark as in bug 1467752, comment #2
Attachment #8985437 - Flags: review?(andrebargull) → review+
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fcf87cdfb39
Don't leak freshly-allocated, copied string chars in CopyStringPure when OOM happens allocating the JSString* that will adopt them.  r=anba
https://hg.mozilla.org/mozilla-central/rev/9fcf87cdfb39
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.