Crash [@ js::CurrentThreadCanAccessRuntime] or Crash on Heap with ES6 getter

VERIFIED FIXED in Firefox 43

Status

()

defect
--
critical
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: decoder, Assigned: jandem)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla43
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 wontfix, firefox43 verified)

Details

(Whiteboard: [jsbugmon:update][adv-main43-], crash signature)

Attachments

(1 attachment)

Reporter

Description

4 years ago
The following testcase crashes on mozilla-central revision 7f987c38bd3e (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2 --ion-eager --ion-offthread-compile=off):

var tokenCodes = {
    get finally() {
        if (tokenCodes[arr[i]] !== i) {}
    }
};
var arr = [
    'finally',
];
for (var i = 0; i < arr.length; i++) {
    if (tokenCodes[arr[i]] !== i) {}
}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x000000000070be34 in js::CurrentThreadCanAccessRuntime (rt=0x7ffff693c000) at js/src/vm/Runtime.cpp:822
#0  0x000000000070be34 in js::CurrentThreadCanAccessRuntime (rt=0x7ffff693c000) at js/src/vm/Runtime.cpp:822
#1  0x000000000070d6e4 in js::CurrentThreadCanAccessZone (zone=zone@entry=0x7ffff6954800) at js/src/vm/Runtime.cpp:829
#2  0x0000000000aa2a73 in zone (this=<optimized out>) at js/src/gc/Heap.h:1435
#3  js::jit::AssertValidObjectPtr (cx=0x7ffff6907000, obj=<optimized out>) at js/src/jit/VMFunctions.cpp:1141
#4  0x00007ffff7fd59b3 in ?? ()
[...]
#10 0x0000000000000000 in ?? ()
rax	0x7ffff7e5f000	140737352429568
rbx	0x7ffff6954800	140737330366464
rcx	0x7ffff7e00000	140737352040448
rdx	0x5f001	389121
rsi	0x1	1
rdi	0x7ffff693c000	140737330266112
rbp	0x7fffff7ff000	140737479962624
rsp	0x7fffff7ff000	140737479962624
r8	0x7ffff7e64f10	140737352453904
r9	0x0	0
r10	0x7fffffffa500	140737488332032
r11	0x7ffff693c1e8	140737330266600
r12	0x7ffff6907000	140737330049024
r13	0x7ffff693c000	140737330266112
r14	0x7ffff7efffe8	140737353089000
r15	0x7fffffffaa30	140737488333360
rip	0x70be34 <js::CurrentThreadCanAccessRuntime(JSRuntime*)+4>
=> 0x70be34 <js::CurrentThreadCanAccessRuntime(JSRuntime*)+4>:	push   %rbx
   0x70be35 <js::CurrentThreadCanAccessRuntime(JSRuntime*)+5>:	sub    $0x8,%rsp


Both this crash and the heap crash fail on a push instruction so I assume it might be that we ran out of stack space but I don't know for sure what's going on here so I'm marking it s-s until investigated.
Assignee

Comment 1

4 years ago
Posted patch PatchSplinter Review
Ion's GetElementIC can attach a scripted getter stub, so we need to call setPerformsCall() to ensure we don't elide the stack/overrecursion check from this function.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8658881 - Flags: review?(bhackett1024)
Attachment #8658881 - Flags: review?(bhackett1024) → review+
Group: core-security → javascript-core-security

Updated

4 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Comment 2

4 years ago
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/d96d552ff899
user:        Jan de Mooij
date:        Wed Feb 11 14:42:01 2015 +0100
summary:     Bug 1129382 - Add Ion ICs for scripted getters/setters. r=efaust,nbp,djvj

This iteration took 169.682 seconds to run.
https://hg.mozilla.org/mozilla-central/rev/123761e37f27
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43

Updated

4 years ago
Status: RESOLVED → VERIFIED

Comment 4

4 years ago
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
Shouldn't this have had sec-approval to go in?
Flags: needinfo?(jdemooij)
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main43+]
It sounds like it is just a missing stack overflow check, so I don't think it is a real security issue. (Potentially somebody could take advantage of this to overflow the stack at just the right point and cause problems.)
I'll just mark it sec-other for now. Feel free to adjust as desired.
Keywords: sec-other
Assignee

Comment 8

4 years ago
(In reply to Andrew McCreight [:mccr8] from comment #6)
> It sounds like it is just a missing stack overflow check, so I don't think
> it is a real security issue.

Right, I think we usually treat stack overflows as not s-s.
Flags: needinfo?(jdemooij)
Whiteboard: [jsbugmon:update][adv-main43+] → [jsbugmon:update][adv-main43-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.