chakra-core / ChakraCore

ChakraCore is an open source Javascript engine with a C API.
MIT License
9.09k stars 1.19k forks source link

Crash (AssertOrFailFast) in ShiftAddr() function #6707

Open rcane opened 3 years ago

rcane commented 3 years ago

The following code crashes after about 250 iterations.

for(let i=0;i<10000;++i) {
  let a=[0.0];
}

The crash seems to only happen in a release build and the value in the array needs to be float.

Environment

Windows 10 x64 Visual Studio 2019 16.9.4 ChakraCore: master at 8917a7e

Call stack

ChakraCore.dll!ReportFatalException(unsigned __int64 context, HRESULT exceptionCode, ErrorReason reasonCode, unsigned __int64) Line 26
    at L:\chakra\2021-04-18___8917a7e\ChakraCore-master\lib\Common\Exceptions\ReportError.cpp(26)
ChakraCore.dll!ShiftAddr(const ThreadContextInfo * const context, unsigned __int64 address) Line 563
    at L:\chakra\2021-04-18___8917a7e\ChakraCore-master\lib\Runtime\Base\ThreadContextInfo.cpp(563)
[Inline Frame] ChakraCore.dll!IR::GetMethodOriginalAddress(ThreadContextInfo * helperMethod, IR::JnHelperMethod) Line 296
    at L:\chakra\2021-04-18___8917a7e\ChakraCore-master\lib\Backend\JnHelperMethod.cpp(296)
[Inline Frame] ChakraCore.dll!IR::GetMethodAddress(ThreadContextInfo * context, IR::HelperCallOpnd *) Line 126
    at L:\chakra\2021-04-18___8917a7e\ChakraCore-master\lib\Backend\JnHelperMethod.cpp(126)
[Inline Frame] ChakraCore.dll!IR::Opnd::GetImmediateValue(Func * func) Line 464
    at L:\chakra\2021-04-18___8917a7e\ChakraCore-master\lib\Backend\Opnd.cpp(464)
ChakraCore.dll!PeepsMD::PeepAssign(IR::Instr * instr) Line 67
    at L:\chakra\2021-04-18___8917a7e\ChakraCore-master\lib\Backend\amd64\PeepsMD.cpp(67)
ChakraCore.dll!Peeps::PeepAssign(IR::Instr * assign) Line 379
    at L:\chakra\2021-04-18___8917a7e\ChakraCore-master\lib\Backend\Peeps.cpp(379)
ChakraCore.dll!Peeps::PeepFunc() Line 149
    at L:\chakra\2021-04-18___8917a7e\ChakraCore-master\lib\Backend\Peeps.cpp(149)
ChakraCore.dll!Func::TryCodegen() Line 516
    at L:\chakra\2021-04-18___8917a7e\ChakraCore-master\lib\Backend\Func.cpp(516)
ChakraCore.dll!Func::Codegen(Memory::JitArenaAllocator * alloc, JITTimeWorkItem * workItem, ThreadContextInfo * threadContextInfo, ScriptContextInfo * scriptContextInfo, JITOutputIDL * outputData, Js::EntryPointInfo * epInfo, const FunctionJITRuntimeInfo * const polymorphicInlineCacheInfo, JITTimePolymorphicInlineCacheInfo * const codeGenAllocators, void * const isBackgroundJIT, Js::ScriptContextProfiler * const) Line 326
    at L:\chakra\2021-04-18___8917a7e\ChakraCore-master\lib\Backend\Func.cpp(326)
ChakraCore.dll!NativeCodeGenerator::CodeGen(Memory::PageAllocatorBase<Memory::VirtualAllocWrapper,Memory::SegmentBase<Memory::VirtualAllocWrapper>,Memory::PageSegmentBase<Memory::VirtualAllocWrapper>> * pageAllocator, CodeGenWorkItemIDL * workItemData, JITOutputIDL & jitWriteData, const bool foreground, Js::EntryPointInfo * epInfo) Line 897
    at L:\chakra\2021-04-18___8917a7e\ChakraCore-master\lib\Backend\NativeCodeGenerator.cpp(897)
ChakraCore.dll!NativeCodeGenerator::CodeGen(Memory::PageAllocatorBase<Memory::VirtualAllocWrapper,Memory::SegmentBase<Memory::VirtualAllocWrapper>,Memory::PageSegmentBase<Memory::VirtualAllocWrapper>> * pageAllocator, CodeGenWorkItem * workItem, const bool foreground) Line 1023
    at L:\chakra\2021-04-18___8917a7e\ChakraCore-master\lib\Backend\NativeCodeGenerator.cpp(1023)
ChakraCore.dll!NativeCodeGenerator::Process(JsUtil::Job * const job, JsUtil::ParallelThreadData * threadData) Line 1908
    at L:\chakra\2021-04-18___8917a7e\ChakraCore-master\lib\Backend\NativeCodeGenerator.cpp(1908)
ChakraCore.dll!JsUtil::BackgroundJobProcessor::Process(JsUtil::Job * const job, JsUtil::ParallelThreadData * threadData) Line 1037
    at L:\chakra\2021-04-18___8917a7e\ChakraCore-master\lib\Common\Common\Jobs.cpp(1037)
