Crash with heap-buffer-overflow [@ js::frontend::TokenStream::TokenBuf::getRawChar()]

RESOLVED FIXED in Firefox 54

Status

()

defect
--
critical
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: decoder, Assigned: shu)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla55
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox-esr52 wontfix, firefox53 wontfix, firefox54 fixed, firefox55 fixed)

Details

(Whiteboard: [jsbugmon:][fuzzblocker][adv-main54-], crash signature)

Attachments

(2 attachments)

The following testcase crashes on mozilla-central revision dc5f85980a82 (build with --enable-gczeal --enable-optimize="-O2 -g" --enable-address-sanitizer --enable-posix-nspr-emulation --disable-jemalloc --disable-tests --disable-debug, run with --fuzzing-safe --thread-count=2):

test = (function () {
  function f() {
    [1,2,("\u00ee" ),4,5];
  };
  return "var obj = { x : 2 };" + f.toSource() + "; f()";
})();
evalWithCache(test);
function evalWithCache(code) {
  code = cacheEntry(code);
  var res1 = evaluate(code, Object.create(new Error("x"), {saveBytecode: { value: true } }));
}



Backtrace:

==3159==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60d0000396f0 at pc 0x83964e bp 0x7fff805b0890 sp 0x7fff805b0888
READ of size 2 at 0x60d0000396f0 thread T0
    #0 0x83964d in js::frontend::TokenStream::TokenBuf::getRawChar() js/src/frontend/TokenStream.h:705
    #1 0x83964d in js::frontend::TokenStream::getTokenInternal(js::frontend::TokenKind*, js::frontend::TokenStream::Modifier) js/src/frontend/TokenStream.cpp:1129
    #2 0x5561cc in js::frontend::TokenStream::peekTokenPos(js::frontend::TokenPos*, js::frontend::TokenStream::Modifier) js/src/frontend/TokenStream.h:412
    #3 0x5561cc in js::frontend::Parser<js::frontend::FullParseHandler>::standaloneLazyFunction(JS::Handle<JSFunction*>, unsigned int, bool, js::GeneratorKind) js/src/frontend/Parser.cpp:2452
    #4 0x7bc402 in js::frontend::CompileLazyFunction(JSContext*, JS::Handle<js::LazyScript*>, char16_t const*, unsigned long) js/src/frontend/BytecodeCompiler.cpp:489
    #5 0x1486431 in JSFunction::createScriptForLazilyInterpretedFunction(JSContext*, JS::Handle<JSFunction*>) js/src/jsfun.cpp:1479
    #6 0x14859cd in JSFunction::getOrCreateScript(JSContext*) js/src/jsfun.h:315
    #7 0x14859cd in js::LazyScript::functionDelazifying(JSContext*) const js/src/jsscriptinlines.h:55
    #8 0x14859cd in JSFunction::createScriptForLazilyInterpretedFunction(JSContext*, JS::Handle<JSFunction*>) js/src/jsfun.cpp:1427
    #9 0x926096 in JSFunction::getOrCreateScript(JSContext*) js/src/jsfun.h:315
    #10 0x926096 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:2966
    #11 0x9041f1 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:677
    #12 0x936b0d in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) js/src/vm/Interpreter.cpp:902
    #13 0x937154 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/vm/Interpreter.cpp:941
    #14 0x1409a2d in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Value*) js/src/jsapi.cpp:4159
    #15 0x4e63cd in Evaluate(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:1327
    #16 0x8e400f in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:235
    #17 0x8e400f in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:720
    #18 0x9250dc in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:2956
    #19 0x9041f1 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:677
    #20 0x936b0d in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) js/src/vm/Interpreter.cpp:902
    #21 0x937154 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/vm/Interpreter.cpp:941
    #22 0x1409a2d in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Value*) js/src/jsapi.cpp:4159
    #23 0x4dad1f in RunFile(JSContext*, char const*, _IO_FILE*, bool) js/src/shell/js.cpp:467
    #24 0x4dad1f in Process(JSContext*, char const*, bool) js/src/shell/js.cpp:597
    #25 0x4d2a4d in ProcessArgs(JSContext*, js::cli::OptionParser*) js/src/shell/js.cpp:5798
    #26 0x4d2a4d in Shell(JSContext*, js::cli::OptionParser*, char**) js/src/shell/js.cpp:6064
    #27 0x4d2a4d in main js/src/shell/js.cpp:6385
    #28 0x7f3d746caec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287
    #29 0x4c59bc in _start (/home/decoder/LangFuzz/work/remote/builds/opt64asan/dist/bin/js+0x4c59bc)

