Closed Bug 1257722 Opened 4 years ago Closed 4 years ago

bug908915.js fails with -1073740791 on 64-bit Visual Studio 2015 builds

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: gps, Assigned: sfink)

References

Details

Attachments

(1 file)

This is related to bug 908915 - a previous sec-high bug. Purposefully not marking as depends on so as to not tip our hand.

When building with Visual Studio 2015 Update 1, JIT tests pass across the board except for a failure in bug908915.js in 64-bit builds.

Here is output from the Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d73b8a3fe6b:

 17:52:06  WARNING -  TEST-UNEXPECTED-FAIL | tests\jit-test\jit-test\tests\basic\bug908915.js |  (code -1073740791, args "--baseline-eager --no-fpu")
 17:52:06     INFO -  INFO exit-status     : -1073740791
 17:52:06     INFO -  INFO timed-out       : False
 17:52:06     INFO -  INFO stderr         2>
 17:52:06     INFO -  Exit code: -1073740791
 17:52:06     INFO -  FAIL - basic\bug908915.js
 17:52:06  WARNING -  TEST-UNEXPECTED-FAIL | tests\jit-test\jit-test\tests\basic\bug908915.js |  (code -1073740791, args "--ion-eager --ion-offthread-compile=off --non-writable-jitcode --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads")
 17:52:06     INFO -  INFO exit-status     : -1073740791
 17:52:06     INFO -  INFO timed-out       : False
 17:52:06     INFO -  INFO stderr         2>
 17:52:06     INFO -  Exit code: -1073740791
 17:52:06     INFO -  FAIL - basic\bug908915.js
 17:52:06  WARNING -  TEST-UNEXPECTED-FAIL | tests\jit-test\jit-test\tests\basic\bug908915.js |  (code -1073740791, args "")
 17:52:06     INFO -  INFO exit-status     : -1073740791
 17:52:06     INFO -  INFO timed-out       : False
 17:52:06     INFO -  INFO stderr         2>
 17:52:06     INFO -  Exit code: -1073740791
 17:52:06     INFO -  FAIL - basic\bug908915.js
 17:52:06  WARNING -  TEST-UNEXPECTED-FAIL | tests\jit-test\jit-test\tests\basic\bug908915.js |  (code -1073740791, args "--ion-eager --ion-offthread-compile=off")
 17:52:06     INFO -  INFO exit-status     : -1073740791
 17:52:06     INFO -  INFO timed-out       : False
 17:52:06     INFO -  INFO stderr         2>
 17:52:06     INFO -  Exit code: -1073740791
 17:52:06     INFO -  FAIL - basic\bug908915.js
 17:52:06  WARNING -  TEST-UNEXPECTED-FAIL | tests\jit-test\jit-test\tests\basic\bug908915.js |  (code -1073740791, args "--no-baseline --no-ion")
 17:52:06     INFO -  INFO exit-status     : -1073740791
 17:52:06     INFO -  INFO timed-out       : False
 17:52:06     INFO -  INFO stderr         2>
 17:52:06     INFO -  TEST-PASS | tests\jit-test\jit-test\tests\basic\bug920484.js | Success (code 0, args "--baseline-eager")
 17:52:06     INFO -  Exit code: -1073740791
 17:52:06     INFO -  FAIL - basic\bug908915.js
 17:52:06  WARNING -  TEST-UNEXPECTED-FAIL | tests\jit-test\jit-test\tests\basic\bug908915.js |  (code -1073740791, args "--baseline-eager")
 17:52:06     INFO -  INFO exit-status     : -1073740791
 17:52:06     INFO -  INFO timed-out       : False
 17:52:06 INFO - INFO stderr 2> 

Error code -1073740791 is a stack overflow.

I haven't yet reproduced this locally, so I don't have crash data. If you'd like to reproduce in automation, see bug 1124033 comment 21 for instructions to get VS2015 running in automation. Building with VS2015u1 locally should be sufficient.

While we don't yet ship VS2015 builds to users, I'm marking this bug as security sensitive because it could point to a security issue in existing code that our existing tests don't trigger. Better be safe than sorry.
I can reproduce on my Windows 10 machine with a 64-bit build:

Unhandled exception at 0x00007FFDADD18528 (ucrtbase.dll) in js.exe: An invalid parameter was passed to a function that considers invalid parameters fatal.

