Bug 246292 – -Wfree-nonheap-object and -Wuse-after-free from CSSValue.h

• Component: CSS
• Status: RESOLVED FIXED

[Michael Catanzaro]

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.

[Michael Catanzaro]

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;

[Darin Adler]

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.

[Darin Adler]

The flag refCountFlagIsStatic is set, and so the reference count never becomes zero.

[Darin Adler]

This is done by calling makeStatic() on the CSSValue, making that CSSValue immortal.

[Michael Catanzaro]

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

[Michael Catanzaro]

*** Bug 246294 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

Pull request: https://github.com/WebKit/WebKit/pull/5254

[EWS]

Committed 255439@main (143afebb28dd): <https://commits.webkit.org/255439@main>

Reviewed commits have been landed. Closing PR #5254 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/101084603>