Bugzilla

(Reporter)

Updated

•

4 years ago

Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]

(Reporter)

Comment 2

•

4 years ago

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

Andrew McCreight [:mccr8]

(Reporter)

Updated

•

4 years ago

Whiteboard: [jsbugmon:bisect] → [jsbugmon:]

Comment 3

•

4 years ago

Jeff, can you look into this? Or should it be closed as incomplete?

Flags: needinfo?(jwalden+bmo)

Comment 4

•

4 years ago

I think someone fixed this since the bug's filing, because I can't reproduce on trunk but can on the noted revision.  Gonna try and figure out what fixed this now...

Flags: needinfo?(jwalden+bmo)

Comment 5

•

4 years ago

Actually, no, it's still there.  Or at least valgrind complains the same way even on trunk.  More investigating to do.

Flags: needinfo?(jwalden+bmo)

Comment 6

•

4 years ago

Somehow the lazyScript's begin/end indexes are way off.  In a somewhat-altered testcase:

(gdb) lis
1407	            return false;
1408	
1409	        const char16_t* lazyStart = chars + lazy->begin();
1410	        size_t lazyLength = lazy->end() - lazy->begin();
1411	
1412	        if (!frontend::CompileLazyFunction(cx, lazy, lazyStart, lazyLength)) {
1413	            // The frontend may have linked the function and the non-lazy
1414	            // script together during bytecode compilation. Reset it now on
1415	            // error.
1416	            fun->initLazyScript(lazy);
(gdb) p chars
$22 = 0x638d8d0 u"var obj = { x: 2 }; function foobar() { [1, 2, '\\u00ee', 4, 5]; }; foobar();"
(gdb) p lazy->begin()
$23 = 152
(gdb) p lazy->end()
$24 = 182

Utterly bonkers.  Still tracking down why the LazyScript is whack.

Comment 7

•

4 years ago

A somewhat more illuminating testcase:

  var ce = cacheEntry("function foobar() { } foobar();");
  var opts = { columnNumber: 167 };
  evaluate(ce, opts);

That 167 gets slotted into a ReadOnlyCompileOptions::column by the shell.  That, then, eventually makes its way into:

TokenStream::TokenStream(ExclusiveContext* cx, const ReadOnlyCompileOptions& options,
                         const char16_t* base, size_t length, StrictModeGetter* smg)
  : srcCoords(cx, options.lineno),
    ...
    userbuf(cx, base, length, options.column),

where we have

    // buf[0..length-1] often represents a substring of some larger source,
    // where we have only the substring in memory. The |startOffset| argument
    // indicates the offset within this larger string at which our string
    // begins, the offset of |buf[0]|.
    class TokenBuf {
      public:
        TokenBuf(ExclusiveContext* cx, const char16_t* buf, size_t length, size_t startOffset)

Wut.  We're passing a *column number* in as a *start offset* within a buffer.  That triggers all subsequent out-of-boundsing as we blindly use the "start offset".

It appears from DXRing last night that setColumn is only called by the evaluate() code, and by code that copies over a column from a LazyScript.  I'm not quite certain where the LazyScript gets its column, but tentative DXR suggests it *might* be directly from the parser, which I think would be safe.  But the maze of twisty passages here is bad enough I don't say that with much confidence right now.

Comment 8

•

4 years ago

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> It appears from DXRing last night that setColumn is only called by the
> evaluate() code, and by code that copies over a column from a LazyScript. 
> I'm not quite certain where the LazyScript gets its column, but tentative
> DXR suggests it *might* be directly from the parser, which I think would be
> safe.

Yes, that's the idea. I added columnNumber support to js.cpp's Evaluate in bug 1083913 so that we could test exactly this case.

But startOffset is only meant to permit non-zero-based indexing of the TokenBuf. It shouldn't be able to trigger out-of-bounds fetches; the assertions in TokenBuf::rawCharPtrAt are supposed to catch that.

Comment 9

•

4 years ago

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> Wut.  We're passing a *column number* in as a *start offset* within a
> buffer.  That triggers all subsequent out-of-boundsing as we blindly use the
> "start offset".

This doesn't sound quite right. startOffset is the offset we attribute to buf[0]; the accessible portion of the buffer is always buf[0 .. length-1], regardless of startOffset.

The only reason we need startOffset at all is that it provides a way to bias the column numbers for the first line of text only; see the definition of TokenStream::getColumn here:

https://dxr.mozilla.org/mozilla-central/source/js/src/frontend/TokenStream.h#269

Once we reach the second line, linebase gets updated, and column numbers pick up at zero.

If you wanted to make the logic there more direct, that would be fantastic. I don't like it; but I feel like I improved it over what went before. If you could take it to the finish line, that would be wonderful.

Comment 10

•

4 years ago

I think Parser<SyntaxParseHandler>::finishFunctionDefinition is not passing the right value to LazyScript::CreateRaw; funbox->bufStart seems to be wrong.

Comment 11

•

4 years ago

There's a confusion about what 'LazyScript::begin_' is supposed to mean. It seems that FunctionBox::setStart uses the token's pos to set funbox->bufStart; that value includes the initial column bias. But that's then used to initialize LazyScript::begin_, which is supposed to be a raw offset into the SourceObject's text.

BMO Automation

Updated

•

4 years ago

Group: core-security

Comment 12

•

4 years ago

Yeah.  Subsequent investigation determined that LazyScript::begin_ indeed had two meanings.  I don't see any way around this but to add another member, with the second desired meaning.  (And thus bloat things a bit everywhere.)

My plan for dealing with this was to pass around something SourceBufHolder-like, that encapsulated text and both forms of offsets.  Getting the different offset meanings correct was turning into enough of a bear, tho, that I ended up attacking other things and haven't circled back.  (It still vaguely appears to me that this particular mis-meant code path is only reachable from the shell, in which case delay isn't going to harm anything.)  Soon, hopefully, to get this out of the queue.

Updated

•

4 years ago

Duplicate of this bug: 1207409

BMO Automation

Updated

•

4 years ago

Crash Signature: [@ js::frontend::TokenStream::TokenBuf::getRawChar()] → [@ js::frontend::TokenStream::TokenBuf::getRawChar()] [@ js::frontend::TokenStream::TokenBuf::getRawChar]

Comment 14

•

3 years ago

Assigning to :Waldo based on comment 12.

Assignee: nobody → jwalden+bmo

Status: NEW → ASSIGNED

Andrew McCreight [:mccr8]

Updated

•

3 years ago

Updated

•

3 years ago

Duplicate of this bug: 1232682

Comment 16

•

3 years ago

Jeff, have you had a chance to look at this? Is there anybody else who could look at it? Thanks.

Tooru Fujisawa [:arai]

Comment 17

•

3 years ago

I don't know much about LazyScript, so I won't be able to work on this shortly, may take some time to learn about it.
(if other one, maybe Waldo, can work on this, it would be faster)

if there's some hint in bug 1209501 (or dupe bugs), can you cc me?

Jason Orendorff [:jorendorff]

Comment 18

•

3 years ago

Putting this on my list, if only to find another owner for it...

Waldo thinks it's shell-only.

Assignee: jwalden+bmo → jorendorff

Flags: needinfo?(jwalden+bmo)

Andrew McCreight [:mccr8]

Updated

•

3 years ago

Duplicate of this bug: 1261817

Comment 20

•

3 years ago

If this is shell-only, I'll mark this sec-audit. Please change it back if that's not right.

Keywords: sec-high → sec-audit

Updated

•

3 years ago

Duplicate of this bug: 1308575

Comment 22

•

3 years ago

Debug builds now fail an assert I added in bug 1304390, making this fail more reliably.

Comment 23

•

3 years ago

:Waldo, what's next here?

Flags: needinfo?(jwalden+bmo)

Updated

•

3 years ago

status-firefox40: affected → ---

status-firefox50: --- → wontfix

status-firefox51: --- → fix-optional

status-firefox52: --- → fix-optional

status-firefox53: --- → fix-optional

(Reporter)

Comment 24

•

3 years ago

Can we finally get this fixed? I keep hitting this bug in all sorts of ways and these crashes always look s-s at first until (after some time) I figure out it's this bug.

Flags: needinfo?(jdemooij)

Comment 25

•

3 years ago

Shu, maybe you can take a look at this as it's frontend stuff? If this line/column API is not actually used in the browser, we should maybe just remove it.

Flags: needinfo?(jdemooij) → needinfo?(shu)

Comment 26

•

3 years ago

The line/column stuff is used in the browser -- it just happens that the coordinates passed in represent reality (because scripts are embedded in HTML pages, or are standalone), not an arbitrary number like you can pass in to evaluate().  This could be *masked* by #if 0'ing out this part of ParseCompileOptions:

    if (!JS_GetProperty(cx, opts, "columnNumber", &v))
        return false;
    if (!v.isUndefined()) {
        int32_t c;
        if (!ToInt32(cx, v, &c))
            return false;
        options.setColumn(c);
    }

Doing this would kill off the only way to load a script at a non-normal column offset, tho -- which is certainly don on the web by inline scripts that start at offsets from the first column -- so I'm not convinced it's a great idea.

(Assignee)

Comment 27

•

3 years ago

IIUC, the bug is only in the shell's evaluate(), because we'd like to be able to pass in an arbitrary column number that is OOB of the provided string, in order to test scripts that have a non-1 column. For non-1 column scripts embedded in HTML, this problem never arises, because those column numbers aren't bogus and will always be in-bounds of the surrounding source buffer.

In other words, the only use case for the extra work of maintaining a separate column and buffer start offset is for internal testing. I don't think that's compelling enough to do the extra work.

Jim, do we need the ability to provide OOB column numbers for evaluate() in the shell? Other than annoyance of writing tests, why not require that the columns are in bounds in the provided buffer? evaluate(), when given a "columnNumber: N" property, would start parsing at the Nth character in the provided string.

Flags: needinfo?(shu) → needinfo?(jimb)

Comment 28

•

2 years ago

(In reply to Shu-yu Guo [:shu] from comment #27)
> IIUC, the bug is only in the shell's evaluate(), because we'd like to be
> able to pass in an arbitrary column number that is OOB of the provided
> string, in order to test scripts that have a non-1 column. For non-1 column
> scripts embedded in HTML, this problem never arises, because those column
> numbers aren't bogus and will always be in-bounds of the surrounding source
> buffer.
> 
> In other words, the only use case for the extra work of maintaining a
> separate column and buffer start offset is for internal testing. I don't
> think that's compelling enough to do the extra work.
> 
> Jim, do we need the ability to provide OOB column numbers for evaluate() in
> the shell? Other than annoyance of writing tests, why not require that the
> columns are in bounds in the provided buffer? evaluate(), when given a
> "columnNumber: N" property, would start parsing at the Nth character in the
> provided string.

You're proposing that we just make the string large, and change the meaning of columnNumber from specifying the column of the strings's first character to specifying the point within the string that parsing begins?

If I recall correctly, the original motivation for adding columnNumber at all was that SpiderMonkey's internal processing handled overflow badly. So the tests like this one are actually pretty important:

js/src/jit-test/tests/parser/columnNumber.js

// Check handling of columns near the limit of our ability to represent them.
// (This is hardly thorough, but since web content can't set column numbers,
// it's probably not worth it to be thorough.)
const maxColumn = Math.pow(2, 30) - 1;
assertEq(evaluate("saveStack().column", { columnNumber: maxColumn }),
         maxColumn + 1);

I don't think we can do that feasibly with your proposed approach.

Have I misunderstood?

Flags: needinfo?(jimb)

Comment 29

•

2 years ago

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
>     // buf[0..length-1] often represents a substring of some larger source,
>     // where we have only the substring in memory. The |startOffset| argument
>     // indicates the offset within this larger string at which our string
>     // begins, the offset of |buf[0]|.
>     class TokenBuf {
>       public:
>         TokenBuf(ExclusiveContext* cx, const char16_t* buf, size_t length,
> size_t startOffset)
> 
> Wut.  We're passing a *column number* in as a *start offset* within a
> buffer.  That triggers all subsequent out-of-boundsing as we blindly use the
> "start offset".

Caveat: it's been a long time since I looked at this, but from memory:

I think you're misunderstanding the comment. It sounds like the code does as well.

If I have a substring S of length 100 that I extracted from some larger document D, and the first character of S lands at column C of some line in D, then the TokenBuf constructor's `startOffset` argument is C, and its `buf` argument should be S. 

It's completely incorrect to treat `startOffset` as an index into `buf`; `startOffset` partially describes the position within D of `buf[0]`.

(Assignee)

Comment 30

•

2 years ago

(In reply to Jim Blandy :jimb from comment #29)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> >     // buf[0..length-1] often represents a substring of some larger source,
> >     // where we have only the substring in memory. The |startOffset| argument
> >     // indicates the offset within this larger string at which our string
> >     // begins, the offset of |buf[0]|.
> >     class TokenBuf {
> >       public:
> >         TokenBuf(ExclusiveContext* cx, const char16_t* buf, size_t length,
> > size_t startOffset)
> > 
> > Wut.  We're passing a *column number* in as a *start offset* within a
> > buffer.  That triggers all subsequent out-of-boundsing as we blindly use the
> > "start offset".
> 
> Caveat: it's been a long time since I looked at this, but from memory:
> 
> I think you're misunderstanding the comment. It sounds like the code does as
> well.
> 
> If I have a substring S of length 100 that I extracted from some larger
> document D, and the first character of S lands at column C of some line in
> D, then the TokenBuf constructor's `startOffset` argument is C, and its
> `buf` argument should be S. 
> 
> It's completely incorrect to treat `startOffset` as an index into `buf`;
> `startOffset` partially describes the position within D of `buf[0]`.

But the Gecko embedding does treat startOffset as an index into buf, no? Are you saying that's also incorrect or it was never intended to work?

Comment 31

•

2 years ago

I actually can't find any uses of CompileOptions::setColumn in gecko. I don't understand how Firefox establishes the starting column number for HTML attribute event handlers, for example.

Comment 32

•

2 years ago

Do we have enough information now to make a decision and get this fixed?

Flags: needinfo?(shu)

Flags: needinfo?(jimb)

Comment 33

•

2 years ago

We do not. It's not clear exactly which code is mis-using the API, or how.

Flags: needinfo?(jimb)

Comment 34

•

2 years ago

Actually, I'm no longer sure about this. It may be that the shell provides a way to pass parameters that ordinary code could never pass, because they're carefully constructed to be correct. If that's true, then this is not relevant to content.

Properly settling this entails pinning down the meanings of the column and startOffset and all those things. I have no idea what they mean any more.

I'm unfortunately not able to put time into this right now. The last time I did, in bug 1083913, I tried to improve the situation, at least.

(Reporter)

Comment 35

•

2 years ago

This is an automated crash issue comment:

Summary: Assertion failure: begin + len <= length(), at js/src/jsscript.cpp:1691
Build version: mozilla-central revision e17cbb839dd2
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize
Runtime options: --fuzzing-safe min.js

Testcase:

assertEq(evaluate(`new Promise(res=>res)`, {
    columnNumber: 1729
}), 1730);

Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x0000000000a05d68 in js::ScriptSource::chars (this=this@entry=0x7ffff4343340, cx=0x7ffff6954000, holder=..., begin=1741, len=8) at js/src/jsscript.cpp:1691
#0  0x0000000000a05d68 in js::ScriptSource::chars (this=this@entry=0x7ffff4343340, cx=0x7ffff6954000, holder=..., begin=1741, len=8) at js/src/jsscript.cpp:1691
#1  0x0000000000a05f62 in js::ScriptSource::PinnedChars::PinnedChars (this=0x7fffffffc110, cx=<optimized out>, source=0x7ffff4343340, holder=..., begin=<optimized out>, len=<optimized out>) at js/src/jsscript.cpp:1653
#2  0x0000000000998763 in JSFunction::createScriptForLazilyInterpretedFunction (cx=0x7ffff6954000, fun=fun@entry=...) at js/src/jsfun.cpp:1497
#3  0x0000000000467918 in JSFunction::getOrCreateScript (cx=<optimized out>, fun=...) at js/src/jsfun.h:428
#4  0x0000000000998320 in js::LazyScript::functionDelazifying (script=..., cx=0x7ffff6954000) at js/src/jsscriptinlines.h:81
#5  JSFunction::createScriptForLazilyInterpretedFunction (cx=0x7ffff6954000, fun=fun@entry=...) at js/src/jsfun.cpp:1453
#6  0x0000000000467918 in JSFunction::getOrCreateScript (cx=<optimized out>, fun=...) at js/src/jsfun.h:428
#7  0x000000000053b8d7 in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff6954000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:473
#8  0x000000000053bc38 in InternalCall (cx=cx@entry=0x7ffff6954000, args=...) at js/src/vm/Interpreter.cpp:515
#9  0x000000000053bd6d in js::Call (cx=cx@entry=0x7ffff6954000, fval=..., fval@entry=..., thisv=..., args=..., rval=..., rval@entry=...) at js/src/vm/Interpreter.cpp:534
#10 0x00000000005db22a in js::PromiseObject::create (cx=cx@entry=0x7ffff6954000, executor=..., executor@entry=..., proto=..., proto@entry=..., needsWrapping=<optimized out>) at js/src/builtin/Promise.cpp:1415
#11 0x00000000005db781 in PromiseConstructor (cx=0x7ffff6954000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/Promise.cpp:1345
#12 0x0000000000546b1f in js::CallJSNative (cx=cx@entry=0x7ffff6954000, native=native@entry=0x5db520 <PromiseConstructor(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:291
[...]
#35 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:8684

(Reporter)

Comment 36

•

2 years ago

This bug keeps producing new signatures, all of which look s-s in triage. Based on that, I'm marking this as fuzzblocker. Please fix this to avoid triggering more false positives in our triage.

Whiteboard: [jsbugmon:] → [jsbugmon:][fuzzblocker]

(Assignee)

Comment 37

•

2 years ago

Posted patch De-offset ScriptSource offsets from starting column in JSScript and LazyScript. — Details — Splinter Review

The JS shell, for testing purposes, allows passing in an OOB column
number, which offsets all token positions by column. These positions
need to be de-offset before being saved as offsets into the
ScriptSource buffer in the script structures.

Attachment #8861704 - Flags: review?(jimb)

(Assignee)

Updated

•

2 years ago

Flags: needinfo?(shu)

(Assignee)

Updated

•

2 years ago

Duplicate of this bug: 1209501

Comment 39

•

2 years ago

Comment on attachment 8861704 [details] [diff] [review]
De-offset ScriptSource offsets from starting column in JSScript and LazyScript.

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

This looks like a really nice solution.

Attachment #8861704 - Flags: review?(jimb) → review+

Updated

•

2 years ago

Assignee: jorendorff → shu

Comment 40

•

2 years ago

Shu-yu, is this ready for landing?

Flags: needinfo?(jwalden+bmo) → needinfo?(shu)

(Assignee)

Comment 41

•

2 years ago

I just landed it. We should also open this bug, because it is shell-only.

Flags: needinfo?(shu)

Comment 42

•

2 years ago

Yep, apparently you just did:

https://hg.mozilla.org/integration/mozilla-inbound/rev/041899558dd5

Opening up as per comment 41.

Group: javascript-core-security

Iris Hsiao [:ihsiao]

Comment 43

•

2 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/041899558dd5

Status: ASSIGNED → RESOLVED

Last Resolved: 2 years ago

status-firefox55: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla55

Comment 44

•

2 years ago

Gary, are there any branches that would benefit from a fuzzing standpoint by backporting this fix to them?

status-firefox51: fix-optional → wontfix

status-firefox52: fix-optional → wontfix

status-firefox53: fix-optional → wontfix

status-firefox54: --- → affected

status-firefox-esr52: --- → affected

Flags: needinfo?(gary)

Flags: in-testsuite+

Comment 45

•

2 years ago

Since Aurora is dying, probably Beta would be good. It isn't strictly needed for ESR52, though it would be nice since it's early in the ESR cycle.

Flags: needinfo?(gary) → needinfo?(ryanvm)

Comment 46

•

2 years ago

Shu, can you please nominate this for Beta/ESR52 to help reduce fuzzing noise?

Flags: needinfo?(ryanvm) → needinfo?(shu)

(Assignee)

Comment 47

•

2 years ago

Comment on attachment 8861704 [details] [diff] [review]
De-offset ScriptSource offsets from starting column in JSScript and LazyScript.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: None, requesting uplift for convenience to fuzzing infra
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: No clue
[User impact if declined]: None, requesting uplift for convenience of fuzzing infra
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: See "user impact"
[String changes made/needed]: None

Flags: needinfo?(shu)

Attachment #8861704 - Flags: approval-mozilla-esr52?

Attachment #8861704 - Flags: approval-mozilla-beta?

Gerry Chang [:gchang]

Comment 48

•

2 years ago

Comment on attachment 8861704 [details] [diff] [review]
De-offset ScriptSource offsets from starting column in JSScript and LazyScript.

Reduce fuzzing noise. Beta54+. Should be in 54 beta 7.

Attachment #8861704 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 49

•

2 years ago

Looks like this needs rebasing for Beta.

Flags: needinfo?(shu)

(Assignee)

Comment 50

•

2 years ago

Posted patch De-offset ScriptSource offsets from starting column in JSScript and LazyScript. Rebased for beta. — Details — Splinter Review

(Assignee)

Comment 51

•

2 years ago

Comment on attachment 8867012 [details] [diff] [review]
De-offset ScriptSource offsets from starting column in JSScript and LazyScript. Rebased for beta.

See comment 47.

Flags: needinfo?(shu)

Attachment #8867012 - Flags: approval-mozilla-beta?

(Assignee)

Updated

•

2 years ago

Attachment #8861704 - Flags: approval-mozilla-esr52?

Comment 52

•

2 years ago

Comment on attachment 8867012 [details] [diff] [review]
De-offset ScriptSource offsets from starting column in JSScript and LazyScript. Rebased for beta.

No need to re-request approval if it's just a regular rebase. Only if it makes significant changes from the original patch.

Attachment #8867012 - Flags: approval-mozilla-beta?

Comment 53

•

2 years ago

bugherderuplift

https://hg.mozilla.org/releases/mozilla-beta/rev/4e354dd10490

status-firefox54: affected → fixed