#if defined _NO_CRT_STDIO_INLINE
;
#else
{
    int _Result;
    va_list _ArgList;
    __crt_va_start(_ArgList, _Format);
    _Result = _vfprintf_l(_Stream, _Format, NULL, _ArgList);
    __crt_va_end(_ArgList);
    return _Result;
-> }
#endif

		_Format	0x00007ff693ab5f80 "# Roots.\n"	const char * const
		_Result	Error reading register value.	
		_Stream	ucrtbase.dll!0x00007ffdadd94168 (Type information missing from symbol file) {_Placeholder=0x0000000000000000 }	_iobuf * const


 	ucrtbase.dll!_invoke_watson()	Unknown
 	ucrtbase.dll!_invalid_parameter()	Unknown
 	ucrtbase.dll!_invalid_parameter_noinfo()	Unknown
 	ucrtbase.dll!_isatty()	Unknown
 	ucrtbase.dll!__acrt_stdio_begin_temporary_buffering_nolock()	Unknown
 	ucrtbase.dll!__crt_stdio_output::output_processor<char,class __crt_stdio_output::string_output_adapter<char>,class __crt_stdio_output::positional_parameter_base<char,class __crt_stdio_output::string_output_adapter<char> > >::output_processor<char,class __crt_stdio_output::string_output_adapter<char>,class __crt_stdio_output::positional_parameter_base<char,class __crt_stdio_output::string_output_adapter<char> > >(class __crt_stdio_output::string_output_adapter<char> const &,unsigned __int64,char const * const,struct __crt_locale_pointers * const,char * const)	Unknown
 	ucrtbase.dll!__crt_seh_guarded_call<int>::operator()<class <lambda_aa279a7bcb127014629d5b1d62c7325d>,class <lambda_0a64b6a4deda75c01876f29c44fc5eb3> &,class <lambda_3323e69fbefe9d1a11713e670d47cdfa> >(class <lambda_aa279a7bcb127014629d5b1d62c7325d> &&,class <lambda_0a64b6a4deda75c01876f29c44fc5eb3> &,class <lambda_3323e69fbefe9d1a11713e670d47cdfa> &&)	Unknown
 	ucrtbase.dll!__stdio_common_vfprintf()	Unknown
 	js.exe!fprintf(_iobuf * const _Stream, const char * const _Format, ...) Line 838	C++
 	js.exe!js::DumpHeap(JSRuntime * rt, _iobuf * fp, js::DumpHeapNurseryBehaviour nurseryBehaviour) Line 1044	C++
 	js.exe!DumpHeap(JSContext * cx, unsigned int argc, JS::Value * vp) Line 1461	C++
 	js.exe!js::Invoke(JSContext * cx, const JS::CallArgs & args, js::MaybeConstruct construct) Line 478	C++
 	js.exe!js::Invoke(JSContext * cx, const JS::Value & thisv, const JS::Value & fval, unsigned int argc, const JS::Value * argv, JS::MutableHandle<JS::Value> rval) Line 530	C++
 	js.exe!js::CrossCompartmentWrapper::call(JSContext * cx, JS::Handle<JSObject *> wrapper, const JS::CallArgs & args) Line 291	C++
 	js.exe!js::Proxy::call(JSContext * cx, JS::Handle<JSObject *> proxy, const JS::CallArgs & args) Line 391	C++
 	js.exe!js::proxy_Call(JSContext * cx, unsigned int argc, JS::Value * vp) Line 682	C++
 	js.exe!js::Invoke(JSContext * cx, const JS::CallArgs & args, js::MaybeConstruct construct) Line 478	C++
 	js.exe!js::Invoke(JSContext * cx, const JS::Value & thisv, const JS::Value & fval, unsigned int argc, const JS::Value * argv, JS::MutableHandle<JS::Value> rval) Line 530	C++
 	js.exe!js::jit::DoCallFallback(JSContext * cx, js::jit::BaselineFrame * frame, js::jit::ICCall_Fallback * stub_, unsigned int argc, JS::Value * vp, JS::MutableHandle<JS::Value> res) Line 6140	C++
 	[External Code]
Group: core-security → javascript-core-security
This bug is blocking the switch to VS2015.

jorendorff: could we please get an assessment of this bug so we know how if/how it impacts the switch to VS2015?
Flags: needinfo?(jorendorff)
Will take a look.
Flags: needinfo?(jorendorff) → needinfo?(jdemooij)
I can reproduce this with the following script:

  os.file.redirect(null);
  gc();
  backtrace();

The GC likely ends up releasing the last reference to the stdout RCFile and we fclose() stdout.

Then we call backtrace(), where we do fprintf(stdout, "%s", sprinter.string()) and we crash because the CRT notices stdout is invalid.

sfink, how is this supposed to work?
Flags: needinfo?(jdemooij) → needinfo?(sphink)
JS shell issue.
Group: javascript-core-security
Summary: bug908915.js fails with -1073740791 (stack overflow) on 64-bit Visual Studio 2015 builds → bug908915.js fails with -1073740791 on 64-bit Visual Studio 2015 builds
(In reply to Jan de Mooij [:jandem] from comment #4)
> I can reproduce this with the following script:
> 
>   os.file.redirect(null);
>   gc();
>   backtrace();
> 
> The GC likely ends up releasing the last reference to the stdout RCFile and
> we fclose() stdout.
> 
> Then we call backtrace(), where we do fprintf(stdout, "%s",
> sprinter.string()) and we crash because the CRT notices stdout is invalid.
> 
> sfink, how is this supposed to work?

Generally, the shell should now be doing a little logic to figure out what the right filehandle is to be writing to (check gOutFile->isOpen(), then use gOutFile->fp). But for cases like this, the RCFiles for stdout and stderr should be initialized with an extra reference. I'll bet I lost that in all the rebasing I ended up doing for this stuff. I'll take a look.
Assignee: nobody → sphink
Flags: needinfo?(sphink)
This should prevent stdout/stderr from getting closed at all. It's a little arguable whether it'd be ok to close them at the end of main(), but it seems safest to just not close them at all.
Attachment #8733951 - Flags: review?(terrence)
Comment on attachment 8733951 [details] [diff] [review]
Prevent stdout/stderr from getting closed

Review of attachment 8733951 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing. r=me.
Attachment #8733951 - Flags: review?(terrence) → review+
Comment on attachment 8733951 [details] [diff] [review]
Prevent stdout/stderr from getting closed

Review of attachment 8733951 [details] [diff] [review]:
-----------------------------------------------------------------

My last VS2015 Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2f956c69f12 was happy with this patch. Please land it ASAP!
Attachment #8733951 - Flags: feedback+
https://hg.mozilla.org/mozilla-central/rev/50b237b0fbaa
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.