Bug 226557 - -Warray-bounds, -Wstringop-truncation warnings in Packed.h
Summary: -Warray-bounds, -Wstringop-truncation warnings in Packed.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Minor
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
Depends on: 228601 228808
Blocks: 155047
  Show dependency treegraph
 
Reported: 2021-06-02 15:41 PDT by Michael Catanzaro
Modified: 2021-08-09 08:59 PDT (History)
18 users (show)

See Also:


Attachments
Patch (5.71 KB, patch)
2021-06-14 14:31 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (1.92 KB, patch)
2021-06-28 08:15 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (3.28 KB, patch)
2021-06-30 10:57 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.41 KB, patch)
2021-07-29 14:00 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (3.26 KB, patch)
2021-07-30 07:28 PDT, Michael Catanzaro
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (4.51 KB, patch)
2021-07-30 07:36 PDT, Michael Catanzaro
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (4.51 KB, patch)
2021-07-30 07:42 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (1.83 KB, patch)
2021-08-05 08:04 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2021-06-02 15:41:34 PDT
Splitting this from bug #226193. We have a -Warray-bounds warning in Packed.h. Fortunately it's only triggered when compiling WebSocket.cpp, which is why it's not a more serious warning spam:

[506/1614] Building CXX object Source/WebCore/CMakeFiles/...edSources/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:2:
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:188: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:389:52:
WTF/Headers/wtf/Packed.h:140:15: warning: ‘void* memcpy(void*, const void*, size_t)’ offset [0, 5] is out of the bounds [0, 0] [-Warray-bounds]
  140 |         memcpy(&value, m_storage.data(), storageSize);
      |         ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Comment 1 Radar WebKit Bug Importer 2021-06-09 15:42:21 PDT
<rdar://problem/79103658>
Comment 2 Michael Catanzaro 2021-06-14 13:35:33 PDT
OK, at first I thought this was yet another false-positive, but now I see why GCC is complaining. Here is PackedAlignedPtr::get in Packed.h:

    T* get() const
    {
        // FIXME: PackedPtr<> can load memory with one mov by checking page boundary.
        // https://bugs.webkit.org/show_bug.cgi?id=197754
        uintptr_t value = 0;
#if CPU(LITTLE_ENDIAN)
        memcpy(&value, m_storage.data(), storageSize);
#else
        memcpy(bitwise_cast<uint8_t*>(&value) + (sizeof(void*) - storageSize), m_storage.data(), storageSize);
#endif
        if (isAlignmentShiftProfitable)
            value <<= alignmentShiftSize;

#if CPU(X86_64) && !(OS(DARWIN) || OS(LINUX) || OS(WINDOWS))
        // The AMD specification requires that the most significant 16
        // bits of any virtual address, bits 48 through 63, must be
        // copies of bit 47 (in a manner akin to sign extension).
        //
        // The above-named OSes will never allocate user space addresses
        // with bit 47 set, thus are already in canonical form.
        //
        // Reference: https://en.wikipedia.org/wiki/X86-64#Virtual_address_space_details
        constexpr unsigned shiftBits = countOfBits<uintptr_t> - OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH);
        value = (bitwise_cast<intptr_t>(value) << shiftBits) >> shiftBits;
#endif

        return bitwise_cast<T*>(value);
    }

The only lines that matter are here:

        uintptr_t value = 0;
#if CPU(LITTLE_ENDIAN)
        memcpy(&value, m_storage.data(), storageSize);

Here storageSize is 5, so we copy five bytes of data from m_storage.data() to &value, which the compiler expects to be an array but is actually a uintptr_t declared on the stack. So I guess the compiler is reasonable to treat one uintptr_t as a zero-length memory region and emit the warning. We know this ought to be OK for 64-bit systems because we know that uintptr_t will be 8-bytes long, so it's not really zero-length. It is clearly intentional, so the warning should be suppressed manually.

The safety depends on (a) storageSize <= sizeof(uintptr_t) -- so maybe we should add a static_assert for that --, and (b) compiler not deciding the memcpy is undefined behavior that can be "optimized" away.
Comment 3 Michael Catanzaro 2021-06-14 14:31:43 PDT
Created attachment 431366 [details]
Patch
Comment 4 EWS 2021-06-15 09:47:50 PDT
Committed r278878 (238821@main): <https://commits.webkit.org/238821@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431366 [details].
Comment 5 Michael Catanzaro 2021-06-28 08:08:19 PDT
Reopening because I forgot to suppress the -Wstringop-overread warning that appears when the -Warray-bounds warning is suppressed. The analysis in comment #2 applies just the same to this:

