Avoid crash above CFWL_ListItem::GetText()
The issue at hand is caused by a raw pointer rather than a
retained pointer in InheritedData::m_pFontFamily. But the
larger issue is that it's bad to Get() raw pointers from
these, especially when its so cheap to pass them by const
reference.
One reason to Get() a raw pointer is to aid in down-casts, so
add a helper to CFX_RetainPtr to give us downcasted retained
pointers.
BUG=pdfium:665
Change-Id: Ic8624af09664ff603de2e1fda8dbde0cf889f80d
Reviewed-on: https://pdfium-review.googlesource.com/2871
Commit-Queue: dsinclair <dsinclair@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>
diff --git a/core/fxcrt/cfx_retain_ptr.h b/core/fxcrt/cfx_retain_ptr.h
index 62b2694..0267ae0 100644
--- a/core/fxcrt/cfx_retain_ptr.h
+++ b/core/fxcrt/cfx_retain_ptr.h
@@ -30,6 +30,11 @@
template <class U>
CFX_RetainPtr(const CFX_RetainPtr<U>& that) : CFX_RetainPtr(that.Get()) {}
+ template <class U>
+ CFX_RetainPtr<U> As() const {
+ return CFX_RetainPtr<U>(static_cast<U*>(Get()));
+ }
+
void Reset(T* obj = nullptr) {
if (obj)
obj->Retain();
diff --git a/xfa/fde/css/cfde_csscomputedstyle.cpp b/xfa/fde/css/cfde_csscomputedstyle.cpp
index 5024742..01872d1 100644
--- a/xfa/fde/css/cfde_csscomputedstyle.cpp
+++ b/xfa/fde/css/cfde_csscomputedstyle.cpp
@@ -33,8 +33,8 @@
}
const CFX_WideString CFDE_CSSComputedStyle::GetFontFamily(int32_t index) const {
- return static_cast<CFDE_CSSStringValue*>(
- m_InheritedData.m_pFontFamily->GetValue(index))
+ return m_InheritedData.m_pFontFamily->GetValue(index)
+ .As<CFDE_CSSStringValue>()
->Value();
}
@@ -180,6 +180,8 @@
m_eFontStyle(FDE_CSSFontStyle::Normal),
m_eTextAlign(FDE_CSSTextAlign::Left) {}
+CFDE_CSSComputedStyle::InheritedData::~InheritedData() {}
+
CFDE_CSSComputedStyle::NonInheritedData::NonInheritedData()
: m_MarginWidth(FDE_CSSLengthUnit::Point, 0),
m_BorderWidth(FDE_CSSLengthUnit::Point, 0),
diff --git a/xfa/fde/css/cfde_csscomputedstyle.h b/xfa/fde/css/cfde_csscomputedstyle.h
index 92d832e..bba4ccb 100644
--- a/xfa/fde/css/cfde_csscomputedstyle.h
+++ b/xfa/fde/css/cfde_csscomputedstyle.h
@@ -21,11 +21,12 @@
class InheritedData {
public:
InheritedData();
+ ~InheritedData();
FDE_CSSLength m_LetterSpacing;
FDE_CSSLength m_WordSpacing;
FDE_CSSLength m_TextIndent;
- CFDE_CSSValueList* m_pFontFamily;
+ CFX_RetainPtr<CFDE_CSSValueList> m_pFontFamily;
FX_FLOAT m_fFontSize;
FX_FLOAT m_fLineHeight;
FX_ARGB m_dwFontColor;
diff --git a/xfa/fde/css/cfde_cssdeclaration.cpp b/xfa/fde/css/cfde_cssdeclaration.cpp
index 481a31e..3c776ca 100644
--- a/xfa/fde/css/cfde_cssdeclaration.cpp
+++ b/xfa/fde/css/cfde_cssdeclaration.cpp
@@ -133,12 +133,13 @@
CFDE_CSSDeclaration::~CFDE_CSSDeclaration() {}
-CFDE_CSSValue* CFDE_CSSDeclaration::GetProperty(FDE_CSSProperty eProperty,
- bool& bImportant) const {
+CFX_RetainPtr<CFDE_CSSValue> CFDE_CSSDeclaration::GetProperty(
+ FDE_CSSProperty eProperty,
+ bool* bImportant) const {
for (const auto& p : properties_) {
if (p->eProperty == eProperty) {
- bImportant = p->bImportant;
- return p->pValue.Get();
+ *bImportant = p->bImportant;
+ return p->pValue;
}
}
return nullptr;
diff --git a/xfa/fde/css/cfde_cssdeclaration.h b/xfa/fde/css/cfde_cssdeclaration.h
index 7d61675..eb28730 100644
--- a/xfa/fde/css/cfde_cssdeclaration.h
+++ b/xfa/fde/css/cfde_cssdeclaration.h
@@ -34,7 +34,8 @@
CFDE_CSSDeclaration();
~CFDE_CSSDeclaration();
- CFDE_CSSValue* GetProperty(FDE_CSSProperty eProperty, bool& bImportant) const;
+ CFX_RetainPtr<CFDE_CSSValue> GetProperty(FDE_CSSProperty eProperty,
+ bool* bImportant) const;
const_prop_iterator begin() const { return properties_.begin(); }
const_prop_iterator end() const { return properties_.end(); }
diff --git a/xfa/fde/css/cfde_cssstyleselector.cpp b/xfa/fde/css/cfde_cssstyleselector.cpp
index 1319a64..5a7aa1b 100644
--- a/xfa/fde/css/cfde_cssstyleselector.cpp
+++ b/xfa/fde/css/cfde_cssstyleselector.cpp
@@ -22,20 +22,6 @@
#include "xfa/fde/css/cfde_cssvaluelist.h"
#include "xfa/fxfa/app/cxfa_csstagprovider.h"
-namespace {
-
-template <class T>
-T* ToValue(CFDE_CSSValue* val) {
- return static_cast<T*>(val);
-}
-
-template <class T>
-const T* ToValue(const CFDE_CSSValue* val) {
- return static_cast<T*>(val);
-}
-
-} // namespace
-
CFDE_CSSStyleSelector::CFDE_CSSStyleSelector(CFGAS_FontMgr* pFontMgr)
: m_pFontMgr(pFontMgr), m_fDefFontSize(12.0f) {}
@@ -122,15 +108,18 @@
for (auto& decl : declArray)
ExtractValues(decl, &importants, &normals, &customs);
+
if (extraDecl)
ExtractValues(extraDecl, &importants, &normals, &customs);
for (auto& prop : normals)
- ApplyProperty(prop->eProperty, prop->pValue.Get(), pComputedStyle);
+ ApplyProperty(prop->eProperty, prop->pValue, pComputedStyle);
+
for (auto& prop : customs)
pComputedStyle->AddCustomStyle(*prop);
+
for (auto& prop : importants)
- ApplyProperty(prop->eProperty, prop->pValue.Get(), pComputedStyle);
+ ApplyProperty(prop->eProperty, prop->pValue, pComputedStyle);
}
void CFDE_CSSStyleSelector::ExtractValues(
@@ -184,7 +173,7 @@
void CFDE_CSSStyleSelector::ApplyProperty(
FDE_CSSProperty eProperty,
- CFDE_CSSValue* pValue,
+ const CFX_RetainPtr<CFDE_CSSValue>& pValue,
CFDE_CSSComputedStyle* pComputedStyle) {
if (pValue->GetType() != FDE_CSSPrimitiveType::List) {
FDE_CSSPrimitiveType eType = pValue->GetType();
@@ -192,21 +181,22 @@
case FDE_CSSProperty::Display:
if (eType == FDE_CSSPrimitiveType::Enum) {
pComputedStyle->m_NonInheritedData.m_eDisplay =
- ToDisplay(ToValue<CFDE_CSSEnumValue>(pValue)->Value());
+ ToDisplay(pValue.As<CFDE_CSSEnumValue>()->Value());
}
break;
case FDE_CSSProperty::FontSize: {
FX_FLOAT& fFontSize = pComputedStyle->m_InheritedData.m_fFontSize;
if (eType == FDE_CSSPrimitiveType::Number) {
- fFontSize = ToValue<CFDE_CSSNumberValue>(pValue)->Apply(fFontSize);
+ fFontSize = pValue.As<CFDE_CSSNumberValue>()->Apply(fFontSize);
} else if (eType == FDE_CSSPrimitiveType::Enum) {
- fFontSize = ToFontSize(ToValue<CFDE_CSSEnumValue>(pValue)->Value(),
- fFontSize);
+ fFontSize =
+ ToFontSize(pValue.As<CFDE_CSSEnumValue>()->Value(), fFontSize);
}
} break;
case FDE_CSSProperty::LineHeight:
if (eType == FDE_CSSPrimitiveType::Number) {
- const CFDE_CSSNumberValue* v = ToValue<CFDE_CSSNumberValue>(pValue);
+ CFX_RetainPtr<CFDE_CSSNumberValue> v =
+ pValue.As<CFDE_CSSNumberValue>();
if (v->Kind() == FDE_CSSNumberType::Number) {
pComputedStyle->m_InheritedData.m_fLineHeight =
v->Value() * pComputedStyle->m_InheritedData.m_fFontSize;
@@ -219,7 +209,7 @@
case FDE_CSSProperty::TextAlign:
if (eType == FDE_CSSPrimitiveType::Enum) {
pComputedStyle->m_InheritedData.m_eTextAlign =
- ToTextAlign(ToValue<CFDE_CSSEnumValue>(pValue)->Value());
+ ToTextAlign(pValue.As<CFDE_CSSEnumValue>()->Value());
}
break;
case FDE_CSSProperty::TextIndent:
@@ -230,10 +220,10 @@
case FDE_CSSProperty::FontWeight:
if (eType == FDE_CSSPrimitiveType::Enum) {
pComputedStyle->m_InheritedData.m_wFontWeight =
- ToFontWeight(ToValue<CFDE_CSSEnumValue>(pValue)->Value());
+ ToFontWeight(pValue.As<CFDE_CSSEnumValue>()->Value());
} else if (eType == FDE_CSSPrimitiveType::Number) {
int32_t iValue =
- (int32_t)ToValue<CFDE_CSSNumberValue>(pValue)->Value() / 100;
+ (int32_t)pValue.As<CFDE_CSSNumberValue>()->Value() / 100;
if (iValue >= 1 && iValue <= 9) {
pComputedStyle->m_InheritedData.m_wFontWeight = iValue * 100;
}
@@ -242,13 +232,13 @@
case FDE_CSSProperty::FontStyle:
if (eType == FDE_CSSPrimitiveType::Enum) {
pComputedStyle->m_InheritedData.m_eFontStyle =
- ToFontStyle(ToValue<CFDE_CSSEnumValue>(pValue)->Value());
+ ToFontStyle(pValue.As<CFDE_CSSEnumValue>()->Value());
}
break;
case FDE_CSSProperty::Color:
if (eType == FDE_CSSPrimitiveType::RGB) {
pComputedStyle->m_InheritedData.m_dwFontColor =
- ToValue<CFDE_CSSColorValue>(pValue)->Value();
+ pValue.As<CFDE_CSSColorValue>()->Value();
}
break;
case FDE_CSSProperty::MarginLeft:
@@ -338,19 +328,19 @@
case FDE_CSSProperty::VerticalAlign:
if (eType == FDE_CSSPrimitiveType::Enum) {
pComputedStyle->m_NonInheritedData.m_eVerticalAlign =
- ToVerticalAlign(ToValue<CFDE_CSSEnumValue>(pValue)->Value());
+ ToVerticalAlign(pValue.As<CFDE_CSSEnumValue>()->Value());
} else if (eType == FDE_CSSPrimitiveType::Number) {
pComputedStyle->m_NonInheritedData.m_eVerticalAlign =
FDE_CSSVerticalAlign::Number;
pComputedStyle->m_NonInheritedData.m_fVerticalAlign =
- ToValue<CFDE_CSSNumberValue>(pValue)->Apply(
+ pValue.As<CFDE_CSSNumberValue>()->Apply(
pComputedStyle->m_InheritedData.m_fFontSize);
}
break;
case FDE_CSSProperty::FontVariant:
if (eType == FDE_CSSPrimitiveType::Enum) {
pComputedStyle->m_InheritedData.m_eFontVariant =
- ToFontVariant(ToValue<CFDE_CSSEnumValue>(pValue)->Value());
+ ToFontVariant(pValue.As<CFDE_CSSEnumValue>()->Value());
}
break;
case FDE_CSSProperty::LetterSpacing:
@@ -358,7 +348,7 @@
pComputedStyle->m_InheritedData.m_LetterSpacing.Set(
FDE_CSSLengthUnit::Normal);
} else if (eType == FDE_CSSPrimitiveType::Number) {
- if (ToValue<CFDE_CSSNumberValue>(pValue)->Kind() ==
+ if (pValue.As<CFDE_CSSNumberValue>()->Kind() ==
FDE_CSSNumberType::Percent) {
break;
}
@@ -373,7 +363,7 @@
pComputedStyle->m_InheritedData.m_WordSpacing.Set(
FDE_CSSLengthUnit::Normal);
} else if (eType == FDE_CSSPrimitiveType::Number) {
- if (ToValue<CFDE_CSSNumberValue>(pValue)->Kind() ==
+ if (pValue.As<CFDE_CSSNumberValue>()->Kind() ==
FDE_CSSNumberType::Percent) {
break;
}
@@ -406,7 +396,7 @@
break;
}
} else if (pValue->GetType() == FDE_CSSPrimitiveType::List) {
- CFDE_CSSValueList* pList = ToValue<CFDE_CSSValueList>(pValue);
+ CFX_RetainPtr<CFDE_CSSValueList> pList = pValue.As<CFDE_CSSValueList>();
int32_t iCount = pList->CountValues();
if (iCount > 0) {
switch (eProperty) {
@@ -484,15 +474,16 @@
}
}
-bool CFDE_CSSStyleSelector::SetLengthWithPercent(FDE_CSSLength& width,
- FDE_CSSPrimitiveType eType,
- CFDE_CSSValue* pValue,
- FX_FLOAT fFontSize) {
+bool CFDE_CSSStyleSelector::SetLengthWithPercent(
+ FDE_CSSLength& width,
+ FDE_CSSPrimitiveType eType,
+ const CFX_RetainPtr<CFDE_CSSValue>& pValue,
+ FX_FLOAT fFontSize) {
if (eType == FDE_CSSPrimitiveType::Number) {
- const CFDE_CSSNumberValue* v = ToValue<CFDE_CSSNumberValue>(pValue);
+ CFX_RetainPtr<CFDE_CSSNumberValue> v = pValue.As<CFDE_CSSNumberValue>();
if (v->Kind() == FDE_CSSNumberType::Percent) {
width.Set(FDE_CSSLengthUnit::Percent,
- ToValue<CFDE_CSSNumberValue>(pValue)->Value() / 100.0f);
+ pValue.As<CFDE_CSSNumberValue>()->Value() / 100.0f);
return width.NonZero();
}
@@ -500,7 +491,7 @@
width.Set(FDE_CSSLengthUnit::Point, fValue);
return width.NonZero();
} else if (eType == FDE_CSSPrimitiveType::Enum) {
- switch (ToValue<CFDE_CSSEnumValue>(pValue)->Value()) {
+ switch (pValue.As<CFDE_CSSEnumValue>()->Value()) {
case FDE_CSSPropertyValue::Auto:
width.Set(FDE_CSSLengthUnit::Auto);
return true;
@@ -572,14 +563,15 @@
}
}
-uint32_t CFDE_CSSStyleSelector::ToTextDecoration(CFDE_CSSValueList* pValue) {
+uint32_t CFDE_CSSStyleSelector::ToTextDecoration(
+ const CFX_RetainPtr<CFDE_CSSValueList>& pValue) {
uint32_t dwDecoration = 0;
for (int32_t i = pValue->CountValues() - 1; i >= 0; --i) {
- CFDE_CSSValue* pVal = pValue->GetValue(i);
+ const CFX_RetainPtr<CFDE_CSSValue> pVal = pValue->GetValue(i);
if (pVal->GetType() != FDE_CSSPrimitiveType::Enum)
continue;
- switch (ToValue<CFDE_CSSEnumValue>(pVal)->Value()) {
+ switch (pVal.As<CFDE_CSSEnumValue>()->Value()) {
case FDE_CSSPropertyValue::Underline:
dwDecoration |= FDE_CSSTEXTDECORATION_Underline;
break;
diff --git a/xfa/fde/css/cfde_cssstyleselector.h b/xfa/fde/css/cfde_cssstyleselector.h
index b4eaa68..c7b6b41 100644
--- a/xfa/fde/css/cfde_cssstyleselector.h
+++ b/xfa/fde/css/cfde_cssstyleselector.h
@@ -58,7 +58,7 @@
const CFDE_CSSDeclaration* extraDecl,
CFDE_CSSComputedStyle* pDestStyle);
void ApplyProperty(FDE_CSSProperty eProperty,
- CFDE_CSSValue* pValue,
+ const CFX_RetainPtr<CFDE_CSSValue>& pValue,
CFDE_CSSComputedStyle* pComputedStyle);
void ExtractValues(const CFDE_CSSDeclaration* decl,
std::vector<const CFDE_CSSPropertyHolder*>* importants,
@@ -67,7 +67,7 @@
bool SetLengthWithPercent(FDE_CSSLength& width,
FDE_CSSPrimitiveType eType,
- CFDE_CSSValue* pValue,
+ const CFX_RetainPtr<CFDE_CSSValue>& pValue,
FX_FLOAT fFontSize);
FX_FLOAT ToFontSize(FDE_CSSPropertyValue eValue, FX_FLOAT fCurFontSize);
FDE_CSSDisplay ToDisplay(FDE_CSSPropertyValue eValue);
@@ -75,7 +75,7 @@
uint16_t ToFontWeight(FDE_CSSPropertyValue eValue);
FDE_CSSFontStyle ToFontStyle(FDE_CSSPropertyValue eValue);
FDE_CSSVerticalAlign ToVerticalAlign(FDE_CSSPropertyValue eValue);
- uint32_t ToTextDecoration(CFDE_CSSValueList* pList);
+ uint32_t ToTextDecoration(const CFX_RetainPtr<CFDE_CSSValueList>& pList);
FDE_CSSFontVariant ToFontVariant(FDE_CSSPropertyValue eValue);
CFGAS_FontMgr* const m_pFontMgr;
diff --git a/xfa/fde/css/cfde_cssstylesheet_unittest.cpp b/xfa/fde/css/cfde_cssstylesheet_unittest.cpp
index c688642..fa73a7a 100644
--- a/xfa/fde/css/cfde_cssstylesheet_unittest.cpp
+++ b/xfa/fde/css/cfde_cssstylesheet_unittest.cpp
@@ -51,19 +51,19 @@
ASSERT(decl_);
bool important;
- CFDE_CSSValue* v = decl_->GetProperty(prop, important);
+ CFX_RetainPtr<CFDE_CSSValue> v = decl_->GetProperty(prop, &important);
EXPECT_EQ(v->GetType(), FDE_CSSPrimitiveType::Number);
- EXPECT_EQ(static_cast<CFDE_CSSNumberValue*>(v)->Kind(), type);
- EXPECT_EQ(static_cast<CFDE_CSSNumberValue*>(v)->Value(), val);
+ EXPECT_EQ(v.As<CFDE_CSSNumberValue>()->Kind(), type);
+ EXPECT_EQ(v.As<CFDE_CSSNumberValue>()->Value(), val);
}
void VerifyEnum(FDE_CSSProperty prop, FDE_CSSPropertyValue val) {
ASSERT(decl_);
bool important;
- CFDE_CSSValue* v = decl_->GetProperty(prop, important);
+ CFX_RetainPtr<CFDE_CSSValue> v = decl_->GetProperty(prop, &important);
EXPECT_EQ(v->GetType(), FDE_CSSPrimitiveType::Enum);
- EXPECT_EQ(static_cast<CFDE_CSSEnumValue*>(v)->Value(), val);
+ EXPECT_EQ(v.As<CFDE_CSSEnumValue>()->Value(), val);
}
void VerifyList(FDE_CSSProperty prop,
@@ -71,14 +71,14 @@
ASSERT(decl_);
bool important;
- CFDE_CSSValue* v = decl_->GetProperty(prop, important);
- CFDE_CSSValueList* list = static_cast<CFDE_CSSValueList*>(v);
+ CFX_RetainPtr<CFDE_CSSValueList> list =
+ decl_->GetProperty(prop, &important).As<CFDE_CSSValueList>();
EXPECT_EQ(list->CountValues(), pdfium::CollectionSize<int32_t>(values));
for (size_t i = 0; i < values.size(); i++) {
- CFDE_CSSValue* val = list->GetValue(i);
+ CFX_RetainPtr<CFDE_CSSValue> val = list->GetValue(i);
EXPECT_EQ(val->GetType(), FDE_CSSPrimitiveType::Enum);
- EXPECT_EQ(static_cast<CFDE_CSSEnumValue*>(val)->Value(), values[i]);
+ EXPECT_EQ(val.As<CFDE_CSSEnumValue>()->Value(), values[i]);
}
}
diff --git a/xfa/fde/css/cfde_cssvaluelist.cpp b/xfa/fde/css/cfde_cssvaluelist.cpp
index 64ffb9a..737ffcb 100644
--- a/xfa/fde/css/cfde_cssvaluelist.cpp
+++ b/xfa/fde/css/cfde_cssvaluelist.cpp
@@ -20,6 +20,6 @@
return m_ppList.size();
}
-CFDE_CSSValue* CFDE_CSSValueList::GetValue(int32_t index) const {
- return m_ppList[index].Get();
+CFX_RetainPtr<CFDE_CSSValue> CFDE_CSSValueList::GetValue(int32_t index) const {
+ return m_ppList[index];
}
diff --git a/xfa/fde/css/cfde_cssvaluelist.h b/xfa/fde/css/cfde_cssvaluelist.h
index 117b925..a47f8a3 100644
--- a/xfa/fde/css/cfde_cssvaluelist.h
+++ b/xfa/fde/css/cfde_cssvaluelist.h
@@ -17,7 +17,7 @@
~CFDE_CSSValueList() override;
int32_t CountValues() const;
- CFDE_CSSValue* GetValue(int32_t index) const;
+ CFX_RetainPtr<CFDE_CSSValue> GetValue(int32_t index) const;
protected:
std::vector<CFX_RetainPtr<CFDE_CSSValue>> m_ppList;