Closed Bug 1275867 Opened 4 years ago Closed 4 years ago

tests/wasm/basic.js and tests/wasm/basic-const.js fail when disabling jemalloc

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file)

Originally, I found that basic.js and basic-const.js failed on Windows when building with --disable-jemalloc.

FAILURES:
    --ion-eager --ion-offthread-compile=off c:\Users\Administrator\mozilla-central\js\src\jit-test\tests\wasm\basic-const.js
    --baseline-eager c:\Users\Administrator\mozilla-central\js\src\jit-test\tests\wasm\basic-const.js
    --no-baseline --no-ion c:\Users\Administrator\mozilla-central\js\src\jit-test\tests\wasm\basic-const.js
    c:\Users\Administrator\mozilla-central\js\src\jit-test\tests\wasm\basic.js
    --no-baseline --no-ion c:\Users\Administrator\mozilla-central\js\src\jit-test\tests\wasm\basic.js
    --baseline-eager c:\Users\Administrator\mozilla-central\js\src\jit-test\tests\wasm\basic.js

I tried on Linux as well, and there basic.js doesn't fail, but basic-const.js does:

/home/glandium/gecko/js/src/jit-test/tests/wasm/basic-const.js:7:9 Error: Assertion failed: got -0, expected -2.7550413506105542e-8
Stack:
  testConst@/home/glandium/gecko/js/src/jit-test/tests/wasm/basic-const.js:7:9
  @/home/glandium/gecko/js/src/jit-test/tests/wasm/basic-const.js:142:1
Exit code: 3

Interestingly, the error changed once running under rr:
/home/glandium/gecko/js/src/jit-test/tests/wasm/basic-const.js:7:9 Error: Assertion failed: got Infinity, expected 213826
Stack:
  testConst@/home/glandium/gecko/js/src/jit-test/tests/wasm/basic-const.js:7:9
  @/home/glandium/gecko/js/src/jit-test/tests/wasm/basic-const.js:143:1

That one, I tracked down to a buffer overflow in WasmTextToBinary.cpp's ParseFloatLiteral:

https://dxr.mozilla.org/mozilla-central/rev/8d0aadfe7da782d415363880008b4ca027686137/js/src/asmjs/WasmTextToBinary.cpp#1736-1740

buffer is not null terminated, so the string that ends up in js_strtod_harder can end up with "garbage" from previously parsed strings. In my case, in rr, it ended up with: 2.138260e+051055 instead of 2.138260e+05, and that's too much for a float32, so that ended up as infinity.

The obvious fix works, but it doesn't actually fix the error I was getting without rr. However, interestingly, now that I rerun rr, I found that it happens in rr, it just wasn't happening the first time.

So... more to come. (and assigning to myself, considering how far I've come)
It was also overallocated in the case of negative numbers, so fixed that
at the same time.

Review commit: https://reviewboard.mozilla.org/r/55442/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55442/
Attachment #8756829 - Flags: review?(bbouvier)
Comment on attachment 8756829 [details]
MozReview Request: Bug 1275867 - Null-terminate the buffer passed from ParseFloatLiteral to js_strtod_harder. r?bbouvier

https://reviewboard.mozilla.org/r/55442/#review52178

Wow, good catches, thanks. For history, is it not crashing with jemalloc because jemalloc zeroes bits before handing them to the caller?
Attachment #8756829 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> Comment on attachment 8756829 [details]
> MozReview Request: Bug 1275867 - Null-terminate the buffer passed from
> ParseFloatLiteral to js_strtod_harder. r?bbouvier
> 
> https://reviewboard.mozilla.org/r/55442/#review52178
> 
> Wow, good catches, thanks. For history, is it not crashing with jemalloc
> because jemalloc zeroes bits before handing them to the caller?

It doesn't. Could just be luck due to the difference in how memory is laid out.
https://hg.mozilla.org/mozilla-central/rev/5935b7839e2b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.