1650501 - LeakSanitizer: [@ NewString]

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

// Mixed in from js/src/jit-test/tests/basic/external-strings.js
newString("", {
    maybeExternal: true
});

AR=ar sh ./configure --enable-address-sanitizer --disable-jemalloc --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

Using clang 9.0.1

LSAN_OPTIONS=detect_leaks=1 ./js-64-asan-linux-x86_64-efa2336315ed --fuzzing-safe --ion-offthread-compile=off --ion-eager testcase.js

=================================================================
==8271==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 1 byte(s) in 1 object(s) allocated from:
    #0 0x558d1faf27fd in malloc (/home/skygentoo/shell-cache/js-64-asan-linux-x86_64-efa2336315ed/js-64-asan-linux-x86_64-efa2336315ed+0x1f377fd)
    #1 0x558d2084c489 in js_arena_malloc(unsigned long, unsigned long) /home/skygentoo/shell-cache/js-64-asan-linux-x86_64-efa2336315ed/objdir-js/dist/include/js/Utility.h:385:10
    #2 0x558d2084c489 in char16_t* js_pod_arena_malloc<char16_t>(unsigned long, unsigned long) /home/skygentoo/shell-cache/js-64-asan-linux-x86_64-efa2336315ed/objdir-js/dist/include/js/Utility.h:593:26
    #3 0x558d2084c489 in char16_t* js::MallocProvider<JSContext>::maybe_pod_arena_malloc<char16_t>(unsigned long, unsigned long) /home/skygentoo/trees/mozilla-central/js/src/vm/MallocProvider.h:53:12
    #4 0x558d2084c489 in char16_t* js::MallocProvider<JSContext>::pod_arena_malloc<char16_t>(unsigned long, unsigned long) /home/skygentoo/trees/mozilla-central/js/src/vm/MallocProvider.h:105:12
    #5 0x558d2084c489 in mozilla::UniquePtr<char16_t [], JS::FreePolicy> js::MallocProvider<JSContext>::make_pod_arena_array<char16_t>(unsigned long, unsigned long) /home/skygentoo/trees/mozilla-central/js/src/vm/MallocProvider.h:149:43
    #6 0x558d2084c489 in mozilla::UniquePtr<char16_t [], JS::FreePolicy> js::MallocProvider<JSContext>::make_pod_array<char16_t>(unsigned long) /home/skygentoo/trees/mozilla-central/js/src/vm/MallocProvider.h:154:12
    #7 0x558d2084c489 in NewString(JSContext*, unsigned int, JS::Value*) /home/skygentoo/trees/mozilla-central/js/src/builtin/TestingFunctions.cpp:1956:20
    #8 0x558d1fd34893 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:484:13
    #9 0x558d1fd34893 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:576:12
    #10 0x558d2123dacb in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /home/skygentoo/trees/mozilla-central/js/src/jit/BaselineIC.cpp:2992:10
    #11 0x3970ba7c0bc7  (<unknown module>)
    #12 0x6210000bb95f  (<unknown module>)
    #13 0x3970ba7be49e  (<unknown module>)

SUMMARY: AddressSanitizer: 1 byte(s) leaked in 1 allocation(s).

Probably related to:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/ccb2b543e09c
user:        Steve Fink
date:        Thu Jun 11 18:15:25 2020 +0000
summary:     Bug 1644243 - Add a newString testing function for creating various types of JSString r=jonco

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Steve, guessing related to bug 1644243, and this is probably not s-s.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1644243

Comment 3

[Kim Moir [:kmoir] ET]

sdetar: triaging as REO for 79 - can someone from your team look at this bug?

Comment 4

[Steven DeTar [:sdetar]]

sfink, as we talked about to day can you take a look at this bug.

Comment 5

[Steve Fink [:sfink] [:s:]]

I don't think this is going to be a serious problem, since it's a JS shell-only testing function that uses some dummied-up external string callbacks. That said, I need to figure out why it's happening, and whether it might apply to the real external strings used by the browser.

Comment 6

[André Bargull [:anba]]

https://searchfox.org/mozilla-central/rev/3b6958c26049c1e27b2790a43154caaba9f6dd4a/js/src/builtin/TestingFunctions.cpp#1998-2000 is ignoring the isExternal output-param (the ownership isn't transferred when isExternal is false, so in that case it's wrong to call buf.release).

Comment 7

[Steve Fink [:sfink] [:s:]]

(In reply to André Bargull [:anba] from comment #6)

https://searchfox.org/mozilla-central/rev/3b6958c26049c1e27b2790a43154caaba9f6dd4a/js/src/builtin/TestingFunctions.cpp#1998-2000 is ignoring the isExternal output-param (the ownership isn't transferred when isExternal is false, so in that case it's wrong to call buf.release).

Ouch, good catch. Thanks. Though that would be a double-free, not the leak here.

(Er... though the other way around? If isExternal is false, we do want to buf.release; nothing is using that memory anymore. If isExternal is true, then it's wrong to call buf.release.)

Comment 8

[André Bargull [:anba]]

buf.release doesn't release (= free) the memory, it releases the ownership of the unique-ptr.

Comment 9

[Steve Fink [:sfink] [:s:]]

(In reply to André Bargull [:anba] from comment #8)

buf.release doesn't release (= free) the memory, it releases the ownership of the unique-ptr.

Uh... can you and whoever else has read this please promise to pretend that you never saw it?

I need to get my brain repaired.

Comment 10

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1650501 - Fix memory handling in NewString test function

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cfe5daf07a99
Fix memory handling in NewString test function r=anba

Comment 12

[Atila Butkovits]

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

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.