893684 - OdinMonkey: Assertion failure: (size_t) (dst - src) >= len, at jsutil.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

evaluate("\
    valueOf = (function() {\
        \"use asm\";\
        function f() {}\
        return f\
    });\
    this + ''\
", {
    fileName: '',
})
this + ''

asserts js debug shell on m-c changeset 3433a021847b without any CLI arguments intermittently (but fairly reliably) at Assertion failure: (size_t) (dst - src) >= len, at jsutil.h

Locking s-s because previously-found similar assertion bug 855442 was also s-s.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/b674f0e40c8e
user:        Brian Hackett
date:        Wed Jul 10 09:29:52 2013 -0600
summary:     Bug 885758 - Add ExclusiveContext for use by threads with exclusive access to their compartment, r=billm.

Brian, is bug 885758 a likely regressor?

Comment 1

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 3

[Luke Wagner [:luke]]

Nope, silly pre-existing Odin bug.

Comment 4

[Luke Wagner [:luke]]

The problem here is PostFailureLinkInfo stores a copy of the CompileOptions which stores a (dangling) pointer to filename (I should have caught this in review).  The patches in bug 880538 stop doing this and reconstruct the CompileOptions using the filename stored in ScriptSource.  That should be getting landed soon, so I'll wait until that and we can verify this problem is fixed.

Comment 5

[Luke Wagner [:luke]]

Attachment: fix

Actually the fix is super-trivial, no need to wait for the bigger patch.  I'd like to wait to land the testcase until the fix is in release, though, since this is pretty exploitable.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 776120 [details] [diff] [review]
fix

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

Love it when patches are simple like that.

Comment 8

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

Bug 880538 just landed on inbound - is this patch still needed?

Comment 9

[Luke Wagner [:luke]]

Heh, you beat me to commenting: no.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

What does that mean for 23/24?  will we want this as a smaller, lower risk fix on those branches?

Comment 11

[Luke Wagner [:luke]]

We could, if you think it is a good idea.  (It having not been discovered externally yet, I was thinking landing the smaller fix only drew attention.)

Comment 12

[Daniel Veditz [:dveditz]]

We should land the small fix.

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 776120 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 851421
User impact if declined: exploitable bug
Testing completed (on m-c, etc.): a bigger version of this patch is on m-c
Risk to taking this patch (and alternatives if risky): low

Comment 14

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

(Please land the patch only on Aurora and Beta, thanks!)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b3aa850f03fc
https://hg.mozilla.org/releases/mozilla-beta/rev/aa1e0d96de39