Bug 226193

One of these is really frustrating me. I wonder if anybody else sees a problem with this code, or whether GCC's warning is just bogus here: [643/1468] Building CXX object Source/WebCore/CMakeFiles/W...vedSources/unified-sources/UnifiedSource-4babe430-49.cpp.o In file included from WTF/Headers/wtf/text/StringImpl.h:32, from WTF/Headers/wtf/text/WTFString.h:31, from ../../Source/WebCore/dom/Exception.h:30, from ../../Source/WebCore/dom/ExceptionOr.h:29, from ../../Source/WebCore/dom/Event.h:29, from ../../Source/WebCore/Modules/websockets/CloseEvent.h:33, from ../../Source/WebCore/Modules/websockets/CloseEvent.cpp:27, from WebCore/DerivedSources/unified-sources/UnifiedSource-4babe430-49.cpp:3: In member function ‘T* WTF::PackedAlignedPtr<T, <anonymous> >::get() const [with T = JSC::SharedArrayBufferContents; long unsigned int passedAlignment = 1]’, inlined from ‘WTF::PackedAlignedPtr<T, <anonymous> >::operator bool() const [with T = JSC::SharedArrayBufferContents; long unsigned int passedAlignment = 1]’ at WTF/Headers/wtf/Packed.h:194:48, inlined from ‘WTF::RefPtr<T, <template-parameter-1-2>, <template-parameter-1-3> >::operator WTF::RefPtr<T, <template-parameter-1-2>, <template-parameter-1-3> >::UnspecifiedBoolType() const [with T = JSC::SharedArrayBufferContents; _PtrTraits = WTF::PackedPtrTraits<JSC::SharedArrayBufferContents>; _RefDerefTraits = WTF::DefaultRefDerefTraits<JSC::SharedArrayBufferContents>]’ at WTF/Headers/wtf/RefPtr.h:91:57, inlined from ‘bool JSC::ArrayBufferContents::isShared() const’ at JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBuffer.h:84:36, inlined from ‘bool JSC::ArrayBuffer::isShared() const’ at JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBuffer.h:202:31, inlined from ‘WTF::RefPtr<JSC::ArrayBuffer> JSC::ArrayBufferView::unsharedBuffer() const’ at JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBufferView.h:60:9, inlined from ‘WebCore::ExceptionOr<void> WebCore::WebSocket::send(JSC::ArrayBufferView&)’ at ../../Source/WebCore/Modules/websockets/WebSocket.cpp:390:52: WTF/Headers/wtf/Packed.h:143:15: warning: ‘void* memcpy(void*, const void*, size_t)’ offset [0, 5] is out of the bounds [0, 0] [-Warray-bounds] 143 | memcpy(static_cast<void*>(&value), static_cast<void*>(const_cast<uint8_t*>(m_storage.data())), storageSize); | ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ GCC's -Warray-bounds warning is frankly pretty crappy. It's usually a false-positive, so I normally wouldn't worry too much about suppressing it. But in this case, suppressing it introduces a -Wstringop-overread warning: [641/1468] Building CXX object Source/WebCore/CMakeFiles/W...vedSources/unified-sources/UnifiedSource-4babe430-49.cpp.o In file included from WTF/Headers/wtf/text/StringImpl.h:32, from WTF/Headers/wtf/text/WTFString.h:31, from ../../Source/WebCore/dom/Exception.h:30, from ../../Source/WebCore/dom/ExceptionOr.h:29, from ../../Source/WebCore/dom/Event.h:29, from ../../Source/WebCore/Modules/websockets/CloseEvent.h:33, from ../../Source/WebCore/Modules/websockets/CloseEvent.cpp:27, from WebCore/DerivedSources/unified-sources/UnifiedSource-4babe430-49.cpp:3: In member function ‘T* WTF::PackedAlignedPtr<T, <anonymous> >::get() const [with T = JSC::SharedArrayBufferContents; long unsigned int passedAlignment = 1]’, inlined from ‘WTF::PackedAlignedPtr<T, <anonymous> >::operator bool() const [with T = JSC::SharedArrayBufferContents; long unsigned int passedAlignment = 1]’ at WTF/Headers/wtf/Packed.h:194:48, inlined from ‘WTF::RefPtr<T, <template-parameter-1-2>, <template-parameter-1-3> >::operator WTF::RefPtr<T, <template-parameter-1-2>, <template-parameter-1-3> >::UnspecifiedBoolType() const [with T = JSC::SharedArrayBufferContents; _PtrTraits = WTF::PackedPtrTraits<JSC::SharedArrayBufferContents>; _RefDerefTraits = WTF::DefaultRefDerefTraits<JSC::SharedArrayBufferContents>]’ at WTF/Headers/wtf/RefPtr.h:91:57, inlined from ‘bool JSC::ArrayBufferContents::isShared() const’ at JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBuffer.h:84:36, inlined from ‘bool JSC::ArrayBuffer::isShared() const’ at JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBuffer.h:202:31, inlined from ‘WTF::RefPtr<JSC::ArrayBuffer> JSC::ArrayBufferView::unsharedBuffer() const’ at JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBufferView.h:60:9, inlined from ‘WebCore::ExceptionOr<void> WebCore::WebSocket::send(JSC::ArrayBufferView&)’ at ../../Source/WebCore/Modules/websockets/WebSocket.cpp:390:52: WTF/Headers/wtf/Packed.h:143:15: warning: ‘void* memcpy(void*, const void*, size_t)’ reading 6 bytes from a region of size 0 [-Wstringop-overread] 143 | memcpy(static_cast<void*>(&value), static_cast<void*>(const_cast<uint8_t*>(m_storage.data())), storageSize); | ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ And that has got me worried, because -Wstringop-overread is a good warning and I do not remember seeing a false-positive -Wstringop-overread before. But I also do not see any problems with this code. The code is not going crazy here: it seems to be careful to check the size of its buffers. Maybe I am missing something? Anyway, I assume they are both false-positives, because I don't see anything wrong, but I'm calling them out because they leave me a little nervous. (There's also a new -Wnonnull in JITCall.cpp complaining that the "this" pointer can be nullptr in JIT::compileOpCall. As usual, GCC shows no evidence for why we should believe this extraordinary claim. If it's possible, it is far from clear bbhow.)

