Closed Bug 1415549 Opened 3 years ago Closed 3 years ago

Assertion failure: JS::StringIsASCII(args_[i]), at js/src/jscntxt.cpp:698

Categories

(Core :: JavaScript Engine, defect, P2)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: decoder, Assigned: bbouvier)

Details

(Keywords: assertion, bugmon, testcase, Whiteboard: [ossfuzz] [jsbugmon:update])

Attachments

(2 files)

This bug was found by Google's OSS-Fuzz running their custom internal JS fuzzer.

Please note that they apply a 90-day disclose timeline to all bugs.

The following testcase crashes on mozilla-central revision 4e6df5159df3 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe):

See attachment.


Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x00000000009ae627 in AutoMessageArgs::init (this=this@entry=0x7fffffffb430, cx=cx@entry=0x7ffff6948000, argsArg=argsArg@entry=0x0, countArg=countArg@entry=1, typeArg=typeArg@entry=js::ArgumentsAreASCII, ap=ap@entry=0x7fffffffb5e0) at js/src/jscntxt.cpp:698
#1  0x00000000009b3224 in ExpandErrorArgumentsHelper<JSErrorReport> (cx=cx@entry=0x7ffff6948000, callback=<optimized out>, callback@entry=0x970170 <js::GetErrorMessage(void*, unsigned int)>, userRef=userRef@entry=0x0, errorNumber=errorNumber@entry=317, messageArgs=messageArgs@entry=0x0, argumentsType=argumentsType@entry=js::ArgumentsAreASCII, reportp=0x7fffffffb550, ap=0x7fffffffb5e0) at js/src/jscntxt.cpp:799
#2  0x0000000000988f6b in js::ExpandErrorArgumentsVA (ap=0x7fffffffb5e0, reportp=0x7fffffffb550, argumentsType=js::ArgumentsAreASCII, messageArgs=0x0, errorNumber=317, userRef=0x0, callback=0x970170 <js::GetErrorMessage(void*, unsigned int)>, cx=0x7ffff6948000) at js/src/jscntxt.cpp:869
#3  js::ReportErrorNumberVA (cx=0x7ffff6948000, flags=flags@entry=0, callback=0x970170 <js::GetErrorMessage(void*, unsigned int)>, userRef=0x0, errorNumber=317, ap=ap@entry=0x7fffffffb5e0, argumentsType=js::ArgumentsAreASCII) at js/src/jscntxt.cpp:899
#4  0x0000000000988ffd in JS_ReportErrorNumberASCIIVA (cx=<optimized out>, errorCallback=<optimized out>, userRef=<optimized out>, errorNumber=<optimized out>, ap=ap@entry=0x7fffffffb5e0) at js/src/jsapi.cpp:6424
#5  0x0000000000989098 in JS_ReportErrorNumberASCII (cx=<optimized out>, errorCallback=<optimized out>, userRef=userRef@entry=0x0, errorNumber=errorNumber@entry=317) at js/src/jsapi.cpp:6413
#6  0x0000000000df05ad in GetImports (cx=0x7ffff6948000, module=..., importObj=..., importObj@entry=..., funcImports=..., funcImports@entry=..., tableImport=..., tableImport@entry=..., memoryImport=..., memoryImport@entry=..., globalImports=0x7fffffffb8c0) at js/src/wasm/WasmJS.cpp:243
#7  0x0000000000e07f4b in Instantiate (cx=cx@entry=0x7ffff6948000, module=..., importObj=importObj@entry=..., instanceObj=..., instanceObj@entry=...) at js/src/wasm/WasmJS.cpp:1088
#8  0x0000000000e08b1f in js::WasmInstanceObject::construct (cx=0x7ffff6948000, argc=<optimized out>, vp=<optimized out>) at js/src/wasm/WasmJS.cpp:1116
#9  0x000000000055e631 in js::CallJSNative (cx=0x7ffff6948000, native=native@entry=0xe08960 <js::WasmInstanceObject::construct(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:291
[...]
#32 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:8962
rax	0x0	0
rbx	0x7fffffffb5e0	140737488336352
rcx	0x7ffff6c282ad	140737333330605
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffb3e0	140737488335840
rsp	0x7fffffffb330	140737488335664
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9e7a0	140737332766624
r12	0x0	0
r13	0x1	1
r14	0x0	0
r15	0x7fffffffb430	140737488335920
rip	0x9ae627 <AutoMessageArgs::init(JSContext*, char16_t const**, unsigned short, js::ErrorArgumentsType, __va_list_tag*)+903>
=> 0x9ae627 <AutoMessageArgs::init(JSContext*, char16_t const**, unsigned short, js::ErrorArgumentsType, __va_list_tag*)+903>:	movl   $0x0,0x0
   0x9ae632 <AutoMessageArgs::init(JSContext*, char16_t const**, unsigned short, js::ErrorArgumentsType, __va_list_tag*)+914>:	ud2
Attached file Testcase
Whiteboard: [ossfuzz][jsbugmon:update,bisect] → [ossfuzz] [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/fbe76e704b6b
user:        Luke Wagner
date:        Thu Feb 09 18:32:39 2017 -0600
summary:     Bug 1338002 - Baldr: temporarily accept both 0xd and 0x1 (r=sunfish)

This iteration took 0.896 seconds to run.
This looks at a glance like it was read in from CacheableChars::deserialize, which seems to just read up a Pascal-style string and literally copy its bytes over, i.e. treating it as if Latin-1.  Is this function missing UTF-8 validation (and then the error-report including the property name needing to report using the UTF-8 error-reporting API, presumably)?
I've got a patch for this.
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Flags: needinfo?(luke)
Thanks for the help!  Since all wasm strings are now well-defined to be utf8:
  https://webassembly.github.io/spec/binary/values.html#names
can we just unconditionally call the UTF8-reporting variant?
Priority: -- → P2
Updated patch so all the error reporting under wasm/ uses the UTF8 reporting function; this is an error path anyways, and the two functions end up treating the string arguments the same way. So this is just a bit ahead of time.
Comment on attachment 8926843 [details]
Bug 1415549: Use correct encoding when reporting an error with wasm module/field utf8 names;

https://reviewboard.mozilla.org/r/198088/#review204168

Thanks!
Attachment #8926843 - Flags: review?(luke) → review+
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d331391aee4d
Use correct encoding when reporting an error with wasm module/field utf8 names; r=luke
https://hg.mozilla.org/mozilla-central/rev/d331391aee4d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Is there a user impact that warrants Beta uplift consideration here? If so, please nominate the patch for approval :)
Flags: needinfo?(bbouvier)
Flags: in-testsuite+
- this only happens when there's a JS object name that's encoded in utf8, *and*
- there's an error when generating the wasm module

That being said, it's an easy way to ddos the browser, so let's see if we could roll it with other things...
Flags: needinfo?(bbouvier)
Comment on attachment 8926843 [details]
Bug 1415549: Use correct encoding when reporting an error with wasm module/field utf8 names;

Approval Request Comment
[Feature/Bug causing the regression]: wasm
[User impact if declined]: easy way to crash a tab (when reporting an error, not likely to happen)
[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]: n/a
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not much
[Why is the change risky/not risky?]: using a more generic function that does the same thing as the previous function we were using
[String changes made/needed]: n/a
Attachment #8926843 - Flags: approval-mozilla-beta?
Comment on attachment 8926843 [details]
Bug 1415549: Use correct encoding when reporting an error with wasm module/field utf8 names;

Fix a potential tab crash issue. Beta58+.
Attachment #8926843 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.