1584027 - Hit MOZ_CRASH(invalid UTF-8 string: ReportInvalidCharacter) at js/src/vm/CharacterEncoding.cpp:333 with setTestFilenameValidationCallback

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision c31591e0b66f (build with --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

setTestFilenameValidationCallback();
evaluate("throw 2", {fileName: "\uDEFF"})

Backtrace:

received signal SIGSEGV, Segmentation fault.
InflateUTF8ToUTF16<(OnUTF8Error)3, JS::ConstUTF8CharsZ::validate(size_t)::<lambda(char16_t)>, JS::UTF8Chars> (cx=0x0, src=..., dst=...) at js/src/vm/CharacterEncoding.cpp:362
#0  InflateUTF8ToUTF16<(OnUTF8Error)3, JS::ConstUTF8CharsZ::validate(size_t)::<lambda(char16_t)>, JS::UTF8Chars> (cx=0x0, src=..., dst=...) at js/src/vm/CharacterEncoding.cpp:362
#1  JS::ConstUTF8CharsZ::validate (this=this@entry=0x7fffffffadb0, aLength=<optimized out>) at js/src/vm/CharacterEncoding.cpp:652
#2  0x0000555555b70af4 in JS::ConstUTF8CharsZ::ConstUTF8CharsZ (aLength=<optimized out>, aBytes=0x7ffff4774ba0 "unsafe filename: \377", this=0x7fffffffadb0) at dist/include/js/CharacterEncoding.h:147
#3  JSErrorBase::initBorrowedMessage (this=this@entry=0x7fffffffaf50, messageArg=messageArg@entry=0x7ffff4774ba0 "unsafe filename: \377") at dist/include/js/ErrorReport.h:131
#4  0x0000555555b747fb in JSErrorBase::initOwnedMessage (messageArg=0x7ffff4774ba0 "unsafe filename: \377", this=0x7fffffffaf50) at dist/include/js/ErrorReport.h:126
#5  ExpandErrorArgumentsHelper<JSErrorReport> (cx=0x7ffff5f27000, callback=<optimized out>, userRef=<optimized out>, errorNumber=125, messageArgs=<optimized out>, argumentsType=<optimized out>, reportp=0x7fffffffaf50, ap=0x7fffffffb040) at js/src/vm/JSContext.cpp:757
#6  0x0000555555b5f280 in js::ExpandErrorArgumentsVA (ap=0x7fffffffb040, reportp=0x7fffffffaf50, argumentsType=js::ArgumentsAreUTF8, messageArgs=0x0, errorNumber=125, userRef=0x0, callback=0x555555b46560 <js::GetErrorMessage(void*, unsigned int)>, cx=0x7ffff5f27000) at js/src/vm/JSContext.cpp:792
#7  js::ReportErrorNumberVA (cx=cx@entry=0x7ffff5f27000, flags=<optimized out>, flags@entry=0, callback=callback@entry=0x555555b46560 <js::GetErrorMessage(void*, unsigned int)>, userRef=userRef@entry=0x0, errorNumber=errorNumber@entry=125, argumentsType=argumentsType@entry=js::ArgumentsAreUTF8, ap=0x7fffffffb040) at js/src/vm/JSContext.cpp:820
#8  0x0000555555e82b68 in JS_ReportErrorNumberUTF8VA (cx=0x7ffff5f27000, errorCallback=0x555555b46560 <js::GetErrorMessage(void*, unsigned int)>, userRef=0x0, errorNumber=125, ap=ap@entry=0x7fffffffb040) at js/src/jsapi.cpp:4821
#9  0x0000555555e82c18 in JS_ReportErrorNumberUTF8 (cx=cx@entry=0x7ffff5f27000, errorCallback=<optimized out>, userRef=userRef@entry=0x0, errorNumber=errorNumber@entry=125) at js/src/jsapi.cpp:4810
#10 0x0000555555bb1e12 in MaybeValidateFilename (options=..., sso=..., cx=0x7ffff5f27000) at js/src/vm/JSScript.cpp:1721
#11 js::ScriptSourceObject::initFromOptions (cx=<optimized out>, source=source@entry=..., options=...) at js/src/vm/JSScript.cpp:1737
#12 0x0000555556050032 in js::frontend::CreateScriptSourceObject (cx=<optimized out>, options=..., parameterListEnd=...) at js/src/frontend/BytecodeCompiler.cpp:729
#13 0x000055555605017a in js::frontend::BytecodeCompiler::createScriptSource (this=this@entry=0x7fffffffbf50, parameterListEnd=...) at js/src/frontend/BytecodeCompiler.cpp:401
#14 0x000055555607b0e8 in js::frontend::SourceAwareCompiler<char16_t>::createSourceAndParser (this=this@entry=0x7fffffffb3d0, allocScope=..., info=..., goal=goal@entry=js::frontend::ParseGoal::Script, parameterListEnd=...) at js/src/frontend/BytecodeCompiler.cpp:419
#15 0x0000555556071544 in js::frontend::SourceAwareCompiler<char16_t>::prepareScriptParse (info=..., allocScope=..., this=0x7fffffffb3d0) at js/src/frontend/BytecodeCompiler.cpp:128
#16 js::frontend::ScriptCompiler<char16_t>::prepareScriptParse (compiler=..., allocScope=..., this=0x7fffffffb3d0) at js/src/frontend/BytecodeCompiler.cpp:189
#17 CreateGlobalScript<char16_t> (info=..., srcBuf=..., sourceObjectOut=sourceObjectOut@entry=0x0) at js/src/frontend/BytecodeCompiler.cpp:206
#18 0x000055555607186a in js::frontend::CompileGlobalScript (info=..., srcBuf=..., sourceObjectOut=sourceObjectOut@entry=0x0) at js/src/frontend/BytecodeCompiler.cpp:223
#19 0x0000555555aebab3 in CompileSourceBuffer<char16_t> (cx=0x7ffff5f27000, options=..., srcBuf=...) at js/src/vm/CompilationAndEvaluation.cpp:69
#20 0x00005555558ecbc6 in Evaluate (cx=<optimized out>, cx@entry=0x7ffff5f27000, argc=<optimized out>, vp=<optimized out>) at js/src/shell/js.cpp:2231
#21 0x000055555598ef5d in CallJSNative (cx=0x7ffff5f27000, native=native@entry=0x5555558ec0d0 <Evaluate(JSContext*, unsigned int, JS::Value*)>, reason=reason@entry=js::CallReason::Call, args=...) at js/src/vm/Interpreter.cpp:458
[...]
#35 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:11496
rax	0x555557fd70e0	93825036808416
rbx	0x8	8
rcx	0x555556e468e8	93825018390760
rdx	0x0	0
rsi	0x7ffff6eeb770	140737336227696
rdi	0x7ffff6eea540	140737336223040
rbp	0x7fffffffada0	140737488334240
rsp	0x7fffffffacf0	140737488334064
r8	0x7ffff6eeb770	140737336227696
r9	0x7ffff7fe6cc0	140737354034368
r10	0x58	88
r11	0x7ffff6b927a0	140737332717472
r12	0x11	17
r13	0x7fffffffad30	140737488334128
r14	0x11	17
r15	0x7ffff4774bb2	140737294846898
rip	0x555555abef2f <JS::ConstUTF8CharsZ::validate(unsigned long)+543>
=> 0x555555abef2f <JS::ConstUTF8CharsZ::validate(unsigned long)+543>:	movl   $0x0,0x0
   0x555555abef3a <JS::ConstUTF8CharsZ::validate(unsigned long)+554>:	ud2

Likely a shell-only issue.

Comment 1

[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/18d269bf8559
user:        Jan de Mooij
date:        Fri Sep 20 07:41:30 2019 +0000
summary:     Bug 1577280 - Add a script filename validation callback. r=tcampbell

This iteration took 484.339 seconds to run.

Comment 2

[Jan de Mooij [:jandem]]

Hey Waldo, I'd like to start enforcing (read: release-assert) the filename we get in CompileOptions code is UTF8 so that the rest of the engine can safely rely on it. Do you have any thoughts, concerns, suggestions there?

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)

Hey Waldo, I'd like to start enforcing (read: release-assert) the filename we get in CompileOptions code is UTF8 so that the rest of the engine can safely rely on it. Do you have any thoughts, concerns, suggestions there?

That said, the easiest fix for this bug is to check for invalid UTF8 before I throw the exception. Maybe I should do that for now....

Comment 4

[Jeff Walden [:Waldo]]

File names aren't guaranteed to be valid Unicode without surrogates on Windows, as I understand it. Hence why Rust has a separate type for file names, that is not just &str, and is instead WTF-8. If we're fine losing information here because it's only roughly for display purposes, very well, but we will be losing information.

Seems like the usual approach is going to be about the thing to do, use a chokepoint function that sets the file name and enforces a conversion/validity check. I reviewed patchwork implementing something along these lines, or at least in that general direciton, multiple times in bug 987069, then for reasons I never tracked down it got set aside. :-|

Comment 5

[André Bargull [:anba]]

Also see bug 1492090 for change-sets which change the filename to be UTF-8.

Comment 6

[Tom Ritter [:tjr] (OOTO Oct 18-26)]

<My comment was wrong>

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1584027 - Don't include filename in exception message if it's invalid UTF-8. r?jwalden!

Fuzzers can trigger this in the JS shell and until we enforce filenames
are valid UTF-8 we have to deal with it somehow.

Comment 8

[Jan de Mooij [:jandem]]

With the patch I just posted we throw unsafe filename: (invalid UTF-8 filename) instead of including the actual filename. I think that's good enough for this non-standard feature.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28171f948a08
Don't include filename in exception message if it's invalid UTF-8. r=jwalden

Comment 10

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/28171f948a08