[2629/5494] Building CXX object Source...ources/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:1:
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:197: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:389:52:
WTF/Headers/wtf/Packed.h:147:15: warning: ‘void* memcpy(void*, const void*, size_t)’ reading 6 bytes from a region of size 0 [-Wstringop-overread]
  147 |         memcpy(&value, m_storage.data(), storageSize);
      |         ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

We know it's "safe" because storageSize <= sizeof(uintptr_t), and since r278878 we now have a static_assert to guarantee that. But the compiler not-unreasonably thinks it's bad because value is not an array.
Comment 6 Michael Catanzaro 2021-06-28 08:15:31 PDT
Created attachment 432395 [details]
Patch
Comment 7 Michael Catanzaro 2021-06-28 08:54:56 PDT
Comment on attachment 432395 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432395&action=review

> Source/WTF/wtf/Packed.h:149
> +IGNORE_WARNINGS_BEGIN("stringop-overread")

It doesn't work. GCC is just ignoring the pragma.

We might have to rewrite this code.
Comment 8 Michael Catanzaro 2021-06-30 10:57:40 PDT
Created attachment 432610 [details]
Patch
Comment 9 Michael Catanzaro 2021-06-30 12:52:36 PDT
Comment on attachment 432610 [details]
Patch

Ugh, I made a last-minute improvement that reintroduced the -Wstringop-overread warning that this patch was intended to fix. This warning is brutal. I don't understand why the usual warning pragmas are unable to suppress it. Normally they work for all warnings except those implemented by the preprocessor (cpp)....
Comment 10 Michael Catanzaro 2021-07-29 13:13:37 PDT
We'll use -Wno-array-bounds to eliminate the -Warray-bounds warning, bug #228601.

That will introduce the -Wstringop-truncation warning. The only solution I found is -Wno-stringop-truncation. So far I've tested using it globally, but since this warning does not have an excessive amount of false positives like -Warray-bounds does, I think we can safely remove WebSocket.cpp from the unified build and apply the flag only to this file.
Comment 11 Michael Catanzaro 2021-07-29 14:00:20 PDT
Created attachment 434570 [details]
Patch
Comment 12 Michael Catanzaro 2021-07-29 16:29:20 PDT
Ah, I need to add WebSocket.cpp to the XCode project, because that is a thing.
Comment 13 Michael Catanzaro 2021-07-30 07:28:33 PDT
Created attachment 434621 [details]
Patch
Comment 14 Michael Catanzaro 2021-07-30 07:36:11 PDT
Created attachment 434622 [details]
Patch
Comment 15 Michael Catanzaro 2021-07-30 07:42:07 PDT
Created attachment 434624 [details]
Patch
Comment 16 Michael Catanzaro 2021-07-30 07:42:54 PDT
(In reply to Michael Catanzaro from comment #15)
> Created attachment 434624 [details]
> Patch

If this doesn't work, I'm going to give up on editing the XCode project, and suppress -Wstringop-overread globally instead.
Comment 17 Michael Catanzaro 2021-07-30 07:46:48 PDT
(In reply to Michael Catanzaro from comment #16)
> If this doesn't work, I'm going to give up on editing the XCode project

I am defeated. :/
Comment 18 Michael Catanzaro 2021-07-30 08:01:39 PDT
Maybe an Apple developer would be willing to help with removing WebSocket.cpp from the unified build? Otherwise, I will suppress -Wstringop-truncation globally when built with GCC in bug #228601. It's very frustrating that it cannot be suppressed with pragmas. :/
Comment 19 Michael Catanzaro 2021-08-05 07:52:44 PDT
(In reply to Michael Catanzaro from comment #18)
> Maybe an Apple developer would be willing to help with removing
> WebSocket.cpp from the unified build? Otherwise, I will suppress
> -Wstringop-truncation globally when built with GCC in bug #228601. It's very
> frustrating that it cannot be suppressed with pragmas. :/

Darin handled this in bug #228808. Thanks Darin!
Comment 20 Michael Catanzaro 2021-08-05 08:04:21 PDT
Created attachment 434985 [details]
Patch
Comment 21 EWS 2021-08-09 08:59:35 PDT
Committed r280778 (240362@main): <https://commits.webkit.org/240362@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434985 [details].