"Assertion failure: isInt32(*p), at ../jstracer.cpp:2587" with nested trees

VERIFIED FIXED

Status

()

defect
P1
normal
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: jorendorff, Assigned: dvander)

Tracking

({testcase, verified1.9.1, verified1.9.2})

Other Branch
Points:
---
Bug Flags:
blocking1.9.2 +
wanted1.9.0.x -
in-testsuite -

Firefox Tracking Flags

(status1.9.2 beta1-fixed, blocking1.9.1 .6+, status1.9.1 .6-fixed)

Details

(Whiteboard: [sg:critical?])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
var arr = [
    [0, 0, 0, 0, 0], // inner tree compiles first path
    [0, 0, 0, 0, 0],
    [0, 0, 0, 0, 0], // outer tree compiles
    [0, 0, 0, 0, 0],
    [0, 0, 0, 0, 0],
    [1, 0, 0, 0, 0], // inner tree compiles different path, assinging x = ""
                     // outer tree extends (different inner tree exit)
    [1, 0, 0, 0, 0],
    [1, 0, 0, 0, 0],
    [1, 0, 0, 0, 0], // Assertion failure: isInt32(*p), at ../jstracer.cpp:2587
    [1, 0, 0, 0, 0],
    ];

var x = 0;

for (var i = 0; i < 10; i++) {
    var b = arr[i];
    for (var j = 0; j < 5; j++) {
        if (b[0])
            x = "";
    }
    x = 0;
}

Updated

10 years ago
Group: core-security

Comment 1

10 years ago
Paging david. Not again.

Comment 2

10 years ago
which branch does this?
(Reporter)

Comment 3

10 years ago
tracemonkey tip.
(Reporter)

Comment 4

10 years ago
Posted patch testsSplinter Review
Here are some tests to include in the fix.

One is a test for bug 502714 (already fixed). The other is a reduced version of the test in comment 0:

var x = 0;
for (var i = 0; i < 20; i++) {
    for (var j = 0; j < 5; j++) {
        if (i > 5)
            x = "";
    }
    x = 0;
}
Posted patch proposed fix (obsolete) — Splinter Review
bug 502604 and bug 502714 addressed the underlying problem here. Unfortunately AttemptToExtendTree was still assuming that the outer global typemap would always have more globals than the inner typemap. This is not true. LeaveTree() and the tree call mechanism already knew this and computed the global typemap length correctly.
Assignee: general → dvander
Status: NEW → ASSIGNED
Attachment #395445 - Flags: review?(gal)
This is rare, but it should probably block because we can tag pointers incorrectly.
blocking1.9.1: --- → ?
Flags: blocking1.9.2?

Updated

10 years ago
Attachment #395445 - Flags: review?(gal) → review+

Updated

10 years ago
Attachment #395445 - Flags: approval1.9.1.3?
Posted patch refactored (obsolete) — Splinter Review
sayrer noted that this logic should probably be localized to one function.
Attachment #395445 - Attachment is obsolete: true
Attachment #395445 - Flags: approval1.9.1.3?
Using Queue in this part of LeaveTree does not seem to be a performance loss.
Attachment #395469 - Attachment is obsolete: true
Attachment #395482 - Flags: review?(gal)

Comment 9

10 years ago
I would prefer the minimal fix for 1.9.1.

Updated

10 years ago
Attachment #395482 - Flags: review?(gal) → review+

Comment 10

10 years ago
Comment on attachment 395482 [details] [diff] [review]
use typemap everywhere.  simpler

> globalTypeMap = typeMap.data();

that is still not very palatable, but it is most likely not a security bug.
Attachment #395482 - Flags: review?(sayrer) → review+

Updated

10 years ago
Flags: blocking1.9.2? → blocking1.9.2+
It's safe - though if we're worried about it breaking if code moves around in the future, I can add some followup comments/assertions.
Blocking 1.9.1.4 for now, but gated on when the fix lands for 1.9.2 so might be the next one.

> I would prefer the minimal fix for 1.9.1.

Generally I'd agree, but in the case of complicated code there's an argument for being consistent across branches too. What's better in this case?
blocking1.9.1: ? → .4+
Keywords: testcase
Whiteboard: [sg:critical?]

