Closed Bug 1520783 Opened 1 year ago Closed 1 year ago

Assertion failure: map != ((void *) -1), at js/src/gc/Memory.cpp:850 with createMappedArrayBuffer

Categories

(Core :: JavaScript Engine, defect, P2, critical)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- unaffected
firefox66 --- fixed
firefox67 --- fixed

People

(Reporter: decoder, Assigned: ehoogeveen)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Attachments

(2 files)

The following testcase crashes on mozilla-central revision e56cc5e7b57a (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

evaluate(`
    createMappedArrayBuffer("\\x00");
`);

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0 js::gc::AllocateMappedContent (fd=<optimized out>, offset=0, length=4096, alignment=8) at js/src/gc/Memory.cpp:850
#1 0x0000555555853418 in CreateMappedArrayBuffer (cx=<optimized out>, cx@entry=0x7ffff5f19000, argc=<optimized out>, vp=<optimized out>) at js/src/shell/js.cpp:1540
#2 0x0000555555900e09 in CallJSNative (cx=0x7ffff5f19000, native=0x5555558531a0 <CreateMappedArrayBuffer(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/Interpreter.cpp:439
[...]
#26 0x00005555558367cc in Shell (envp=<optimized out>, op=0x7fffffffd9d0, cx=<optimized out>) at js/src/shell/js.cpp:10721
#27 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:11328
rax 0x555557bee280 93825032708736
rbx 0x555556b861d8 93825015505368
rcx 0x7ffff6c1c2dd 140737333281501
rdx 0x0 0
rsi 0x7ffff6eeb770 140737336227696
rdi 0x7ffff6eea540 140737336223040
rbp 0x7fffffffb6d0 140737488336592
rsp 0x7fffffffb5e0 140737488336352
r8 0x7ffff6eeb770 140737336227696
r9 0x7ffff7fe6c80 140737354034304
r10 0x58 88
r11 0x7ffff6b927a0 140737332717472
r12 0x0 0
r13 0x0 0
r14 0xffffffffffffffff -1
r15 0x1000 4096
rip 0x555556010c31 <js::gc::AllocateMappedContent(int, unsigned long, unsigned long, unsigned long)+465>
=> 0x555556010c31 <js::gc::AllocateMappedContent(int, unsigned long, unsigned long, unsigned long)+465>: movl $0x0,0x0
0x555556010c3c <js::gc::AllocateMappedContent(int, unsigned long, unsigned long, unsigned long)+476>: ud2

Shell-only issue, the function used here should probably be marked as fuzzing-unsafe, unless there is a point in testing it in some way (in that case, we need to check it's args better I guess).

Bug 1502733 changed this area recently. That may be implicated.

Priority: -- → P2

Emanuel, do you have time to look into this? If not we can find someone else.

Flags: needinfo?(emanuel.hoogeveen)

That's odd, I thought we marked this fuzzing-unsafe a long time ago - though it need not be. I'll have a look this weekend.

Flags: needinfo?(emanuel.hoogeveen)
Assignee: nobody → jcoppeard

Sorry for the delay on this, I looked at it briefly and I think something really weird is going on. We pass in \x00 and somehow get a valid file pointer out of that (stdin?), with valid fstat results and everything, then fail when we try to actually map it in the memory we allocated. I'm not sure whose responsibility it should be to detect this situation, if indeed we should, though I guess we could just return nullptr on failure.

Stealing. Clearly this can actually happen, so we should just return nullptr in this case.

decoder, could you confirm that this fixes the problem for the fuzzers? It now throws an error instead of crashing, which is the expected behavior.

Assignee: jcoppeard → emanuel.hoogeveen
Status: NEW → ASSIGNED
Flags: needinfo?(choller)
Attachment #9040526 - Flags: review?(sphink)

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #5)

decoder, could you confirm that this fixes the problem for the fuzzers? It now throws an error instead of crashing, which is the expected behavior.

Oh, I should mention: This applies cleanly on top of the patch in bug 1521713 (but should apply with some offset either way).

Attachment #9040526 - Flags: review?(sphink) → review+

Oh hey, I wrote a patch for this and never posted it. The problem is that we pass what amounts to an empty string and this is resolved to the current script's directory which we then attempt to map.

This adds a check that the path we have refers to a regular file. I don't think it would hurt to have both fixes.

Attachment #9040646 - Flags: review?(sphink)
Comment on attachment 9040646 [details] [diff] [review]
Part 2: Check whether the path passed to createMappedArrayBuffer() is a regular file.

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

Oh, nice. I was intending to look more closely at this after reviewing, but got distracted by something. Thanks for fixing.
Attachment #9040646 - Flags: review?(sphink) → review+
Blocks: 1502733

(In reply to Jon Coppeard (:jonco) from comment #8)

This adds a check that the path we have refers to a regular file. I don't think it would hurt to have both fixes.

Thanks! Agreed, it's nice to fix this at the source too.

Here's a try build with both fixes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=21a2fc739c1c39245b5f70edb2258cff21a2c40c

Attachment #9040526 - Attachment description: Don't assert if we fail to map a file after reserving the memory. → Part 1: Don't assert if we fail to map a file after reserving the memory.
Attachment #9040646 - Attachment description: bug1520783-mapped-array-buffer → Part 2: Check whether the path passed to createMappedArrayBuffer() is a regular file.

Looks good.

Keywords: checkin-needed

Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7c2e6ed8a5f
Don't assert if we fail to map a file after reserving the memory. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/29cc6f5aef92
Check whether the path passed to createMappedArrayBuffer() is a regular file. r=sfink

Keywords: checkin-needed
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/32aab5bf983a
parent:      453862:cd696bc79dff
user:        Emanuel Hoogeveen
date:        Sat Dec 15 14:26:00 2018 +0200
summary:     Bug 1502733 - Part 1: Clean up and refactor GC system memory allocation functions. r=sfink

This iteration took 520.852 seconds to run.
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Is there a user impact here which justifies backport consideration or can this ride the trains?

Flags: needinfo?(emanuel.hoogeveen)
Flags: in-testsuite+

I'm fairly sure that mapped array buffers are only available to privileged code and embedders, and this is an extreme edge case that normal code should never hit. On the other hand this should also be a very safe uplift and might reduce noise for fuzzers going forward, so I think I'll request uplift anyway.

Flags: needinfo?(emanuel.hoogeveen)

Comment on attachment 9040526 [details] [diff] [review]
Part 1: Don't assert if we fail to map a file after reserving the memory.

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1502733

User impact if declined

No impact. This patch fixes an edge case that our code should never hit. However, it might reduce the noise for our fuzzers (by turning a safe crash into a known error).

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

This patch makes a fallible function return null in an obscure case that used to crash.

String changes made/needed

Attachment #9040526 - Flags: approval-mozilla-beta?

Comment on attachment 9040646 [details] [diff] [review]
Part 2: Check whether the path passed to createMappedArrayBuffer() is a regular file.

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1502733

User impact if declined

None. This patch simply provides a more helpful error message on the failure path. It also adds a regression test for the failure in this bug.

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

This is a simple change to an obscure function only accessible via the JS shell.

String changes made/needed

Attachment #9040646 - Flags: approval-mozilla-beta?
Comment on attachment 9040526 [details] [diff] [review]
Part 1: Don't assert if we fail to map a file after reserving the memory.

Improving results for fuzzing sounds good.
Let's take this for beta 6.
Attachment #9040526 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 9040646 [details] [diff] [review]
Part 2: Check whether the path passed to createMappedArrayBuffer() is a regular file.

Looks like we missed one here.
Attachment #9040646 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(choller)
You need to log in before you can comment on or make changes to this bug.