0x60d0000396f0 is located 4 bytes to the right of 140-byte region [0x60d000039660,0x60d0000396ec)
allocated by thread T0 here:
    #0 0x4ac297 in __interceptor_malloc /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
    #1 0xc1085f in js_malloc(unsigned long) js/src/opt64asan/js/src/../../dist/include/js/Utility.h:119
    #2 0xc1085f in _ZL13js_pod_mallocIDsEPT_m js/src/opt64asan/js/src/../../dist/include/js/Utility.h:274
    #3 0xc1085f in char16_t* js::MallocProvider<JS::Zone>::pod_malloc<char16_t>(unsigned long) js/src/vm/MallocProvider.h:63
    #4 0x1564876 in js::ScriptSource::ensureOwnsSource(js::ExclusiveContext*) js/src/jsscript.cpp:1706
    #5 0x1564876 in js::ScriptSource::setSourceCopy(js::ExclusiveContext*, JS::SourceBufferHolder&, bool, js::SourceCompressionTask*) js/src/jsscript.cpp:1757
    #6 0x7b04bc in js::frontend::CompileScript(js::ExclusiveContext*, js::LifoAlloc*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Handle<js::StaticEvalObject*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JSString*, unsigned int, js::SourceCompressionTask*) js/src/frontend/BytecodeCompiler.cpp:258
    #7 0x1404ccb in JS::Compile(JSContext*, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) js/src/jsapi.cpp:3818
    #8 0x1404fea in JS::Compile(JSContext*, JS::ReadOnlyCompileOptions const&, char16_t const*, unsigned long, JS::MutableHandle<JSScript*>) js/src/jsapi.cpp:3828
    #9 0x4e5255 in Evaluate(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:1294
    #10 0x8e400f in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:235
    #11 0x8e400f in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:720
    #12 0x9250dc in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:2956
    #13 0x9041f1 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:677
    #14 0x936b0d in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) js/src/vm/Interpreter.cpp:902
    #15 0x937154 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/vm/Interpreter.cpp:941
    #16 0x1409a2d in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Value*) js/src/jsapi.cpp:4159
    #17 0x4dad1f in RunFile(JSContext*, char const*, _IO_FILE*, bool) js/src/shell/js.cpp:467
    #18 0x4dad1f in Process(JSContext*, char const*, bool) js/src/shell/js.cpp:597
    #19 0x4d2a4d in ProcessArgs(JSContext*, js::cli::OptionParser*) js/src/shell/js.cpp:5798
    #20 0x4d2a4d in Shell(JSContext*, js::cli::OptionParser*, char**) js/src/shell/js.cpp:6064
    #21 0x4d2a4d in main js/src/shell/js.cpp:6385
    #22 0x7f3d746caec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287

SUMMARY: AddressSanitizer: heap-buffer-overflow js/src/frontend/TokenStream.h:705 js::frontend::TokenStream::TokenBuf::getRawChar()
Shadow bytes around the buggy address:
  0x0c1a7ffff280: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c1a7ffff290: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c1a7ffff2a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c1a7ffff2b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c1a7ffff2c0: fa fa fa fa fa fa fa fa fa fa fa fa 00 00 00 00
=>0x0c1a7ffff2d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 04[fa]fa
  0x0c1a7ffff2e0: fa fa fa fa fa fa 00 00 00 00 00 00 00 00 00 00
  0x0c1a7ffff2f0: 00 00 00 00 00 00 00 06 fa fa fa fa fa fa fa fa
  0x0c1a7ffff300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1a7ffff310: 06 fa fa fa fa fa fa fa fa fa 00 00 00 00 00 00
  0x0c1a7ffff320: 00 00 00 00 00 00 00 00 00 00 04 fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Contiguous container OOB:fc
  ASan internal:           fe
==3159==ABORTING



Marking s-s due to heap-buffer-overflow, but this could be a shell-only bug in the saveBytecode as well.
The stacks don't seem shell-specific so assuming the worst (that this is accessible in Firefox) for now.
Keywords: sec-high
Group: javascript-core-security
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Jeff, can you look into this? Or should it be closed as incomplete?
Flags: needinfo?(jwalden+bmo)
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)
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)
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.
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.
(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.
(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.
I think Parser<SyntaxParseHandler>::finishFunctionDefinition is not passing the right value to LazyScript::CreateRaw; funbox->bufStart seems to be wrong.
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.
Group: core-security
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.
Duplicate of this bug: 1207409
Crash Signature: [@ js::frontend::TokenStream::TokenBuf::getRawChar()] → [@ js::frontend::TokenStream::TokenBuf::getRawChar()] [@ js::frontend::TokenStream::TokenBuf::getRawChar]
Assigning to :Waldo based on comment 12.
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
See Also: → 1209501
Duplicate of this bug: 1232682
Jeff, have you had a chance to look at this? Is there anybody else who could look at it? Thanks.
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?
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)
Duplicate of this bug: 1261817
If this is shell-only, I'll mark this sec-audit. Please change it back if that's not right.
Keywords: sec-highsec-audit
Duplicate of this bug: 1308575
Debug builds now fail an assert I added in bug 1304390, making this fail more reliably.
:Waldo, what's next here?
Flags: needinfo?(jwalden+bmo)
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)
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)
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.
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)
(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)
(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]`.
(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?
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.
Do we have enough information now to make a decision and get this fixed?
Flags: needinfo?(shu)
Flags: needinfo?(jimb)
We do not. It's not clear exactly which code is mis-using the API, or how.
Flags: needinfo?(jimb)
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.
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
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]
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)
Flags: needinfo?(shu)
Duplicate of this bug: 1209501
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+
Assignee: jorendorff → shu
Shu-yu, is this ready for landing?
Flags: needinfo?(jwalden+bmo) → needinfo?(shu)
I just landed it. We should also open this bug, because it is shell-only.
Flags: needinfo?(shu)
Yep, apparently you just did:

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

Opening up as per comment 41.
Group: javascript-core-security
https://hg.mozilla.org/mozilla-central/rev/041899558dd5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Gary, are there any branches that would benefit from a fuzzing standpoint by backporting this fix to them?
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)
Shu, can you please nominate this for Beta/ESR52 to help reduce fuzzing noise?
Flags: needinfo?(ryanvm) → needinfo?(shu)
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?
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+
Looks like this needs rebasing for Beta.
Flags: needinfo?(shu)
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?
Attachment #8861704 - Flags: approval-mozilla-esr52?
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?
Looks like this patch is going to require further rebasing for ESR52. I discussed it with Gary and he agreed that it's not worth spending more time on rebasing.
(In reply to Shu-yu Guo [:shu] from comment #47)
> [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

Setting qe-verify- based on Shu-yu Guo's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Whiteboard: [jsbugmon:][fuzzblocker] → [jsbugmon:][fuzzblocker][adv-main54-]
You need to log in before you can comment on or make changes to this bug.