Comment 14

10 years ago
The difference is only code reshuffling/cleanup. No functional difference. If you are ok with the extra risk of the slight code movement, I don't mind either patch on branch. They both do the same thing.
Propagate the cleanup. It will help not only here but future merges.

/be

Comment 16

10 years ago
http://hg.mozilla.org/mozilla-central/rev/3a1ab9f19b66
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Not yet landed for 1.9.2, this misses the 1.9.1.4 release
blocking1.9.1: .4+ → .5+

Comment 18

10 years ago
(In reply to comment #17)
> Not yet landed for 1.9.2, this misses the 1.9.1.4 release

when did landing on an unreleased branch become a requirement?

Updated

10 years ago
Priority: -- → P1
Does this patch apply to 1.9.1? If not, can you please attach one that does? Code freeze for 1.9.1.6 is set for November 10 at 11:59pm PDT and this blocks that release.

Please request approval on a 1.9.1-applicable patch.
This does not apply. I will get a 1.9.1-applicable patch here tomorrow.
Posted patch branch patchSplinter Review
Attachment #410572 - Flags: review?(sayrer)
Attachment #410572 - Flags: approval1.9.1.6?
Comment on attachment 410572 [details] [diff] [review]
branch patch

This should be identical to the other patch with a few exceptions:
 * Some inline helpers are missing so the code is a little more verbose.
 * JSTraceType is uint8
 * Queue needed the max==0 support.
Attachment #410572 - Flags: review?(sayrer) → review?(gal)

Comment 24

10 years ago
Comment on attachment 410572 [details] [diff] [review]
branch patch

Please set max to 0 in the default constructor to avoid malloc-ing in hot paths when we don't actually use the typemap.
Attachment #410572 - Flags: review?(gal) → review+
(In reply to comment #24)

For posterity, IRL conclusion was to not bother doing this since it's a branch patch.
Comment on attachment 410572 [details] [diff] [review]
branch patch

Approved for 1.9.1.6, a=dveditz for release-drivers
Attachment #410572 - Flags: approval1.9.1.6? → approval1.9.1.6+
Bob, can you verify this for 1.9.1?

Comment 29

10 years ago
Nope. It's not fixed on mac at least.

$ Darwin_DBG.OBJ/js -j
js> var arr = [
    [0, 0, 0, 0, 0], // inner tree compiles first path
    [0, 0, 0, 0, 0],
    [0, 0, 0, 0, 0], // outer tree compiles
    [0, 0, 0, 0, 0],
    [0, 0, 0, 0, 0],
    [1, 0, 0, 0, 0], // inner tree compiles different path, assinging x = ""
                     // outer tree extends (different inner tree exit)
    [1, 0, 0, 0, 0],
    [1, 0, 0, 0, 0],
    [1, 0, 0, 0, 0], // Assertion failure: isInt32(*p), at ../jstracer.cpp:2587
    [1, 0, 0, 0, 0],
    ];

var x = 0;

for (var i = 0; i < 10; i++) {
    var b = arr[i];
    for (var j = 0; j < 5; j++) {
        if (b[0])
            x = "";
    }
    x = 0;
}js> js> js> js> 
Assertion failure: isInt32(*p), at ../jstracer.cpp:2121
Trace/BPT trap
Since this is blocking on 1.9.1, we need someone to look at this during the next day or two.
Bob, you might want to check on 1.9.2 as well since it is blocking there.
(In reply to comment #29)

Are you sure? I checked out a clean 1.9.1 tree and couldn't reproduce the problem.

Comment 33

10 years ago
did you run the shell with -j ?

1.9.2/1.9.3 look clean.
Yup, I tried both saving as a file and copy+paste into interactive shell.

Comment 35

10 years ago
verified 1.9.1,1.9.2,1.9.3 shell on mac os x. not sure what was up with my previous brain fart, but after an rm and re-build it works fine.
Status: RESOLVED → VERIFIED
Group: core-security
Flags: wanted1.9.0.x-
Filter on qa-project-auto-change:

Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.