ChakraCore.dll!JsUtil::BackgroundJobProcessor::Run(JsUtil::ParallelThreadData * threadData) Line 1135
    at L:\chakra\2021-04-18___8917a7e\ChakraCore-master\lib\Common\Common\Jobs.cpp(1135)
ChakraCore.dll!JsUtil::BackgroundJobProcessor::StaticThreadProc(void * lpParam) Line 1324
    at L:\chakra\2021-04-18___8917a7e\ChakraCore-master\lib\Common\Common\Jobs.cpp(1324)
rhuanjl commented 3 years ago

This crash occurs inside an ifdef that's only true on windows; also an unlikely code pattern + likely an old issue so marking as severity 2 for now.

rcane commented 3 years ago

The code I shared is the result of reducing the actual use case in my application which looks more like this:

updater = new DeltaUpdater(delta => {
        app.rotateObject(obj, [0.0, 2.0, 0.0], [0.0, 1.0, 0.0], delta);
    });

This sets up a callback that is invoked periodically from the C++ application. It crashes the application because of the two arrays passed to the function. Is this really an unusual code pattern as you say? The only alternative I see right now is to not use arrays at all.

Could I just disable ENABLE_OOP_NATIVE_CODEGEN as a temporary workaround?

rhuanjl commented 3 years ago

Yes you should be able to disable that unless you're using it?

OOP_NATIVE_CODEGEN enables a facility to run the JIT as a separate process to the engine see this PR for info on how to use it: https://github.com/chakra-core/ChakraCore/pull/6161

But if you're not using it and you're OK building CC yourself, yes just switch it off.

(Sorry for the less than helpful initial response - most recent bug reports have been from fuzzers so triaging how likely they are to affect real world use cases and hence what to focus on)

rhuanjl commented 3 years ago

You could perhaps also dodge the issue by creating the input arrays once and cacheing them in a local variable unless they need to be modified?

That aside I will try and fix this in due course - it's just unlikely to be easy/simple so may take some time.

rcane commented 3 years ago

Thank you for the quick response.

I did not known about that feature so I certainly did not use it (at least not knowingly). And since I build CC myself disabling it won't be a problem.

Btw, I did not take your initial response as being bad (that's why follow up comments were invented). And I certainly understand the time it takes to sift through bug reports and prioritize the little time you have. So please keep up the good work. Because I really don't want to be forced to use v8 again. It is so painful to integrate. ;-)

rhuanjl commented 3 years ago

One follow up question for when I try and fix this: are you linking to ChakraCore at build time or loading it dynamically as a dll?

rcane commented 3 years ago

I build ChakraCore as a dll with the supplied solution.

rhuanjl commented 3 years ago

This doesn't reproduce offline for me, building with visual studio 16.5.4.

The error is in low level methods that check the size of the loaded CC library in memory AND the size of the c-runtime; so I think it's probably been introduced by a change in a recent version of MSVC.

rcane commented 3 years ago

I am using the latest VS 16.9.4 here but have tested it on another computer with 16.8.2 as well. So maybe you are right and this was introduced by a VS version between 16.5.5 and 16.8.1. Bisecting this should be quite annoying since MS does not make it easy to downgrade VS.

Btw, I can confirm that disabling ENABLE_OOP_NATIVE_CODEGEN seems to fix (or at least hide) the problem. But it was not as easy as I'd hoped because just commenting out the define was not enough. Apparently the code was never tested with this flag disabled. I had to change eight files to get CC compile again.

rhuanjl commented 3 years ago

Thanks for the further investigation - I hesitate to upgrade to a newer version of VS to confirm the issue - I only have one windows machine available and as you've noted downgrading is not easy.

I may see if we can make the flag easier to disable - AND if/when we next post a release build of CC we'll (try to) ensure it doesn't have this crash either by building with this flag off OR using an older version of MSVC.

Marked for v1.12 as reminder to check these points before doing a release of 1.12.

MikeHolman commented 3 years ago

This code is verifying that any direct calls from JIT code are calling into either Chakra or the CRT. My initial guess is that the CRT dll you loaded has a different name than we are expecting (msvcrt.dll), as that is the most delicate part of this code. Is that something you can verify?

rcane commented 3 years ago

I don't do anything special with the CRT. And the debugger also shows that it loaded msvcrt.dll from "C:\Windows\System32" for my unit test. But since you mentioned msvcrt, I double checked our build script for chakra and I have to correct my initial statement about using the supplied solution as-is. I have actually changed the runtime from static to dll. Could this be a factor?

MikeHolman commented 3 years ago

That is definitely possible. Can you see what dll contains the address you are crashing on?

rcane commented 3 years ago

The failing address lies inside the vcruntime140.dll.

I also just tested with an unmodified version of chakra and I am embarrassed to say that the crash does not happen there. So I guess I owe you guys a beer now. ;-)

MikeHolman commented 3 years ago

Haha, don't worry about it! I think your config should be supported. As for a fix, I think changing ShiftAddr to only do the diff calculations if (JITManager::GetJITManager()->IsJITServer()) would be sufficient, as that is the only time an address actually needs to be adjusted.

rhuanjl commented 3 years ago

@MikeHolman thanks for the input on this - appreciated.

@rcane as the issue doesn't arise without your altered config I've removed the bug label; with your altered config disabling the ENABLE_OOP_NATIVE_CODEGEN flag should be fine - or you could put in the check MikeHolman's suggested before the conditions that fail to check at runtime instead of compile time.