From a11ac1bedef8c7a55b7e35ec89f5bdcbfcdc5025 Mon Sep 17 00:00:00 2001 From: Tom Sepez Date: Mon, 27 Feb 2017 17:08:28 -0800 Subject: 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 Reviewed-by: dsinclair --- core/fxcrt/cfx_retain_ptr.h | 5 ++ xfa/fde/css/cfde_csscomputedstyle.cpp | 6 ++- xfa/fde/css/cfde_csscomputedstyle.h | 3 +- xfa/fde/css/cfde_cssdeclaration.cpp | 9 ++-- xfa/fde/css/cfde_cssdeclaration.h | 3 +- xfa/fde/css/cfde_cssstyleselector.cpp | 78 +++++++++++++---------------- xfa/fde/css/cfde_cssstyleselector.h | 6 +-- xfa/fde/css/cfde_cssstylesheet_unittest.cpp | 18 +++---- xfa/fde/css/cfde_cssvaluelist.cpp | 4 +- xfa/fde/css/cfde_cssvaluelist.h | 2 +- 10 files changed, 68 insertions(+), 66 deletions(-) diff --git a/core/fxcrt/cfx_retain_ptr.h b/core/fxcrt/cfx_retain_ptr.h index 62b26942ba..0267ae04cd 100644 --- a/core/fxcrt/cfx_retain_ptr.h +++ b/core/fxcrt/cfx_retain_ptr.h @@ -30,6 +30,11 @@ class CFX_RetainPtr { template CFX_RetainPtr(const CFX_RetainPtr& that) : CFX_RetainPtr(that.Get()) {} + template + CFX_RetainPtr As() const { + return CFX_RetainPtr(static_cast(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 50247420be..01872d18fc 100644 --- a/xfa/fde/css/cfde_csscomputedstyle.cpp +++ b/xfa/fde/css/cfde_csscomputedstyle.cpp @@ -33,8 +33,8 @@ int32_t CFDE_CSSComputedStyle::CountFontFamilies() const { } const CFX_WideString CFDE_CSSComputedStyle::GetFontFamily(int32_t index) const { - return static_cast( - m_InheritedData.m_pFontFamily->GetValue(index)) + return m_InheritedData.m_pFontFamily->GetValue(index) + .As() ->Value(); } @@ -180,6 +180,8 @@ CFDE_CSSComputedStyle::InheritedData::InheritedData() 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 92d832eff7..bba4ccbcd4 100644 --- a/xfa/fde/css/cfde_csscomputedstyle.h +++ b/xfa/fde/css/cfde_csscomputedstyle.h @@ -21,11 +21,12 @@ class CFDE_CSSComputedStyle : public CFX_Retainable { class InheritedData { public: InheritedData(); + ~InheritedData(); FDE_CSSLength m_LetterSpacing; FDE_CSSLength m_WordSpacing; FDE_CSSLength m_TextIndent; - CFDE_CSSValueList* m_pFontFamily; + CFX_RetainPtr 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 481a31e423..3c776ca771 100644 --- a/xfa/fde/css/cfde_cssdeclaration.cpp +++ b/xfa/fde/css/cfde_cssdeclaration.cpp @@ -133,12 +133,13 @@ CFDE_CSSDeclaration::CFDE_CSSDeclaration() {} CFDE_CSSDeclaration::~CFDE_CSSDeclaration() {} -CFDE_CSSValue* CFDE_CSSDeclaration::GetProperty(FDE_CSSProperty eProperty, - bool& bImportant) const { +CFX_RetainPtr 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 7d61675bea..eb287308f8 100644 --- a/xfa/fde/css/cfde_cssdeclaration.h +++ b/xfa/fde/css/cfde_cssdeclaration.h @@ -34,7 +34,8 @@ class CFDE_CSSDeclaration { CFDE_CSSDeclaration(); ~CFDE_CSSDeclaration(); - CFDE_CSSValue* GetProperty(FDE_CSSProperty eProperty, bool& bImportant) const; + CFX_RetainPtr 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 1319a64a84..5a7aa1b7ff 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 -T* ToValue(CFDE_CSSValue* val) { - return static_cast(val); -} - -template -const T* ToValue(const CFDE_CSSValue* val) { - return static_cast(val); -} - -} // namespace - CFDE_CSSStyleSelector::CFDE_CSSStyleSelector(CFGAS_FontMgr* pFontMgr) : m_pFontMgr(pFontMgr), m_fDefFontSize(12.0f) {} @@ -122,15 +108,18 @@ void CFDE_CSSStyleSelector::ApplyDeclarations( 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::AppendInlineStyle(CFDE_CSSDeclaration* pDecl, void CFDE_CSSStyleSelector::ApplyProperty( FDE_CSSProperty eProperty, - CFDE_CSSValue* pValue, + const CFX_RetainPtr& pValue, CFDE_CSSComputedStyle* pComputedStyle) { if (pValue->GetType() != FDE_CSSPrimitiveType::List) { FDE_CSSPrimitiveType eType = pValue->GetType(); @@ -192,21 +181,22 @@ void CFDE_CSSStyleSelector::ApplyProperty( case FDE_CSSProperty::Display: if (eType == FDE_CSSPrimitiveType::Enum) { pComputedStyle->m_NonInheritedData.m_eDisplay = - ToDisplay(ToValue(pValue)->Value()); + ToDisplay(pValue.As()->Value()); } break; case FDE_CSSProperty::FontSize: { FX_FLOAT& fFontSize = pComputedStyle->m_InheritedData.m_fFontSize; if (eType == FDE_CSSPrimitiveType::Number) { - fFontSize = ToValue(pValue)->Apply(fFontSize); + fFontSize = pValue.As()->Apply(fFontSize); } else if (eType == FDE_CSSPrimitiveType::Enum) { - fFontSize = ToFontSize(ToValue(pValue)->Value(), - fFontSize); + fFontSize = + ToFontSize(pValue.As()->Value(), fFontSize); } } break; case FDE_CSSProperty::LineHeight: if (eType == FDE_CSSPrimitiveType::Number) { - const CFDE_CSSNumberValue* v = ToValue(pValue); + CFX_RetainPtr v = + pValue.As(); if (v->Kind() == FDE_CSSNumberType::Number) { pComputedStyle->m_InheritedData.m_fLineHeight = v->Value() * pComputedStyle->m_InheritedData.m_fFontSize; @@ -219,7 +209,7 @@ void CFDE_CSSStyleSelector::ApplyProperty( case FDE_CSSProperty::TextAlign: if (eType == FDE_CSSPrimitiveType::Enum) { pComputedStyle->m_InheritedData.m_eTextAlign = - ToTextAlign(ToValue(pValue)->Value()); + ToTextAlign(pValue.As()->Value()); } break; case FDE_CSSProperty::TextIndent: @@ -230,10 +220,10 @@ void CFDE_CSSStyleSelector::ApplyProperty( case FDE_CSSProperty::FontWeight: if (eType == FDE_CSSPrimitiveType::Enum) { pComputedStyle->m_InheritedData.m_wFontWeight = - ToFontWeight(ToValue(pValue)->Value()); + ToFontWeight(pValue.As()->Value()); } else if (eType == FDE_CSSPrimitiveType::Number) { int32_t iValue = - (int32_t)ToValue(pValue)->Value() / 100; + (int32_t)pValue.As()->Value() / 100; if (iValue >= 1 && iValue <= 9) { pComputedStyle->m_InheritedData.m_wFontWeight = iValue * 100; } @@ -242,13 +232,13 @@ void CFDE_CSSStyleSelector::ApplyProperty( case FDE_CSSProperty::FontStyle: if (eType == FDE_CSSPrimitiveType::Enum) { pComputedStyle->m_InheritedData.m_eFontStyle = - ToFontStyle(ToValue(pValue)->Value()); + ToFontStyle(pValue.As()->Value()); } break; case FDE_CSSProperty::Color: if (eType == FDE_CSSPrimitiveType::RGB) { pComputedStyle->m_InheritedData.m_dwFontColor = - ToValue(pValue)->Value(); + pValue.As()->Value(); } break; case FDE_CSSProperty::MarginLeft: @@ -338,19 +328,19 @@ void CFDE_CSSStyleSelector::ApplyProperty( case FDE_CSSProperty::VerticalAlign: if (eType == FDE_CSSPrimitiveType::Enum) { pComputedStyle->m_NonInheritedData.m_eVerticalAlign = - ToVerticalAlign(ToValue(pValue)->Value()); + ToVerticalAlign(pValue.As()->Value()); } else if (eType == FDE_CSSPrimitiveType::Number) { pComputedStyle->m_NonInheritedData.m_eVerticalAlign = FDE_CSSVerticalAlign::Number; pComputedStyle->m_NonInheritedData.m_fVerticalAlign = - ToValue(pValue)->Apply( + pValue.As()->Apply( pComputedStyle->m_InheritedData.m_fFontSize); } break; case FDE_CSSProperty::FontVariant: if (eType == FDE_CSSPrimitiveType::Enum) { pComputedStyle->m_InheritedData.m_eFontVariant = - ToFontVariant(ToValue(pValue)->Value()); + ToFontVariant(pValue.As()->Value()); } break; case FDE_CSSProperty::LetterSpacing: @@ -358,7 +348,7 @@ void CFDE_CSSStyleSelector::ApplyProperty( pComputedStyle->m_InheritedData.m_LetterSpacing.Set( FDE_CSSLengthUnit::Normal); } else if (eType == FDE_CSSPrimitiveType::Number) { - if (ToValue(pValue)->Kind() == + if (pValue.As()->Kind() == FDE_CSSNumberType::Percent) { break; } @@ -373,7 +363,7 @@ void CFDE_CSSStyleSelector::ApplyProperty( pComputedStyle->m_InheritedData.m_WordSpacing.Set( FDE_CSSLengthUnit::Normal); } else if (eType == FDE_CSSPrimitiveType::Number) { - if (ToValue(pValue)->Kind() == + if (pValue.As()->Kind() == FDE_CSSNumberType::Percent) { break; } @@ -406,7 +396,7 @@ void CFDE_CSSStyleSelector::ApplyProperty( break; } } else if (pValue->GetType() == FDE_CSSPrimitiveType::List) { - CFDE_CSSValueList* pList = ToValue(pValue); + CFX_RetainPtr pList = pValue.As(); int32_t iCount = pList->CountValues(); if (iCount > 0) { switch (eProperty) { @@ -484,15 +474,16 @@ FDE_CSSFontStyle CFDE_CSSStyleSelector::ToFontStyle( } } -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& pValue, + FX_FLOAT fFontSize) { if (eType == FDE_CSSPrimitiveType::Number) { - const CFDE_CSSNumberValue* v = ToValue(pValue); + CFX_RetainPtr v = pValue.As(); if (v->Kind() == FDE_CSSNumberType::Percent) { width.Set(FDE_CSSLengthUnit::Percent, - ToValue(pValue)->Value() / 100.0f); + pValue.As()->Value() / 100.0f); return width.NonZero(); } @@ -500,7 +491,7 @@ bool CFDE_CSSStyleSelector::SetLengthWithPercent(FDE_CSSLength& width, width.Set(FDE_CSSLengthUnit::Point, fValue); return width.NonZero(); } else if (eType == FDE_CSSPrimitiveType::Enum) { - switch (ToValue(pValue)->Value()) { + switch (pValue.As()->Value()) { case FDE_CSSPropertyValue::Auto: width.Set(FDE_CSSLengthUnit::Auto); return true; @@ -572,14 +563,15 @@ FDE_CSSVerticalAlign CFDE_CSSStyleSelector::ToVerticalAlign( } } -uint32_t CFDE_CSSStyleSelector::ToTextDecoration(CFDE_CSSValueList* pValue) { +uint32_t CFDE_CSSStyleSelector::ToTextDecoration( + const CFX_RetainPtr& pValue) { uint32_t dwDecoration = 0; for (int32_t i = pValue->CountValues() - 1; i >= 0; --i) { - CFDE_CSSValue* pVal = pValue->GetValue(i); + const CFX_RetainPtr pVal = pValue->GetValue(i); if (pVal->GetType() != FDE_CSSPrimitiveType::Enum) continue; - switch (ToValue(pVal)->Value()) { + switch (pVal.As()->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 b4eaa685ae..c7b6b4164a 100644 --- a/xfa/fde/css/cfde_cssstyleselector.h +++ b/xfa/fde/css/cfde_cssstyleselector.h @@ -58,7 +58,7 @@ class CFDE_CSSStyleSelector { const CFDE_CSSDeclaration* extraDecl, CFDE_CSSComputedStyle* pDestStyle); void ApplyProperty(FDE_CSSProperty eProperty, - CFDE_CSSValue* pValue, + const CFX_RetainPtr& pValue, CFDE_CSSComputedStyle* pComputedStyle); void ExtractValues(const CFDE_CSSDeclaration* decl, std::vector* importants, @@ -67,7 +67,7 @@ class CFDE_CSSStyleSelector { bool SetLengthWithPercent(FDE_CSSLength& width, FDE_CSSPrimitiveType eType, - CFDE_CSSValue* pValue, + const CFX_RetainPtr& pValue, FX_FLOAT fFontSize); FX_FLOAT ToFontSize(FDE_CSSPropertyValue eValue, FX_FLOAT fCurFontSize); FDE_CSSDisplay ToDisplay(FDE_CSSPropertyValue eValue); @@ -75,7 +75,7 @@ class CFDE_CSSStyleSelector { 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& 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 c6886428ab..fa73a7a9ba 100644 --- a/xfa/fde/css/cfde_cssstylesheet_unittest.cpp +++ b/xfa/fde/css/cfde_cssstylesheet_unittest.cpp @@ -51,19 +51,19 @@ class CFDE_CSSStyleSheetTest : public testing::Test { ASSERT(decl_); bool important; - CFDE_CSSValue* v = decl_->GetProperty(prop, important); + CFX_RetainPtr v = decl_->GetProperty(prop, &important); EXPECT_EQ(v->GetType(), FDE_CSSPrimitiveType::Number); - EXPECT_EQ(static_cast(v)->Kind(), type); - EXPECT_EQ(static_cast(v)->Value(), val); + EXPECT_EQ(v.As()->Kind(), type); + EXPECT_EQ(v.As()->Value(), val); } void VerifyEnum(FDE_CSSProperty prop, FDE_CSSPropertyValue val) { ASSERT(decl_); bool important; - CFDE_CSSValue* v = decl_->GetProperty(prop, important); + CFX_RetainPtr v = decl_->GetProperty(prop, &important); EXPECT_EQ(v->GetType(), FDE_CSSPrimitiveType::Enum); - EXPECT_EQ(static_cast(v)->Value(), val); + EXPECT_EQ(v.As()->Value(), val); } void VerifyList(FDE_CSSProperty prop, @@ -71,14 +71,14 @@ class CFDE_CSSStyleSheetTest : public testing::Test { ASSERT(decl_); bool important; - CFDE_CSSValue* v = decl_->GetProperty(prop, important); - CFDE_CSSValueList* list = static_cast(v); + CFX_RetainPtr list = + decl_->GetProperty(prop, &important).As(); EXPECT_EQ(list->CountValues(), pdfium::CollectionSize(values)); for (size_t i = 0; i < values.size(); i++) { - CFDE_CSSValue* val = list->GetValue(i); + CFX_RetainPtr val = list->GetValue(i); EXPECT_EQ(val->GetType(), FDE_CSSPrimitiveType::Enum); - EXPECT_EQ(static_cast(val)->Value(), values[i]); + EXPECT_EQ(val.As()->Value(), values[i]); } } diff --git a/xfa/fde/css/cfde_cssvaluelist.cpp b/xfa/fde/css/cfde_cssvaluelist.cpp index 64ffb9a174..737ffcb045 100644 --- a/xfa/fde/css/cfde_cssvaluelist.cpp +++ b/xfa/fde/css/cfde_cssvaluelist.cpp @@ -20,6 +20,6 @@ int32_t CFDE_CSSValueList::CountValues() const { return m_ppList.size(); } -CFDE_CSSValue* CFDE_CSSValueList::GetValue(int32_t index) const { - return m_ppList[index].Get(); +CFX_RetainPtr 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 117b925678..a47f8a3250 100644 --- a/xfa/fde/css/cfde_cssvaluelist.h +++ b/xfa/fde/css/cfde_cssvaluelist.h @@ -17,7 +17,7 @@ class CFDE_CSSValueList : public CFDE_CSSValue { ~CFDE_CSSValueList() override; int32_t CountValues() const; - CFDE_CSSValue* GetValue(int32_t index) const; + CFX_RetainPtr GetValue(int32_t index) const; protected: std::vector> m_ppList; -- cgit v1.2.3