216320 - [ABW]js_FinishTakingSrcNotes is exceeding the notes array

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[David Bradley]

The SN_MAKE_TERMINATOR(&notes[totalCount]); line is exceeding the note's array
passed in. Probably may some more adjusting like what was done in bug 215878?

Comment 1

[Phil Schwartau]

Reassigning -

Comment 2

[Brendan Eich [:brendan]]

dbradley: is this with the fixes for bug 215878 ?  Can you give a stack trace,
or better yet the script on which this happened?

/be

Comment 3

[David Bradley]

Attachment: Stack of array bounds and allocation

I was just running the browser under Purify checking for something else,
unfortunately I don't remember the specific thing I was doing. I was running
the browser under Purify trying to diagnose another crash at the time. I'll
look back and see if I figure out what I was doing. I meant to go back anyway
and see if I could figure out what was going on, but got distracted.

Yes, this occured with the patch(es) in bug 215878.

Comment 4

[David Bradley]

I've been unable to reproduce the problem since I first reported it.
Unfortunately I didn't record what I was doing at the time. I'll keep an eye out
and reopen if I come across it.

Comment 5

[Phil Schwartau]

Marking Verified for now -

Comment 6

[Brendan Eich [:brendan]]

bryner found a reproducible case, I think it's essentially this bug, modulo heap
sensitivity.  This needs to get fixed for 1.5 final.

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: testcase, can run in js shell to show bug

Hope this is ok to attach, I didn't see anything confidential in it.

/be

Comment 8

[Brendan Eich [:brendan]]

Patch in a few.

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: proposed fix (apply this)

I should write "KEEP CG_COUNT_FINAL_TRYNOTES IN SYNC WITH
js_FinishTakingSrcNotes" 100 times on a blackboard.

This fixes the bug, valgrind testifies that we're pure.

diff -w version in a second for review.

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: diff -w of last patch (review this)

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 130794 [details] [diff] [review]
diff -w of last patch (review this)

I hope shaver's around so this can get r= fast, and go in for 1.5.

/be

Comment 12

[Brendan Eich [:brendan]]

Attachment: HTML testcase, click here to crash

Sorry for the mecha.mozilla.org link, it's easy to fix if it becomes
impermanent.

/be

Comment 13

[Mike Shaver (:shaver emeritus)]

Comment on attachment 130794 [details] [diff] [review]
diff -w of last patch (review this)

Looks good. sr=test-suite, and away? =)

Comment 14

[Brendan Eich [:brendan]]

I'm sure Phil will make a regression test -- he always does ;-).  Thanks, shaver.

Now for 1.5final approval.  This was a regression in 1.5beta.

/be

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 130794 [details] [diff] [review]
diff -w of last patch (review this)

a=asa (on behalf of drivers) for checkin to Mozilla 1.5

Comment 16

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 17

[David Bradley]

>I should write "KEEP CG_COUNT_FINAL_TRYNOTES IN SYNC WITH
>js_FinishTakingSrcNotes" 100 times on a blackboard.

I'll be there with you, writing "Always post a test case"

Comment 18

[Phil Schwartau]

Testcase added to JS testsuite:

      mozilla/js/tests/js1_5/Regress/regress-216320.js

Comment 19

[Phil Schwartau]

Marking Verified FIXED.

The above testcase does not crash for me in either the debug
or optimized JS shell, on either WinNT or Linux -