Skip to content

Commit

Permalink
Stop ignoring GCC's use-after-free warnings in CSSValue::deref()
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=247120

Reviewed by Michael Catanzaro.

The use-after-free warnings originating in CSSValue::deref() when compiling with
GCC were locally ignored. The few offending places are adjusted to fix the
warnings, and the IGNORE_GCC_WARNINGS macros are removed.

The warnings are usually thrown when specific optimization passes are enabled
and end up creating a use-after-free situation. In both current cases these
warnings can be ignored by simplifying the code.

In StyleProperties.cpp, the isValueID() and isValueIDIncludingList() functions
accepting a Ref<CSSValue> reference are simplified by accepting a trivial
CSSValue reference. If a Ref<CSSValue> is passed to these functions now, there's
a conversion operator on the Ref template that will handily retrieve the
reference. This also reduces reference-counting churn.

In CSSPropertyParser.cpp, getBaselineKeyword() is similarly simplified, now
accepting a CSSValue reference that's then downcasted into the primitive value,
and the sole call site is adjusted accordingly.

* Source/WebCore/css/CSSValue.h:
(WebCore::CSSValue::deref const):
* Source/WebCore/css/StyleProperties.cpp:
(WebCore::isValueID):
(WebCore::isValueIDIncludingList):
* Source/WebCore/css/parser/CSSPropertyParser.cpp:
(WebCore::getBaselineKeyword):
(WebCore::consumeContentDistributionOverflowPosition):

Canonical link: https://commits.webkit.org/256063@main
  • Loading branch information
zdobersek committed Oct 27, 2022
1 parent d485dd0 commit 5cfc81b
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 10 deletions.
2 changes: 0 additions & 2 deletions Source/WebCore/css/CSSValue.h
Expand Up @@ -259,9 +259,7 @@ class CSSValue {

inline void CSSValue::deref() const
{
IGNORE_GCC_WARNINGS_BEGIN("use-after-free")
unsigned tempRefCount = m_refCount - refCountIncrement;
IGNORE_GCC_WARNINGS_END

if (!tempRefCount) {
IGNORE_GCC_WARNINGS_BEGIN("free-nonheap-object")
Expand Down
12 changes: 7 additions & 5 deletions Source/WebCore/css/StyleProperties.cpp
Expand Up @@ -78,22 +78,24 @@ static bool isNormalValue(const RefPtr<CSSValue>& value)
return value && value->isPrimitiveValue() && downcast<CSSPrimitiveValue>(value.get())->isValueID() && downcast<CSSPrimitiveValue>(value.get())->valueID() == CSSValueNormal;
}

static bool isValueID(const Ref<CSSValue>& value, CSSValueID id)
static bool isValueID(const CSSValue& value, CSSValueID id)
{
return value->isPrimitiveValue() && downcast<CSSPrimitiveValue>(value.get()).isValueID() && downcast<CSSPrimitiveValue>(value.get()).valueID() == id;
return value.isPrimitiveValue() && downcast<CSSPrimitiveValue>(value).isValueID() && downcast<CSSPrimitiveValue>(value).valueID() == id;
}

static bool isValueID(const RefPtr<CSSValue>& value, CSSValueID id)
{
return value && isValueID(*value, id);
}

static bool isValueIDIncludingList(const Ref<CSSValue>& value, CSSValueID id)
static bool isValueIDIncludingList(const CSSValue& value, CSSValueID id)
{
if (is<CSSValueList>(value)) {
if (downcast<CSSValueList>(value.get()).size() != 1)
auto& valueList = downcast<CSSValueList>(value);
if (valueList.size() != 1)
return false;
return isValueID(downcast<CSSValueList>(value.get()).item(0), id);
auto* item = valueList.item(0);
return item && isValueID(*item, id);
}
return isValueID(value, id);
}
Expand Down
6 changes: 3 additions & 3 deletions Source/WebCore/css/parser/CSSPropertyParser.cpp
Expand Up @@ -3018,9 +3018,9 @@ static RefPtr<CSSPrimitiveValue> consumeOverflowPositionKeyword(CSSParserTokenRa
return isOverflowKeyword(range.peek().id()) ? consumeIdent(range) : nullptr;
}

static CSSValueID getBaselineKeyword(RefPtr<CSSValue> value)
static CSSValueID getBaselineKeyword(const CSSValue& value)
{
auto& primitiveValue = downcast<CSSPrimitiveValue>(*value);
auto& primitiveValue = downcast<CSSPrimitiveValue>(value);
if (primitiveValue.pairValue()) {
ASSERT(primitiveValue.pairValue()->first()->valueID() == CSSValueLast);
ASSERT(primitiveValue.pairValue()->second()->valueID() == CSSValueBaseline);
Expand Down Expand Up @@ -3054,7 +3054,7 @@ static RefPtr<CSSValue> consumeContentDistributionOverflowPosition(CSSParserToke
RefPtr<CSSValue> baseline = consumeBaselineKeyword(range);
if (!baseline)
return nullptr;
return CSSContentDistributionValue::create(CSSValueInvalid, getBaselineKeyword(baseline), CSSValueInvalid);
return CSSContentDistributionValue::create(CSSValueInvalid, getBaselineKeyword(*baseline), CSSValueInvalid);
}

if (isContentDistributionKeyword(id))
Expand Down

0 comments on commit 5cfc81b

Please sign in to comment.