Bug 246292 - -Wfree-nonheap-object and -Wuse-after-free from CSSValue.h
Summary: -Wfree-nonheap-object and -Wuse-after-free from CSSValue.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
: 246294 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-10-10 13:06 PDT by Michael Catanzaro
Modified: 2022-10-12 10:11 PDT (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2022-10-10 13:06:30 PDT
254846@main introduced a new GCC warning:

[3805/7121] Building CXX object Source/WebCore/CMakeFiles...edSources/unified-sources/UnifiedSource-2f84417a-11.cpp.o
In file included from /home/mcatanzaro/Projects/WebKit/Source/WebCore/css/CSSPrimitiveValue.h:26,
                 from /home/mcatanzaro/Projects/WebKit/Source/WebCore/css/CSSValuePool.h:28,
                 from /home/mcatanzaro/Projects/WebKit/Source/WebCore/css/CSSValuePool.cpp:27,
                 from /home/mcatanzaro/Projects/WebKit/WebKitBuild/gtk3/WebCore/DerivedSources/unified-sources/UnifiedSource-2f84417a-11.cpp:1:
In member function ‘void WebCore::CSSValue::deref() const’,
    inlined from ‘WTF::Ref<T, <template-parameter-1-2> >::~Ref() [with T = WebCore::CSSPrimitiveValue; Traits = WTF::RawPtrTraits<WebCore::CSSPrimitiveValue>]’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/gtk3/WTF/Headers/wtf/Ref.h:61:23,
    inlined from ‘WTF::Ref<WebCore::CSSValue> WebCore::valueForContainIntrinsicSize(const RenderStyle&, const ContainIntrinsicSizeType&, std::optional<Length>)’ at /home/mcatanzaro/Projects/WebKit/Source/WebCore/css/ComputedStyleExtractor.cpp:1817:5:
/home/mcatanzaro/Projects/WebKit/Source/WebCore/css/CSSValue.h:258:16: warning: ‘static void WebCore::CSSValue::operator delete(WebCore::CSSValue*, std::destroying_delete_t)’ called on unallocated object ‘WebCore::staticCSSValuePool’ [-Wfree-nonheap-object]
  258 |         delete this;


I've never seen -Wfree-nonheap-object before ever. Cool.
Comment 1 Michael Catanzaro 2022-10-10 13:07:51 PDT
Oops, there was just a little more to this warning:

/home/mcatanzaro/Projects/WebKit/Source/WebCore/css/CSSValuePool.cpp: In function ‘WTF::Ref<WebCore::CSSValue> WebCore::valueForContainIntrinsicSize(const RenderStyle&, const ContainIntrinsicSizeType&, std::optional<Length>)’:
/home/mcatanzaro/Projects/WebKit/Source/WebCore/css/CSSValuePool.cpp:37:40: note: declared here
   37 | LazyNeverDestroyed<StaticCSSValuePool> staticCSSValuePool;
Comment 2 Darin Adler 2022-10-10 13:59:59 PDT
The objects in the staticCSSValuePool have special values for the reference count that make the delete line unreachable. But the compiler can’t reason about this. Happy to supply additional details. This is a false positive.
Comment 3 Darin Adler 2022-10-10 14:05:07 PDT
The flag refCountFlagIsStatic is set, and so the reference count never becomes zero.
Comment 4 Darin Adler 2022-10-10 14:05:33 PDT
This is done by calling makeStatic() on the CSSValue, making that CSSValue immortal.
Comment 5 Michael Catanzaro 2022-10-11 14:07:27 PDT
I created a separate bug #246294 for a different issue introduced by the same commit, but I want to solve it in the same patch because it requires changes in the exact same code, so I will mark it as a duplicate now and address it here:

[3817/7121] Building CXX object Source/WebCore/CMakeFiles...edSources/unified-sources/UnifiedSource-2f84417a-18.cpp.o
In file included from /home/mcatanzaro/Projects/WebKit/Source/WebCore/css/CSSPrimitiveValue.h:26,
                 from /home/mcatanzaro/Projects/WebKit/Source/WebCore/css/parser/CSSParserToken.h:32,
                 from /home/mcatanzaro/Projects/WebKit/Source/WebCore/css/parser/CSSParserToken.cpp:31,
                 from /home/mcatanzaro/Projects/WebKit/WebKitBuild/gtk3/WebCore/DerivedSources/unified-sources/UnifiedSource-2f84417a-18.cpp:2:
In member function ‘void WebCore::CSSValue::deref() const’,
    inlined from ‘static void WTF::DefaultRefDerefTraits< <template-parameter-1-1> >::derefIfNotNull(T*) [with T = WebCore::CSSValue]’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/gtk3/WTF/Headers/wtf/RefPtr.h:42:23,
    inlined from ‘WTF::RefPtr<T, <template-parameter-1-2>, <template-parameter-1-3> >::~RefPtr() [with T = WebCore::CSSValue; _PtrTraits = WTF::RawPtrTraits<WebCore::CSSValue>; _RefDerefTraits = WTF::DefaultRefDerefTraits<WebCore::CSSValue>]’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/gtk3/WTF/Headers/wtf/RefPtr.h:74:61,
    inlined from ‘WTF::RefPtr<WebCore::CSSValue> WebCore::consumeContentDistributionOverflowPosition(CSSParserTokenRange&, IsPositionKeyword)’ at /home/mcatanzaro/Projects/WebKit/Source/WebCore/css/parser/CSSPropertyParser.cpp:3049:5:
/home/mcatanzaro/Projects/WebKit/Source/WebCore/css/CSSValue.h:256:29: warning: pointer used after ‘static void WebCore::CSSValue::operator delete(WebCore::CSSValue*, std::destroying_delete_t)’ [-Wuse-after-free]
  256 |     unsigned tempRefCount = m_refCount - refCountIncrement;
      |                             ^~~~~~~~~~
In member function ‘void WebCore::CSSValue::deref() const’,
    inlined from ‘static void WTF::DefaultRefDerefTraits< <template-parameter-1-1> >::derefIfNotNull(T*) [with T = WebCore::CSSValue]’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/gtk3/WTF/Headers/wtf/RefPtr.h:42:23,
    inlined from ‘WTF::RefPtr<T, <template-parameter-1-2>, <template-parameter-1-3> >::~RefPtr() [with T = WebCore::CSSValue; _PtrTraits = WTF::RawPtrTraits<WebCore::CSSValue>; _RefDerefTraits = WTF::DefaultRefDerefTraits<WebCore::CSSValue>]’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/gtk3/WTF/Headers/wtf/RefPtr.h:74:61,
    inlined from ‘WTF::RefPtr<WebCore::CSSValue> WebCore::consumeContentDistributionOverflowPosition(CSSParserTokenRange&, IsPositionKeyword)’ at /home/mcatanzaro/Projects/WebKit/Source/WebCore/css/parser/CSSPropertyParser.cpp:3048:51:
/home/mcatanzaro/Projects/WebKit/Source/WebCore/css/CSSValue.h:258:16: note: call to ‘static void WebCore::CSSValue::operator delete(WebCore::CSSValue*, std::destroying_delete_t)’ here
  258 |         delete this;
      |                ^~~~
Comment 6 Michael Catanzaro 2022-10-11 14:07:31 PDT
*** Bug 246294 has been marked as a duplicate of this bug. ***
Comment 7 Michael Catanzaro 2022-10-11 14:18:13 PDT
Pull request: https://github.com/WebKit/WebKit/pull/5254
Comment 8 EWS 2022-10-12 10:10:24 PDT
Committed 255439@main (143afebb28dd): <https://commits.webkit.org/255439@main>

Reviewed commits have been landed. Closing PR #5254 and removing active labels.
Comment 9 Radar WebKit Bug Importer 2022-10-12 10:11:18 PDT
<rdar://problem/101084603>