Closed
Bug 1161312
Opened 10 years ago
Closed 8 years ago
Crash with heap-buffer-overflow [@ js::frontend::TokenStream::TokenBuf::getRawChar()]
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: decoder, Assigned: shu)
References
Details
(4 keywords, Whiteboard: [jsbugmon:][fuzzblocker][adv-main54-])
Crash Data
Attachments
(2 files)
8.92 KB,
patch
|
jimb
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
8.23 KB,
patch
|
Details | Diff | Splinter Review |
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.
Comment 1•10 years ago
|
||
The stacks don't seem shell-specific so assuming the worst (that this is accessible in Firefox) for now.
Keywords: sec-high
Updated•10 years ago
|
Group: javascript-core-security
Reporter | ||
Updated•10 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
Reporter | ||
Comment 2•10 years ago
|
||
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Reporter | ||
Updated•10 years ago
|
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Comment 3•10 years ago
|
||
Jeff, can you look into this? Or should it be closed as incomplete?
Flags: needinfo?(jwalden+bmo)
Comment 4•10 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•10 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•10 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•10 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•10 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•10 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•10 years ago
|
||
I think Parser<SyntaxParseHandler>::finishFunctionDefinition is not passing the right value to LazyScript::CreateRaw; funbox->bufStart seems to be wrong.
Comment 11•10 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.
Updated•10 years ago
|
Group: core-security
Comment 12•10 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•10 years ago
|
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
Comment 16•9 years ago
|
||
Jeff, have you had a chance to look at this? Is there anybody else who could look at it? Thanks.
Comment 17•9 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?
Comment 18•9 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)
Comment 20•9 years ago
|
||
If this is shell-only, I'll mark this sec-audit. Please change it back if that's not right.
Comment 22•9 years ago
|
||
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)
Updated•9 years ago
|
status-firefox40:
affected → ---
status-firefox50:
--- → wontfix
status-firefox51:
--- → fix-optional
status-firefox52:
--- → fix-optional
status-firefox53:
--- → fix-optional
Reporter | ||
Comment 24•9 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•9 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•9 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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
||
Do we have enough information now to make a decision and get this fixed?
Flags: needinfo?(shu)
Flags: needinfo?(jimb)
Comment 33•8 years ago
|
||
We do not. It's not clear exactly which code is mis-using the API, or how.
Flags: needinfo?(jimb)
Comment 34•8 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•8 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•8 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•8 years ago
|
||
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•8 years ago
|
Flags: needinfo?(shu)
Comment 39•8 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•8 years ago
|
Assignee: jorendorff → shu
Shu-yu, is this ready for landing?
Flags: needinfo?(jwalden+bmo) → needinfo?(shu)
Assignee | ||
Comment 41•8 years ago
|
||
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
Comment 43•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 44•8 years ago
|
||
Gary, are there any branches that would benefit from a fuzzing standpoint by backporting this fix to them?
status-firefox54:
--- → affected
status-firefox-esr52:
--- → affected
Flags: needinfo?(gary)
Flags: in-testsuite+
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•8 years ago
|
||
Shu, can you please nominate this for Beta/ESR52 to help reduce fuzzing noise?
Flags: needinfo?(ryanvm) → needinfo?(shu)
Assignee | ||
Comment 47•8 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?
Comment 48•8 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+
Assignee | ||
Comment 50•8 years ago
|
||
Assignee | ||
Comment 51•8 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•8 years ago
|
Attachment #8861704 -
Flags: approval-mozilla-esr52?
Comment 52•8 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•8 years ago
|
||
bugherder uplift |
Comment 54•8 years ago
|
||
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.
Comment 55•8 years ago
|
||
(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-
Updated•8 years ago
|
Whiteboard: [jsbugmon:][fuzzblocker] → [jsbugmon:][fuzzblocker][adv-main54-]
You need to log in
before you can comment on or make changes to this bug.
Description
•