635235 - Incorrect line number in e4x tokens leads to buffer overreads and possible crashes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: testcase

Attached testcase crashes both js debug and opt shell on TM changeset 0d4b01278890 without -m nor -j.

Nominating blocking2.0? and setting s-s because I have no idea what this is capable of. It seems to access a weird memory location at 0x1006627d6 in 64-bit.

This also reproduces in 32-bit.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stacks

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   62719:2d44fc834071
user:        Nicholas Nethercote
date:        Thu Feb 17 19:02:48 2011 -0800
summary:     Bug 634444 - Errors in long lines cause memory spikes when a console is in use.  r=brendan, a=blocking.

Comment 3

[Jeff Walden [:Waldo]]

Investigating.

Comment 4

[Jeff Walden [:Waldo]]

I don't crash on this.  On the other hand, dvander gets an error running it in valgrind.  Will investigate.

Comment 5

[Nicholas Nethercote [inactive]]

Could be related to bug 635144, which indicates that there are some latent problems with parsing of XML tokens that bug 634444 seems to have exposed.

I won't be able to take a look until Monday morning Melbourne time (2pm Sunday, MV time).  Backing out the bug 634444 patch is an option;  without the javascript.options.strict flag set the chance of having many error messages on very long lines while a console is attached is not that high.

Comment 6

[Nicholas Nethercote [inactive]]

Thinking some more:  let's just back out bug 634444.  This crash plus the strangeness in 635144 are worrying, and I don't think it's worth the effort and risk right now.  The memory spikes we get are at least well understood, they do go back down (assuming your machine hasn't ground to a halt) and this can be fixed in a 4.0.1 release.

Waldo, can you back out the two patches in bug 634444 today?  If not, I'll do it on Monday morning Melbourne time.

Comment 7

[Nicholas Nethercote [inactive]]

