From 29d447ae35101675a2a2d8bc1dcfca65de7f3929 Mon Sep 17 00:00:00 2001 From: thestig Date: Thu, 23 Jun 2016 19:57:45 -0700 Subject: Improve hint table validation checks. Check required hint table dictionary entries and make sure they: - Exist. - Are of the right type. Along the way: - Fix FX_atonum() to not have a non-const pass-by-ref param. - Simplify code in CPDF_StreamContentParser. - Make CPDF_Number::IsInteger() a const method. BUG=610555 Review-Url: https://codereview.chromium.org/2095763003 --- core/fpdfapi/fpdf_page/fpdf_page_parser.cpp | 42 ++++++++++---------- core/fpdfapi/fpdf_page/pageint.h | 2 +- core/fpdfapi/fpdf_parser/cpdf_data_avail.cpp | 41 +++++++++++++------- core/fpdfapi/fpdf_parser/cpdf_number.cpp | 13 +++---- core/fpdfapi/fpdf_parser/include/cpdf_number.h | 4 +- core/fxcrt/fx_basic_util.cpp | 53 +++++++++++++------------- core/fxcrt/include/fx_string.h | 2 +- 7 files changed, 85 insertions(+), 72 deletions(-) diff --git a/core/fpdfapi/fpdf_page/fpdf_page_parser.cpp b/core/fpdfapi/fpdf_page/fpdf_page_parser.cpp index 802f424274..0d16994bbe 100644 --- a/core/fpdfapi/fpdf_page/fpdf_page_parser.cpp +++ b/core/fpdfapi/fpdf_page/fpdf_page_parser.cpp @@ -274,35 +274,36 @@ int CPDF_StreamContentParser::GetNextParamPos() { void CPDF_StreamContentParser::AddNameParam(const FX_CHAR* name, int len) { CFX_ByteStringC bsName(name, len); - int index = GetNextParamPos(); + ContentParam& param = m_ParamBuf[GetNextParamPos()]; if (len > 32) { - m_ParamBuf[index].m_Type = ContentParam::OBJECT; - m_ParamBuf[index].m_pObject = new CPDF_Name(PDF_NameDecode(bsName)); + param.m_Type = ContentParam::OBJECT; + param.m_pObject = new CPDF_Name(PDF_NameDecode(bsName)); } else { - m_ParamBuf[index].m_Type = ContentParam::NAME; + param.m_Type = ContentParam::NAME; if (bsName.Find('#') == -1) { - FXSYS_memcpy(m_ParamBuf[index].m_Name.m_Buffer, name, len); - m_ParamBuf[index].m_Name.m_Len = len; + FXSYS_memcpy(param.m_Name.m_Buffer, name, len); + param.m_Name.m_Len = len; } else { CFX_ByteString str = PDF_NameDecode(bsName); - FXSYS_memcpy(m_ParamBuf[index].m_Name.m_Buffer, str.c_str(), - str.GetLength()); - m_ParamBuf[index].m_Name.m_Len = str.GetLength(); + FXSYS_memcpy(param.m_Name.m_Buffer, str.c_str(), str.GetLength()); + param.m_Name.m_Len = str.GetLength(); } } } void CPDF_StreamContentParser::AddNumberParam(const FX_CHAR* str, int len) { - int index = GetNextParamPos(); - m_ParamBuf[index].m_Type = ContentParam::NUMBER; - FX_atonum(CFX_ByteStringC(str, len), m_ParamBuf[index].m_Number.m_bInteger, - &m_ParamBuf[index].m_Number.m_Integer); + ContentParam& param = m_ParamBuf[GetNextParamPos()]; + param.m_Type = ContentParam::NUMBER; + param.m_Number.m_bInteger = + FX_atonum(CFX_ByteStringC(str, len), ¶m.m_Number.m_Integer); } + void CPDF_StreamContentParser::AddObjectParam(CPDF_Object* pObj) { - int index = GetNextParamPos(); - m_ParamBuf[index].m_Type = ContentParam::OBJECT; - m_ParamBuf[index].m_pObject = pObj; + ContentParam& param = m_ParamBuf[GetNextParamPos()]; + param.m_Type = ContentParam::OBJECT; + param.m_pObject = pObj; } + void CPDF_StreamContentParser::ClearAllParams() { uint32_t index = m_ParamStartPos; for (uint32_t i = 0; i < m_ParamCount; i++) { @@ -1601,14 +1602,13 @@ void CPDF_StreamContentParser::ParsePathObject() { break; } case CPDF_StreamParser::Number: { - if (nParams == 6) { + if (nParams == 6) break; - } - FX_BOOL bInteger; + int value; - FX_atonum( + bool bInteger = FX_atonum( CFX_ByteStringC(m_pSyntax->GetWordBuf(), m_pSyntax->GetWordSize()), - bInteger, &value); + &value); params[nParams++] = bInteger ? (FX_FLOAT)value : *(FX_FLOAT*)&value; break; } diff --git a/core/fpdfapi/fpdf_page/pageint.h b/core/fpdfapi/fpdf_page/pageint.h index 29fd6fb86e..08cfd0472a 100644 --- a/core/fpdfapi/fpdf_page/pageint.h +++ b/core/fpdfapi/fpdf_page/pageint.h @@ -87,7 +87,7 @@ struct ContentParam { Type m_Type; union { struct { - FX_BOOL m_bInteger; + bool m_bInteger; union { int m_Integer; FX_FLOAT m_Float; diff --git a/core/fpdfapi/fpdf_parser/cpdf_data_avail.cpp b/core/fpdfapi/fpdf_parser/cpdf_data_avail.cpp index 5a7a7be544..cc5b529b45 100644 --- a/core/fpdfapi/fpdf_parser/cpdf_data_avail.cpp +++ b/core/fpdfapi/fpdf_parser/cpdf_data_avail.cpp @@ -6,6 +6,8 @@ #include "core/fpdfapi/fpdf_parser/include/cpdf_data_avail.h" +#include + #include "core/fpdfapi/fpdf_parser/cpdf_hint_tables.h" #include "core/fpdfapi/fpdf_parser/fpdf_parser_utility.h" #include "core/fpdfapi/fpdf_parser/include/cpdf_array.h" @@ -723,37 +725,48 @@ FX_BOOL CPDF_DataAvail::CheckHintTables(DownloadHints* pHints) { return FALSE; } - if (!pDict->KeyExist("H") || !pDict->KeyExist("O") || !pDict->KeyExist("N")) { + // The actual value is not required here, but validate its existence and type. + CPDF_Number* pFirstPage = ToNumber(pDict->GetDirectObjectBy("O")); + if (!pFirstPage || !pFirstPage->IsInteger()) { + m_docStatus = PDF_DATAAVAIL_ERROR; + return FALSE; + } + + CPDF_Number* pPageCount = ToNumber(pDict->GetDirectObjectBy("N")); + if (!pPageCount || !pPageCount->IsInteger()) { m_docStatus = PDF_DATAAVAIL_ERROR; return FALSE; } - int nPageCount = pDict->GetDirectObjectBy("N")->GetInteger(); + int nPageCount = pPageCount->GetInteger(); if (nPageCount <= 1) { m_docStatus = PDF_DATAAVAIL_DONE; return TRUE; } CPDF_Array* pHintStreamRange = pDict->GetArrayBy("H"); - if (!pHintStreamRange) { + size_t nHintStreamSize = pHintStreamRange ? pHintStreamRange->GetCount() : 0; + if (nHintStreamSize != 2 && nHintStreamSize != 4) { m_docStatus = PDF_DATAAVAIL_ERROR; return FALSE; } - FX_FILESIZE szHSStart = - pHintStreamRange->GetDirectObjectAt(0) - ? pHintStreamRange->GetDirectObjectAt(0)->GetInteger() - : 0; - FX_FILESIZE szHSLength = - pHintStreamRange->GetDirectObjectAt(1) - ? pHintStreamRange->GetDirectObjectAt(1)->GetInteger() - : 0; - if (szHSStart < 0 || szHSLength <= 0) { + for (const CPDF_Object* pArrayObject : *pHintStreamRange) { + const CPDF_Number* pNumber = ToNumber(pArrayObject->GetDirect()); + if (!pNumber || !pNumber->IsInteger()) { + m_docStatus = PDF_DATAAVAIL_ERROR; + return FALSE; + } + } + + FX_FILESIZE szHintStart = pHintStreamRange->GetIntegerAt(0); + FX_FILESIZE szHintLength = pHintStreamRange->GetIntegerAt(1); + if (szHintStart < 0 || szHintLength <= 0) { m_docStatus = PDF_DATAAVAIL_ERROR; return FALSE; } - if (!IsDataAvail(szHSStart, szHSLength, pHints)) + if (!IsDataAvail(szHintStart, szHintLength, pHints)) return FALSE; m_syntaxParser.InitParser(m_pFileRead, m_dwHeaderOffset); @@ -761,7 +774,7 @@ FX_BOOL CPDF_DataAvail::CheckHintTables(DownloadHints* pHints) { std::unique_ptr pHintTables( new CPDF_HintTables(this, pDict)); std::unique_ptr> pHintStream( - ParseIndirectObjectAt(szHSStart, 0)); + ParseIndirectObjectAt(szHintStart, 0)); CPDF_Stream* pStream = ToStream(pHintStream.get()); if (pStream && pHintTables->LoadHintStream(pStream)) m_pHintTables = std::move(pHintTables); diff --git a/core/fpdfapi/fpdf_parser/cpdf_number.cpp b/core/fpdfapi/fpdf_parser/cpdf_number.cpp index a6bace57c9..5012e308ce 100644 --- a/core/fpdfapi/fpdf_parser/cpdf_number.cpp +++ b/core/fpdfapi/fpdf_parser/cpdf_number.cpp @@ -6,15 +6,14 @@ #include "core/fpdfapi/fpdf_parser/include/cpdf_number.h" -CPDF_Number::CPDF_Number() : m_bInteger(TRUE), m_Integer(0) {} +CPDF_Number::CPDF_Number() : m_bInteger(true), m_Integer(0) {} -CPDF_Number::CPDF_Number(int value) : m_bInteger(TRUE), m_Integer(value) {} +CPDF_Number::CPDF_Number(int value) : m_bInteger(true), m_Integer(value) {} -CPDF_Number::CPDF_Number(FX_FLOAT value) : m_bInteger(FALSE), m_Float(value) {} +CPDF_Number::CPDF_Number(FX_FLOAT value) : m_bInteger(false), m_Float(value) {} -CPDF_Number::CPDF_Number(const CFX_ByteStringC& str) { - FX_atonum(str, m_bInteger, &m_Integer); -} +CPDF_Number::CPDF_Number(const CFX_ByteStringC& str) + : m_bInteger(FX_atonum(str, &m_Integer)) {} CPDF_Number::~CPDF_Number() {} @@ -47,7 +46,7 @@ const CPDF_Number* CPDF_Number::AsNumber() const { } void CPDF_Number::SetString(const CFX_ByteString& str) { - FX_atonum(str.AsStringC(), m_bInteger, &m_Integer); + m_bInteger = FX_atonum(str.AsStringC(), &m_Integer); } CFX_ByteString CPDF_Number::GetString() const { diff --git a/core/fpdfapi/fpdf_parser/include/cpdf_number.h b/core/fpdfapi/fpdf_parser/include/cpdf_number.h index 7b93a992b1..eaa4009543 100644 --- a/core/fpdfapi/fpdf_parser/include/cpdf_number.h +++ b/core/fpdfapi/fpdf_parser/include/cpdf_number.h @@ -29,12 +29,12 @@ class CPDF_Number : public CPDF_Object { CPDF_Number* AsNumber() override; const CPDF_Number* AsNumber() const override; - FX_BOOL IsInteger() { return m_bInteger; } + bool IsInteger() const { return m_bInteger; } protected: ~CPDF_Number() override; - FX_BOOL m_bInteger; + bool m_bInteger; union { int m_Integer; FX_FLOAT m_Float; diff --git a/core/fxcrt/fx_basic_util.cpp b/core/fxcrt/fx_basic_util.cpp index 12d3aefd1c..abd84a864f 100644 --- a/core/fxcrt/fx_basic_util.cpp +++ b/core/fxcrt/fx_basic_util.cpp @@ -18,33 +18,34 @@ #include #include -void FX_atonum(const CFX_ByteStringC& strc, FX_BOOL& bInteger, void* pData) { - if (strc.Find('.') == -1) { - bInteger = TRUE; - int cc = 0; - pdfium::base::CheckedNumeric integer = 0; - FX_STRSIZE len = strc.GetLength(); - bool bNegative = false; - if (strc[0] == '+') { - cc++; - } else if (strc[0] == '-') { - bNegative = true; - cc++; - } - while (cc < len && std::isdigit(strc[cc])) { - integer = integer * 10 + FXSYS_toDecimalDigit(strc.CharAt(cc)); - if (!integer.IsValid()) - break; - cc++; - } - if (bNegative) { - integer = -integer; - } - *(int*)pData = integer.ValueOrDefault(0); - } else { - bInteger = FALSE; - *(FX_FLOAT*)pData = FX_atof(strc); +bool FX_atonum(const CFX_ByteStringC& strc, void* pData) { + if (strc.Find('.') != -1) { + FX_FLOAT* pFloat = static_cast(pData); + *pFloat = FX_atof(strc); + return false; + } + + int cc = 0; + pdfium::base::CheckedNumeric integer = 0; + bool bNegative = false; + if (strc[0] == '+') { + cc++; + } else if (strc[0] == '-') { + bNegative = true; + cc++; } + while (cc < strc.GetLength() && std::isdigit(strc[cc])) { + integer = integer * 10 + FXSYS_toDecimalDigit(strc.CharAt(cc)); + if (!integer.IsValid()) + break; + cc++; + } + if (bNegative) + integer = -integer; + + int* pInt = static_cast(pData); + *pInt = integer.ValueOrDefault(0); + return true; } static const FX_FLOAT fraction_scales[] = { diff --git a/core/fxcrt/include/fx_string.h b/core/fxcrt/include/fx_string.h index b0a89fd06c..c1bd82dd74 100644 --- a/core/fxcrt/include/fx_string.h +++ b/core/fxcrt/include/fx_string.h @@ -437,7 +437,7 @@ FX_FLOAT FX_atof(const CFX_ByteStringC& str); inline FX_FLOAT FX_atof(const CFX_WideStringC& wsStr) { return FX_atof(FX_UTF8Encode(wsStr.c_str(), wsStr.GetLength()).c_str()); } -void FX_atonum(const CFX_ByteStringC& str, FX_BOOL& bInteger, void* pData); +bool FX_atonum(const CFX_ByteStringC& str, void* pData); FX_STRSIZE FX_ftoa(FX_FLOAT f, FX_CHAR* buf); #endif // CORE_FXCRT_INCLUDE_FX_STRING_H_ -- cgit v1.2.3