Summary: | sanitizeStackForVMImpl writes below stack pointer, triggers huge warning spam from valgrind | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Critical | CC: | bugs-noreply, cgarcia, ews-watchlist, federicosantamorena, guijemont, keith_miller, mark.lam, mcatanzaro, mcrha, msaboff, saam, tzagallo, webkit-bug-importer, xan.lopez, ysuzuki | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | PC | ||||||||
OS: | Linux | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 204997 | ||||||||
Attachments: |
|
Description
Michael Catanzaro
2019-06-27 15:49:37 PDT
My suspicion is maybe something wrong in jscContextSetVirtualMachine, but I don't see the problem so I don't really know. I'm getting similar errors when running jsc c api tests with valgrind, so I don't think this is specific to the glib api (nor even to GTK and WPE) ==12053== Invalid write of size 8 ==12053== at 0x53ED29C: ??? (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x4C7A3AA: JSC::LocalAllocator::allocate(JSC::GCDeferralContext*, JSC::AllocationFailureMode)::{lambda()#1}::operator()() const (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x55FDD76: JSC::JSFunction::create(JSC::VM&, JSC::JSGlobalObject*, int, WTF::String const&, JSC::NativeFunction, JSC::Intrinsic, JSC::NativeFunction, JSC::DOMJIT::Signature const*) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x55AA06E: JSC::FunctionPrototype::addFunctionProperties(JSC::VM&, JSC::JSGlobalObject*, JSC::JSFunction**, JSC::JSFunction**, JSC::JSFunction**) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x561E183: JSC::JSGlobalObject::init(JSC::VM&) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x5626B2D: JSC::JSGlobalObject::finishCreation(JSC::VM&) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x4C82895: JSGlobalContextCreateInGroup (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x117FC8: WTF::Detail::CallableWrapper<testCAPIViaCpp::{lambda()#11}, void>::call() (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/bin/testapi) ==12053== by 0x58E3DCA: WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x593F008: WTF::wtfThreadEntryPoint(void*) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x7B0DFA2: start_thread (pthread_create.c:486) ==12053== by 0x84934CE: clone (clone.S:95) ==12053== Address 0xbe029c8 is on thread 7's stack ==12053== 368 bytes below stack pointer ==12053== ==12053== Invalid write of size 8 ==12053== at 0x53ED29C: ??? (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x4E624B3: JSC::UnlinkedFunctionExecutable::link(JSC::VM&, JSC::ScriptExecutable*, JSC::SourceCode const&, WTF::Optional<int>, JSC::Intrinsic) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x4C6F54F: JSC::functionPrototypeApplyCodeGenerator(JSC::VM&) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x55AA09D: JSC::FunctionPrototype::addFunctionProperties(JSC::VM&, JSC::JSGlobalObject*, JSC::JSFunction**, JSC::JSFunction**, JSC::JSFunction**) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x561E183: JSC::JSGlobalObject::init(JSC::VM&) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x5626B2D: JSC::JSGlobalObject::finishCreation(JSC::VM&) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x4C82895: JSGlobalContextCreateInGroup (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x117FC8: WTF::Detail::CallableWrapper<testCAPIViaCpp::{lambda()#11}, void>::call() (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/bin/testapi) ==12053== by 0x58E3DCA: WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x593F008: WTF::wtfThreadEntryPoint(void*) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x7B0DFA2: start_thread (pthread_create.c:486) ==12053== by 0x84934CE: clone (clone.S:95) ==12053== Address 0xbe02918 is on thread 7's stack ==12053== 416 bytes below stack pointer ==12053== ==12053== Invalid write of size 8 ==12053== at 0x53ED29C: ??? (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x526A88F: JSC::CompleteSubspace::tryAllocateSlow(JSC::VM&, unsigned long, JSC::GCDeferralContext*) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x526AAF8: JSC::CompleteSubspace::allocateSlow(JSC::VM&, unsigned long, JSC::GCDeferralContext*, JSC::AllocationFailureMode) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x56BBAE0: JSC::ObjectPrototype::create(JSC::VM&, JSC::JSGlobalObject*, JSC::Structure*) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x561E507: JSC::JSGlobalObject::init(JSC::VM&) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x5626B2D: JSC::JSGlobalObject::finishCreation(JSC::VM&) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x4C82895: JSGlobalContextCreateInGroup (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x117FC8: WTF::Detail::CallableWrapper<testCAPIViaCpp::{lambda()#11}, void>::call() (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/bin/testapi) ==12053== by 0x58E3DCA: WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x593F008: WTF::wtfThreadEntryPoint(void*) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x7B0DFA2: start_thread (pthread_create.c:486) ==12053== by 0x84934CE: clone (clone.S:95) ==12053== Address 0xbe02968 is on thread 7's stack ==12053== 496 bytes below stack pointer ==12053== ==12053== Invalid write of size 8 ==12053== at 0x53ED29C: ??? (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x5734C7E: JSC::StructureRareData::create(JSC::VM&, JSC::Structure*) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x5734CC6: JSC::Structure::allocateRareData(JSC::VM&) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x5738787: JSC::Structure::ensurePropertyReplacementWatchpointSet(JSC::VM&, int) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x4E484A2: JSC::PropertyCondition::isWatchableWhenValid(JSC::Structure*, JSC::PropertyCondition::WatchabilityEffort) const (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x560FF72: JSC::JSGlobalObject::init(JSC::VM&)::{lambda(JSC::JSObject*, JSC::Identifier const&)#72}::operator()(JSC::JSObject*, JSC::Identifier const&) const [clone .isra.226] (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x5624B62: JSC::JSGlobalObject::init(JSC::VM&) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x5626B2D: JSC::JSGlobalObject::finishCreation(JSC::VM&) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x4C82895: JSGlobalContextCreateInGroup (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x117FC8: WTF::Detail::CallableWrapper<testCAPIViaCpp::{lambda()#11}, void>::call() (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/bin/testapi) ==12053== by 0x58E3DCA: WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x593F008: WTF::wtfThreadEntryPoint(void*) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== Address 0x4ecff858 is on thread 12's stack ==12053== 656 bytes below stack pointer ==12053== ==12053== Invalid write of size 8 ==12053== at 0x53ED29C: ??? (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x4C7A3AA: JSC::LocalAllocator::allocate(JSC::GCDeferralContext*, JSC::AllocationFailureMode)::{lambda()#1}::operator()() const (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x5715351: void* JSC::allocateCell<JSC::ProgramCodeBlock>(JSC::Heap&, unsigned long) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x57113CB: JSC::ScriptExecutable::newCodeBlockFor(JSC::CodeSpecializationKind, JSC::JSFunction*, JSC::JSScope*, JSC::Exception*&) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x5714436: JSC::ScriptExecutable::prepareForExecutionImpl(JSC::VM&, JSC::JSFunction*, JSC::JSScope*, JSC::CodeSpecializationKind, JSC::CodeBlock*&) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x5331B57: JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x55737CC: JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x5573957: JSC::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x4C76CC6: JSEvaluateScriptInternal (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x4C76F5A: JSEvaluateScript (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18.14.1) ==12053== by 0x11882A: WTF::SharedTaskFunctor<void (TestAPI&), testCAPIViaCpp::{lambda(TestAPI&)#2}>::run(TestAPI&) (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/bin/testapi) ==12053== by 0x11810E: WTF::Detail::CallableWrapper<testCAPIViaCpp::{lambda()#11}, void>::call() (in /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/bin/testapi) ==12053== Address 0x4ecffca8 is on thread 12's stack ==12053== 656 bytes below stack pointer ==12053== (In reply to Carlos Garcia Campos from comment #2) > I'm getting similar errors when running jsc c api tests with valgrind, so I > don't think this is specific to the glib api (nor even to GTK and WPE) Thanks. What command do you use to use run-javascriptcore-tests under valgrind? I just run testapi from bin dir under valgrind It seems to be sanitizeStackForVMImpl what makes valgrind complain. Oh dear, from LowLevelInterpreter.asm? This is beyond me.... Last change was r229481 "[Re-landing] Prepare LLInt code to support pointer profiling." I don't remember the last time I tried valgrind but I suppose it could have been that long.... I can confirm, I have the same exact problem. using the standard webkit2gtk-4.0 on Fedora causes this problem when running some Javascript scripts with version 2.24.3-1.fc30 webkit_web_view_run_javascript( webview, "var variable_name = {}", null, null, null ); will corrupt memory I think this isn't receiving the attention it needs from JSC devs. Stack corruption is serious and this occurs every time JSGlobalContext is created. I don't know much about asm, but if I understand the code correctly, it zeroes the stack from VM::m_lastStackTop to sp. It shouldn't change the m_lastStackTop, right? for some reason it's changing it, sometimes with values outside the stack bounds, which is what causes the valgrind errors. sanitizeStackForVMImpl is a bit low-level function which clears unused stack spaces to make conservative GC work well. I think this is false-positive reports from valgrind. Hm, I'm not sure what to do about it. We don't have a valgrind suppression file, and don't really want to add one because nobody ever actually uses those when debugging or reporting bugs. Ideally, WebKit would not do anything that triggers complaints from valgrind. This is currently the only false-positive reported by valgrind that's directly WebKit's fault. We also have bug #146729, where we write uninitialized memory as part of WebKit IPC, which is harmless but clearly something to be fixed, so not a false-positive. Also, we have some issues with dependencies in bug #204997. I thought this had been introduced with r227617, aka bug #181559, but I can reproduce it (on the WebKitWebProcess side) also with r227616. I'm not sure whether this information is good for anything. Here's what it looks like with --track-origins: ==449866== Invalid write of size 8 ==449866== at 0x9F56DCB: ??? (in /home/mcatanzaro/Projects/GNOME/install/lib/libjavascriptcoregtk-4.0.so.18.17.0) ==449866== by 0xA0CB298: operator() (LocalAllocatorInlines.h:39) ==449866== by 0xA0CB298: allocate<JSC::LocalAllocator::allocate(JSC::Heap&, JSC::GCDeferralContext*, JSC::AllocationFailureMode)::<lambda()> > (FreeListInlines.h:46) ==449866== by 0xA0CB298: allocate (LocalAllocatorInlines.h:37) ==449866== by 0xA0CB298: allocate (AllocatorInlines.h:35) ==449866== by 0xA0CB298: allocateNonVirtual (IsoSubspaceInlines.h:34) ==449866== by 0xA0CB298: tryAllocateCellHelper<JSC::ArrayPrototype> (JSCellInlines.h:163) ==449866== by 0xA0CB298: allocateCell<JSC::ArrayPrototype> (JSCellInlines.h:177) ==449866== by 0xA0CB298: JSC::ArrayPrototype::create(JSC::VM&, JSC::JSGlobalObject*, JSC::Structure*) (ArrayPrototype.cpp:72) ==449866== by 0xA1D0CA2: JSC::JSGlobalObject::init(JSC::VM&) (JSGlobalObject.cpp:703) ==449866== by 0xA1D78E8: JSC::JSGlobalObject::finishCreation(JSC::VM&) (JSGlobalObject.cpp:2144) ==449866== by 0x96DE09D: create (JSAPIGlobalObject.h:51) ==449866== by 0x96DE09D: JSGlobalContextCreateInGroup (JSContextRef.cpp:143) ==449866== by 0x967E901: jscContextSetVirtualMachine(_JSCContext*, WTF::GRefPtr<_JSCVirtualMachine>&&) (JSCContext.cpp:107) ==449866== by 0x9681DEE: jscContextConstructed(_GObject*) (JSCContext.cpp:153) ==449866== by 0x53C25F5: g_object_new_internal (gobject.c:1867) ==449866== by 0x53C3B04: g_object_new_with_properties (gobject.c:1995) ==449866== by 0x53C46B0: g_object_new (gobject.c:1667) ==449866== by 0x967EF29: jsc_context_new (JSCContext.cpp:596) ==449866== by 0x6974F54: getOrCreateContext (WebKitJavascriptResultPrivate.h:44) ==449866== by 0x6974F54: _WebKitJavascriptResult (WebKitJavascriptResult.cpp:31) ==449866== by 0x6974F54: webkitJavascriptResultCreate(WebCore::SerializedScriptValue&) (WebKitJavascriptResult.cpp:45) ==449866== Address 0x1ffeffdb68 is on thread 1's stack ==449866== 464 bytes below stack pointer So, I know the stack grows down, and therefore this shouldn't be causing any harm because the zeroed memory should not be used by any current stack frame. That said: valgrind spam like this makes it impractical to debug serious memory safety problems and detect actual security bugs, so we need to avoid triggering this warning somehow. And valgrind suppressions are not an OK answer; nobody ever uses those. Does JSC *really* need to write below the stack pointer? When did we start doing this? (Early last year? The warnings did not occur before last year.) Ideally we would stay within our own stack frame and not trigger a spam of serious-looking warnings like this. It's seriously weird.... My first thought was that sanitizeStackForVMImpl could use alloca() as a workaround, because that should be basically zero-cost, right? But alloca() really just moves the stack pointer. I don't understand llint asm (or any asm) but I guess adjusting sp should probably suffice to avoid the warnings... right? I think the easiest solution to suppress this is just changing sp in sanitizeStackForVMImpl. Created attachment 393948 [details]
Patch
Created attachment 393950 [details]
Patch
Comment on attachment 393950 [details]
Patch
r=me
I gave the patch a try and it seems to fix it, valgrind is happy with it. My smoke (unit) tests (not exhausting, but using javascript a lot at least) didn't show any failure, nor regression, too. Thanks a bunch, Yusuke. :) Comment on attachment 393950 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393950&action=review > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1427 > + move sp, zeroValue > + storep zeroValue, VM::m_lastStackTop[vm] Let's storep sp directly and eliminate the move sp, zeroValue. > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1428 > + move sp, vm Let's rename "vm" to be something like vmOrStartSP? Comment on attachment 393950 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393950&action=review >> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1427 >> + storep zeroValue, VM::m_lastStackTop[vm] > > Let's storep sp directly and eliminate the move sp, zeroValue. It seems that ARM64 assembler is not happy with this https://ews-build.webkit.org/#/builders/22/builds/13248... >> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1428 >> + move sp, vm > > Let's rename "vm" to be something like vmOrStartSP? Sounds nice, fixed. Committed r258717: <https://trac.webkit.org/changeset/258717> Committed r258719: <https://trac.webkit.org/changeset/258719> I can confirm it's fixed for me too. Thanks! |