Created attachment 429581 [details] Patch

(CC: Robin for reminder about -Wclass-memaccess. The way to suppress when you're doing something weird and intentionally want to avoid running constructors/destructors is to static_cast<void*>() when passing any object pointer to memset()/memcpy() etc.)

Ping reviewers

Comment on attachment 429581 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429581&action=review > Source/WTF/wtf/Packed.h:149 > +// https://bugs.webkit.org/show_bug.cgi?id=226193 > +IGNORE_GCC_WARNINGS_BEGIN("stringop-overread") > +IGNORE_GCC_WARNINGS_BEGIN("array-bounds") > #if CPU(LITTLE_ENDIAN) > - memcpy(&value, m_storage.data(), storageSize); > + memcpy(static_cast<void*>(&value), static_cast<void*>(const_cast<uint8_t*>(m_storage.data())), storageSize); > #else > - memcpy(bitwise_cast<uint8_t*>(&value) + (sizeof(void*) - storageSize), m_storage.data(), storageSize); > + memcpy(static_cast<void*>(bitwise_cast<uint8_t*>(&value) + (sizeof(void*) - storageSize)), static_cast<void*>(const_cast<uint8_t*>(m_storage.data())), storageSize); > #endif > +IGNORE_GCC_WARNINGS_END > +IGNORE_GCC_WARNINGS_END IMO this merits an entirely separate bug report. Let's use this bug for the easy warnings.

(In reply to Michael Catanzaro from comment #5) > IMO this merits an entirely separate bug report. Bug #226557 > Let's use this bug for the easy warnings. There are several more than last week, because of course there are.

Created attachment 430415 [details] Patch

(In reply to Michael Catanzaro from comment #3) > (CC: Robin for reminder about -Wclass-memaccess. The way to suppress when > you're doing something weird and intentionally want to avoid running > constructors/destructors is to static_cast<void*>() when passing any object > pointer to memset()/memcpy() etc.) Thanks for the tip!

Ping reviewers, let's land this please.

Comment on attachment 430415 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430415&action=review > Source/JavaScriptCore/jit/JITCall.cpp:258 > +IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_BEGIN > auto slowPaths = info->emitFastPath(*this, regT0, regT2, CallLinkInfo::UseDataIC::Yes); > +IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_END This looks not great to me. Can you bypass this warning without messing up the code?

(In reply to Yusuke Suzuki from comment #10) > This looks not great to me. Can you bypass this warning without messing up > the code? Not that I know of. We have a special suppression macro that says ERRONEOUS for a reason, unfortunately. :( The null check warning is for "this" and I assume it's bogus. If you see some way "this" can plausibly be null there, then of course that would be a disaster that we ought to fix, but GCC isn't going to give us any hints as to how it might be possible. I'll note that normally when "this" becomes invalid during a function call, it's normally a use-after-free vulnerability where "this" is left dangling because it got destroyed somewhere else by mistake. It would be *very* unusual for "this" to somehow become set to null. I'm tempted to disable -Wnonnull globally when building with GCC, because since GCC 11 it's just a bunch of false-positives.

Comment on attachment 430415 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430415&action=review >> Source/JavaScriptCore/jit/JITCall.cpp:258 >> +IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_END > > This looks not great to me. Can you bypass this warning without messing up the code? Adrian suggested adding an assert, so I tried: RELEASE_ASSERT(this) right before the call. This made it worse by introducing a long chain of -Wnonnull-compare warnings and didn't fix the original -Wnonnull. FWIW the full warning with our current unmodified code is: [3/3414] Building CXX object Source/JavaScriptCore/CMakeF...vedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp.o In file included from JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp:7: ../../Source/JavaScriptCore/jit/JITCall.cpp: In member function ‘void JSC::JIT::compileOpCall(const JSC::Instruction*, unsigned int) [with Op = JSC::OpCallEval]’: ../../Source/JavaScriptCore/jit/JITCall.cpp:256:10: warning: ‘this’ pointer is null [-Wnonnull] 256 | auto slowPaths = info->emitFastPath(*this, regT0, regT2, CallLinkInfo::UseDataIC::Yes); | ^~~~~~~~~ In file included from ../../Source/JavaScriptCore/bytecode/CodeBlock.h:34, from ../../Source/JavaScriptCore/jit/AssemblyHelpers.h:30, from ../../Source/JavaScriptCore/jit/CCallHelpers.h:30, from ../../Source/JavaScriptCore/jit/JITAddGenerator.h:30, from ../../Source/JavaScriptCore/jit/JITAddGenerator.cpp:27, from JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp:1: ../../Source/JavaScriptCore/bytecode/CallLinkInfo.h:178:30: note: in a call to non-static member function ‘JSC::AbstractMacroAssembler<JSC::X86Assembler>::JumpList JSC::CallLinkInfo::emitFastPath(JSC::CCallHelpers&, JSC::GPRReg, JSC::GPRReg, JSC::CallLinkInfo::UseDataIC)’ 178 | MacroAssembler::JumpList emitFastPath(CCallHelpers&, GPRReg calleeGPR, GPRReg callLinkInfoGPR, UseDataIC) WARN_UNUSED_RETURN; | ^~~~~~~~~~~~

Ugh sorry, it's warning that *info* might be null, it's only "this" inside the call to CallLinkInfo::emitFastPath. So confusing. :/ Let me try RELEASE_ASSERT(info) and see what happens....

Comment on attachment 430415 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430415&action=review >>> Source/JavaScriptCore/jit/JITCall.cpp:258 >>> +IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_END >> >> This looks not great to me. Can you bypass this warning without messing up the code? > > Adrian suggested adding an assert, so I tried: RELEASE_ASSERT(this) right before the call. This made it worse by introducing a long chain of -Wnonnull-compare warnings and didn't fix the original -Wnonnull. FWIW the full warning with our current unmodified code is: > > [3/3414] Building CXX object Source/JavaScriptCore/CMakeF...vedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp.o > In file included from JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp:7: > ../../Source/JavaScriptCore/jit/JITCall.cpp: In member function ‘void JSC::JIT::compileOpCall(const JSC::Instruction*, unsigned int) [with Op = JSC::OpCallEval]’: > ../../Source/JavaScriptCore/jit/JITCall.cpp:256:10: warning: ‘this’ pointer is null [-Wnonnull] > 256 | auto slowPaths = info->emitFastPath(*this, regT0, regT2, CallLinkInfo::UseDataIC::Yes); > | ^~~~~~~~~ > In file included from ../../Source/JavaScriptCore/bytecode/CodeBlock.h:34, > from ../../Source/JavaScriptCore/jit/AssemblyHelpers.h:30, > from ../../Source/JavaScriptCore/jit/CCallHelpers.h:30, > from ../../Source/JavaScriptCore/jit/JITAddGenerator.h:30, > from ../../Source/JavaScriptCore/jit/JITAddGenerator.cpp:27, > from JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp:1: > ../../Source/JavaScriptCore/bytecode/CallLinkInfo.h:178:30: note: in a call to non-static member function ‘JSC::AbstractMacroAssembler<JSC::X86Assembler>::JumpList JSC::CallLinkInfo::emitFastPath(JSC::CCallHelpers&, JSC::GPRReg, JSC::GPRReg, JSC::CallLinkInfo::UseDataIC)’ > 178 | MacroAssembler::JumpList emitFastPath(CCallHelpers&, GPRReg calleeGPR, GPRReg callLinkInfoGPR, UseDataIC) WARN_UNUSED_RETURN; > | ^~~~~~~~~~~~ OK, RELEASE_ASSERT() works after all. So we can do: RELEASE_ASSERT(info); auto slowPaths = info->emitFastPath(*this, regT0, regT2, CallLinkInfo::UseDataIC::Yes); Yusuke, do you prefer that to using IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_BEGIN? Now that I realize GCC is complaining about "info" rather than "this" -- it's highlighting the totally wrong portion of the line there and I claim its error message is incorrect, it should say "info" rather than "this" -- well it makes more sense why it's complaining, and I'm less-inclined to label this case erroneous. It sure does look like info can be null there. Could you look at this case please?

Comment on attachment 430415 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430415&action=review >>>> Source/JavaScriptCore/jit/JITCall.cpp:258 >>>> +IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_END >>> >>> This looks not great to me. Can you bypass this warning without messing up the code? >> >> Adrian suggested adding an assert, so I tried: RELEASE_ASSERT(this) right before the call. This made it worse by introducing a long chain of -Wnonnull-compare warnings and didn't fix the original -Wnonnull. FWIW the full warning with our current unmodified code is: >> >> [3/3414] Building CXX object Source/JavaScriptCore/CMakeF...vedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp.o >> In file included from JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp:7: >> ../../Source/JavaScriptCore/jit/JITCall.cpp: In member function ‘void JSC::JIT::compileOpCall(const JSC::Instruction*, unsigned int) [with Op = JSC::OpCallEval]’: >> ../../Source/JavaScriptCore/jit/JITCall.cpp:256:10: warning: ‘this’ pointer is null [-Wnonnull] >> 256 | auto slowPaths = info->emitFastPath(*this, regT0, regT2, CallLinkInfo::UseDataIC::Yes); >> | ^~~~~~~~~ >> In file included from ../../Source/JavaScriptCore/bytecode/CodeBlock.h:34, >> from ../../Source/JavaScriptCore/jit/AssemblyHelpers.h:30, >> from ../../Source/JavaScriptCore/jit/CCallHelpers.h:30, >> from ../../Source/JavaScriptCore/jit/JITAddGenerator.h:30, >> from ../../Source/JavaScriptCore/jit/JITAddGenerator.cpp:27, >> from JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp:1: >> ../../Source/JavaScriptCore/bytecode/CallLinkInfo.h:178:30: note: in a call to non-static member function ‘JSC::AbstractMacroAssembler<JSC::X86Assembler>::JumpList JSC::CallLinkInfo::emitFastPath(JSC::CCallHelpers&, JSC::GPRReg, JSC::GPRReg, JSC::CallLinkInfo::UseDataIC)’ >> 178 | MacroAssembler::JumpList emitFastPath(CCallHelpers&, GPRReg calleeGPR, GPRReg callLinkInfoGPR, UseDataIC) WARN_UNUSED_RETURN; >> | ^~~~~~~~~~~~ > > OK, RELEASE_ASSERT() works after all. So we can do: > > RELEASE_ASSERT(info); > auto slowPaths = info->emitFastPath(*this, regT0, regT2, CallLinkInfo::UseDataIC::Yes); > > Yusuke, do you prefer that to using IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_BEGIN? > > Now that I realize GCC is complaining about "info" rather than "this" -- it's highlighting the totally wrong portion of the line there and I claim its error message is incorrect, it should say "info" rather than "this" -- well it makes more sense why it's complaining, and I'm less-inclined to label this case erroneous. It sure does look like info can be null there. Could you look at this case please? I had a suspicion that asserting that something is non-null might make the compiler assume that the value will be non-NULL afterwards — the first time I saw this kind of behavior was in the Rust compiler a few years ago, which was mind-blowing because then it also went ahead and did additional optimizations possible under the asserted assumption. C/C++ were not that smart then :] This being the only change there are doubts about, I think we can land the patch as-is sans this change so we can avoid most of the warning spam during compilation sooner; then do a follow-up to address this one. Michael, feel free to land the patch *without* this first, if you feel so inclined. By all means if you or Yusuke manage to figure out whether “info“ can be NULL (or not) here, updating the patch is also an option — sadly, I won't have time today to help with that.

Created attachment 430574 [details] Patch for landing

Committed r278458 (238478@main): <https://commits.webkit.org/238478@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 430574 [details].

There are two remaining warnings: bug #226557 and bug #226643.