Bug 629858 also looks related to bug 634444.  I suspect there may be some latent problems with the lineno/index calculations in the parser, though I may have broken something myself.  Let's back bug 634444 out!

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #6)
> Thinking some more:  let's just back out bug 634444.  This crash plus the
> strangeness in 635144 are worrying, and I don't think it's worth the effort and
> risk right now.  The memory spikes we get are at least well understood, they do
> go back down (assuming your machine hasn't ground to a halt) and this can be
> fixed in a 4.0.1 release.

Besides bug 634444 and bug 635144, a fragile testcase in bug 629858 may be related. (A small patch in the latter seems to touch lines that the patch in bug 634444 does.)

Just to summarize the plethora of bugs:

Without the patch in bug 634444: implications vary from increasing memory usage in bug 634444, and / or unreliable crashes in bug 629858.

With the patch: problems in bug 635144 and this bug 635235.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: e4x_TCO++

I debugged this before seeing the latest comments, with which I concur completely.  I'll back out that change later tonight, an easy tree-watch since I have a m-c push to make.

Comment 10

[Nicholas Nethercote [inactive]]

Thanks, Waldo.  I wonder if your patch will fix bug 635144.

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 513667 [details] [diff] [review]
e4x_TCO++

Stealing the review.  I get two Valgrind warnings without the patch, and they disappear with the patch.  Thanks, Waldo!

Comment 12

[Nicholas Nethercote [inactive]]

(In reply to comment #8)
> 
> Without the patch in bug 634444: implications vary from increasing memory usage
> in bug 634444, and / or unreliable crashes in bug 629858.

Bug 629858 was present since at least 1.9.1, but the patch in bug 634444 exposed it.

> With the patch: problems in bug 635144 and this bug 635235.

Bug 635144 is likely a dup of this bug, I'll confirm that later.

Comment 13

[Nicholas Nethercote [inactive]]

Turns out this isn't a regression.  I've confirmed that this code:

  eval("try{}catch(e){} <x> </\nx>");

leads to buffer overruns (reported by Valgrind) in both 1.9.1 and 1.9.2.  So I've requested that this block 1.9.1 and 1.9.2.

I tracked this bug back, it exists in revision 1 in the hg repository, which means it's at least 4 years old.  I didn't track it back further than that, the thought was too depressing because (a) I'd have to deal with CVS, and (b) I'm guessing I'd learn that it's been present since the initial e4x work was done in late 2004.

Two thumbs up to Gary and Waldo for finding and fixing it!

Comment 14

[Nicholas Nethercote [inactive]]

Pushed for Waldo:

http://hg.mozilla.org/tracemonkey/rev/335dbb9502e0

Comment 15

[Brendan Eich [:brendan]]

Yeah, this is an old bug. We have older. In 2004, we didn't have fuzz-testing like we do now, or valgrind in its current state of the art, or more than 11 employees at Mozilla Foundation, or more than the early and uncertain hope of browser competition restarting due to Firefox growing fast since earlier that year, or a functioning Ecma TC39 TG1 (E4X got that body back together, but Microsoft's rep was manager of the C# compiler team, having fun doing paper designs and taking trips -- the IE6 team had been disbanded and only a skeleton crew remained), or a great many other things we now enjoy.

So, cheer up, and no retcons please.

Thanks to Gary and Waldo. Keep going, there are other such bugs hiding from the old days.

/be

Comment 16

[Brendan Eich [:brendan]]

Talking to njn, reviewing the details, it looks like this bug is not an sg:critical full remove euid exploit. It's a read-chars-past-end-of-userbuf bug. Privacy loss, not control flow integrity loss or memory overwrite.

/be

Comment 17

[Brendan Eich [:brendan]]

"full remote", of course.

/be

Comment 18

[Nicholas Nethercote [inactive]]

(In reply to comment #15)
> 
> So, cheer up, and no retcons please.

I thought I knew what that word meant -- "rewriting history", more or less.  Maybe I've got it's meaning wrong, though, because all I did was point out that this is an old bug.

Comment 19

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #13)
> (a) I'd have to deal with CVS

You could try the mozilla-cvs in git repo, it has bisection too :)

http://muizelaar.blogspot.com/2011/02/updated-mozilla-cvs-history-git-repo.html

Comment 20

[Jeff Walden [:Waldo]]

This might only be an overread.  In the code at issue here I *think* that's all it is.  (Even this soon after it's getting a little hazy in my mind.)  But I would not be surprised if other locations rely on this information being exactly correct (looks like the position comparison operators do, and they're hard to grep for unfortunately).  So I'd still counsel paranoia even if the symptom here looks "not entirely horrible".

Comment 21

[Brendan Eich [:brendan]]

Cut the FUD. TokenPos comparison operators are used for def-use fixups in transplanted comprehension expressions. Meanwhile, we have bug bounties going out to real sg:critical reporters. That coin should not be debased by exaggeration here.

/be

Comment 22

[Jeff Walden [:Waldo]]

Sorry.  I'd just thought it was standard practice to err on the side of caution when the extent of a problem was not easily grasped, as I do not (believe I) grasp the extent of the problem here.  It's the same approach I've taken lots of other times when asked to assess the security severity of a bug, and that I thought others have taken as well.

Comment 23

[LegNeato]

We'd like this bug on branches but due to comment 16 we won't block s security release on it. Please ask for approval on the patch if it is the same for branches.

Comment 24

[Daniel Veditz [:dveditz]]

lowering to sg:moderate for potentially revealing memory contents, although could be sg:high if you can read arbitrary amounts of memory and get reliable access to it from the script. Not blocking a branch release but we'd love to take this patch when it's ready and baked on trunk.

Comment 25

[Nicholas Nethercote [inactive]]

It's baked on trunk for a few days now.  It's a one line patch and very low risk, IMO.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #25)
> It's baked on trunk for a few days now.  It's a one line patch and very low
> risk, IMO.

You mean baked on tracemonkey?

Comment 27

[Nicholas Nethercote [inactive]]

(In reply to comment #26)
> 
> You mean baked on tracemonkey?

Oh, yes, sorry for any confusion.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Shouldn't it bake on trunk first? Also it's too late for requesting approval for 1.9.1.17 and 1.9.2.14. Those builds are already in beta stage.

Comment 29

[LegNeato]

Per conversation with drivers, only sg:moderate so we don't think it needs to block. Please renominate if you disagree.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 513667 [details] [diff] [review]
e4x_TCO++

Approved for 1.9.2.15 and 1.9.1.18, a=dveditz for release-drivers

Comment 31

[Brendan Eich [:brendan]]

This bug's patch is in tm, so it is gonna ship in Firefox 4 AFAIK, and it's as safe a fix as you could want. Just sayin'.

/be

Comment 32

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This already landed:

http://hg.mozilla.org/mozilla-central/rev/335dbb9502e0

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 513667 [details] [diff] [review]
e4x_TCO++

moving approvals to the next release.

Comment 34

[Daniel Veditz [:dveditz]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/90fd20f5056f