Bug 226557 – -Warray-bounds, -Wstringop-truncation, -Wstringop-overread warnings in Packed.h

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Michael Catanzaro]

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);
      |         ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[Radar WebKit Bug Importer]

<rdar://problem/79103658>

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 431366 [details]
Patch

[EWS]

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].

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 432395 [details]
Patch

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 432610 [details]
Patch

[Michael Catanzaro]

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)....

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 434570 [details]
Patch

[Michael Catanzaro]

Ah, I need to add WebSocket.cpp to the XCode project, because that is a thing.

[Michael Catanzaro]

Created attachment 434621 [details]
Patch

[Michael Catanzaro]

Created attachment 434622 [details]
Patch

[Michael Catanzaro]

Created attachment 434624 [details]
Patch

[Michael Catanzaro]

(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.

[Michael Catanzaro]

(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. :/

[Michael Catanzaro]

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. :/

[Michael Catanzaro]

(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!

[Michael Catanzaro]

Created attachment 434985 [details]
Patch

[EWS]

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].

[Michael Catanzaro]

Reopening. When building with LTO enabled, there are more warnings here:

[5489/5749] Linking CXX executable bin/TestWebKitAPI/TestWebCore
/home/mcatanzaro/Projects/WebKit/Source/ThirdParty/ANGLE/src/compiler/translator/ParseContext.h:33: note: type name ‘sh::TParseContext’ should match type name ‘angle::pp::Tokenizer::Context’
   33 | class TParseContext : angle::NonCopyable
      | 
/home/mcatanzaro/Projects/WebKit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/Tokenizer.h:25: note: the incompatible type is defined here
   25 |     struct Context
      | 
In member function ‘get’,
    inlined from ‘__conv_op ’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WTF/Headers/wtf/Packed.h:198:48,
    inlined from ‘__conv_op ’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WTF/Headers/wtf/RefPtr.h:89:57,
    inlined from ‘isShared’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBuffer.h:93:36,
    inlined from ‘isShared’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBuffer.h:211:31,
    inlined from ‘unsharedBuffer’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBufferView.h:61:9,
    inlined from ‘send’ at /home/mcatanzaro/Projects/WebKit/Source/WebCore/Modules/websockets/WebSocket.cpp:386:52,
    inlined from ‘operator()’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WebCore/DerivedSources/JSWebSocket.cpp:537:5,
    inlined from ‘toJS’ at /home/mcatanzaro/Projects/WebKit/Source/WebCore/bindings/js/JSDOMConvertBase.h:168:18,
    inlined from ‘jsWebSocketPrototypeFunction_send2Body’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WebCore/DerivedSources/JSWebSocket.cpp:537:5,
    inlined from ‘jsWebSocketPrototypeFunction_sendOverloadDispatcher’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WebCore/DerivedSources/JSWebSocket.cpp:578:13,
    inlined from ‘call’ at /home/mcatanzaro/Projects/WebKit/Source/WebCore/bindings/js/JSDOMOperation.h:63:9,
    inlined from ‘jsWebSocketPrototypeFunction_send’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WebCore/DerivedSources/JSWebSocket.cpp:588:96:
/home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WTF/Headers/wtf/Packed.h:146:15: warning: ‘__builtin_memcpy’ reading 6 bytes from a region of size 0 [-Wstringop-overread]
  146 |         memcpy(&value, m_storage.data(), storageSize);
      |               ^

[5490/5749] Linking CXX shared library lib/libwebkit2gtk-4.1.so.0.1.0
/home/mcatanzaro/Projects/WebKit/Source/ThirdParty/ANGLE/src/compiler/translator/ParseContext.h:33: note: type name ‘sh::TParseContext’ should match type name ‘angle::pp::Tokenizer::Context’
   33 | class TParseContext : angle::NonCopyable
      | 
/home/mcatanzaro/Projects/WebKit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/Tokenizer.h:25: note: the incompatible type is defined here
   25 |     struct Context
      | 
In member function ‘get’,
    inlined from ‘__conv_op ’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WTF/Headers/wtf/Packed.h:198:48,
    inlined from ‘__conv_op ’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WTF/Headers/wtf/RefPtr.h:89:57,
    inlined from ‘isShared’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBuffer.h:93:36,
    inlined from ‘isShared’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBuffer.h:211:31,
    inlined from ‘unsharedBuffer’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBufferView.h:61:9,
    inlined from ‘send’ at /home/mcatanzaro/Projects/WebKit/Source/WebCore/Modules/websockets/WebSocket.cpp:386:52,
    inlined from ‘operator()’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WebCore/DerivedSources/JSWebSocket.cpp:537:5,
    inlined from ‘toJS’ at /home/mcatanzaro/Projects/WebKit/Source/WebCore/bindings/js/JSDOMConvertBase.h:168:18,
    inlined from ‘jsWebSocketPrototypeFunction_send2Body’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WebCore/DerivedSources/JSWebSocket.cpp:537:5,
    inlined from ‘jsWebSocketPrototypeFunction_sendOverloadDispatcher’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WebCore/DerivedSources/JSWebSocket.cpp:578:13,
    inlined from ‘call’ at /home/mcatanzaro/Projects/WebKit/Source/WebCore/bindings/js/JSDOMOperation.h:63:9,
    inlined from ‘jsWebSocketPrototypeFunction_send’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WebCore/DerivedSources/JSWebSocket.cpp:588:96:
/home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WTF/Headers/wtf/Packed.h:146:15: warning: ‘memcpy’ reading 6 bytes from a region of size 0 [-Wstringop-overread]
  146 |         memcpy(&value, m_storage.data(), storageSize);
      |               ^

I'm not sure if it is possible to suppress them only for affected source files because they are derived sources and would have to be somehow removed from the unified build. Might need to revert r240362 and suppress -Wstringop-overread globally when building with GCC, since this is yet another case where pragmas fail to suppress the warning.

[Michael Catanzaro]

I'm going to turn off -Wstringop-overread for all of WebKit when building with GCC 11 or newer.

Note this particular instance of the warning is a false-positive because we read storageSize bytes from m_storage.data(), and m_storage.data() is guaranteed to storageSize, and m_storage is a std::array of size storageSize. So this seems is an extremely simple case, where it is obvious that GCC is wrong, but we can't use the IGNORE_WARNINGS scalpel, and therefore must resort to the sledgehammer....

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #22)
> Might need to revert r240362

I meant 240362@main, which is r280778.

[Darin Adler]

In general, when we have these kinds of warning issues, I think it’s OK to first turn off the warning globally (for the particular problematic compiler, and no more) and then try to restore the warning, but without the time pressure caused by having builds failing with -Werror.

Do you think that can work? Of course we are worried that eventually we’ll have many useful warnings left turned off, but the harder we work at it, the more we can restore those warnings.

Meanwhile, we’ll still have a project that can build.

[Michael Catanzaro]

Note we don't use -Werror at all currently, but I'm trying to change that in bug #155047, so this does block the effort to enable it.

(In reply to Darin Adler from comment #25)
> In general, when we have these kinds of warning issues, I think it’s OK to
> first turn off the warning globally (for the particular problematic
> compiler, and no more)

I have a patch to turn it off specifically for GCC 11 and newer versions.

> and then try to restore the warning, but without the
> time pressure caused by having builds failing with -Werror.

Realistically, it will require changes in GCC. I've complained to GCC devs and we'll see what they say.

[Michael Catanzaro]

Created attachment 443980 [details]
Patch

[EWS]

Committed r285652 (244150@main): <https://commits.webkit.org/244150@main>

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