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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Christian Holler (:decoder)]

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

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jan de Mooij [:jandem]]

Non-ASCII string here, looks like:

https://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/js/src/wasm/WasmJS.cpp#242-243

Comment 3

[Fuzzing Team]

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.

Comment 4

[Jeff Walden [:Waldo]]

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)?

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

I've got a patch for this.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Bug 1415549: Use correct encoding when reporting an error with wasm module/field utf8 names;

Review commit: https://reviewboard.mozilla.org/r/198088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198088/

Comment 7

[Luke Wagner [: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?

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8926843 [details]
Bug 1415549: Use correct encoding when reporting an error with wasm module/field utf8 names;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198088/diff/1-2/

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

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 10

[Luke Wagner [:luke]]

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!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d331391aee4d
Use correct encoding when reporting an error with wasm module/field utf8 names; r=luke

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/d331391aee4d

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact that warrants Beta uplift consideration here? If so, please nominate the patch for approval :)

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

- 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...

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 16

[Gerry Chang [:gchang]]

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+.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/def7425b015c