From 78353d5dbc0b0c9b2d6946005439a51efa7d108c Mon Sep 17 00:00:00 2001 From: Tom Sepez Date: Fri, 19 Feb 2016 14:22:56 -0800 Subject: Use safe arithmentic in CFX_BinaryBuf::ExpandBuf. Always call ExpandBuf(), and if it returns, we know the subsequent calculations won't overflow. Also use std::unique_ptr, and fix unintentional copies thus detected by its suppressed copy ctor in fsdk_baseform.cpp Also Remove unused CFX_BinaryBuf::TakeOver(), AppendFill(), CopyData(). Also remove operator= in favor of using <<, for similarity with std::ostream and friends. Also move ByteStringC methods to CFX_ByteTextBuf sub-class. Also re-order members, may pack tighter on 64-bits. R=thestig@chromium.org Review URL: https://codereview.chromium.org/1710403002 . --- core/include/fxcrt/fx_basic.h | 110 +++++-------- core/include/fxcrt/fx_safe_types.h | 1 + .../src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp | 6 +- core/src/fxcodec/codec/fx_codec_fax.cpp | 3 +- core/src/fxcrt/fx_basic_buffer.cpp | 174 +++++++++------------ 5 files changed, 122 insertions(+), 172 deletions(-) (limited to 'core') diff --git a/core/include/fxcrt/fx_basic.h b/core/include/fxcrt/fx_basic.h index 7f47122dea..4402856996 100644 --- a/core/include/fxcrt/fx_basic.h +++ b/core/include/fxcrt/fx_basic.h @@ -8,6 +8,7 @@ #define CORE_INCLUDE_FXCRT_FX_BASIC_H_ #include +#include #include "core/include/fxcrt/fx_memory.h" #include "core/include/fxcrt/fx_stream.h" @@ -29,109 +30,90 @@ template char(&ArraySizeHelper(T(&array)[N]))[N]; +// Used with std::unique_ptr to FX_Free raw memory. +struct FxFreeDeleter { + inline void operator()(void* ptr) const { FX_Free(ptr); } +}; + +// Used with std::unique_ptr to Release() objects that can't be deleted. +template +struct ReleaseDeleter { + inline void operator()(T* ptr) const { ptr->Release(); } +}; + class CFX_BinaryBuf { public: CFX_BinaryBuf(); - CFX_BinaryBuf(FX_STRSIZE size); + explicit CFX_BinaryBuf(FX_STRSIZE size); - ~CFX_BinaryBuf(); + uint8_t* GetBuffer() const { return m_pBuffer.get(); } + FX_STRSIZE GetSize() const { return m_DataSize; } void Clear(); - void EstimateSize(FX_STRSIZE size, FX_STRSIZE alloc_step = 0); - void AppendBlock(const void* pBuf, FX_STRSIZE size); - - void AppendFill(uint8_t byte, FX_STRSIZE count); - void AppendString(const CFX_ByteStringC& str) { AppendBlock(str.GetPtr(), str.GetLength()); } - inline void AppendByte(uint8_t byte) { - if (m_AllocSize <= m_DataSize) { - ExpandBuf(1); - } - m_pBuffer[m_DataSize++] = byte; + void AppendByte(uint8_t byte) { + ExpandBuf(1); + m_pBuffer.get()[m_DataSize++] = byte; } void InsertBlock(FX_STRSIZE pos, const void* pBuf, FX_STRSIZE size); - - void AttachData(void* pBuf, FX_STRSIZE size); - - void CopyData(const void* pBuf, FX_STRSIZE size); - - void TakeOver(CFX_BinaryBuf& other); - void Delete(int start_index, int count); - uint8_t* GetBuffer() const { return m_pBuffer; } + // Takes ownership of |pBuf|. + void AttachData(uint8_t* pBuf, FX_STRSIZE size); - FX_STRSIZE GetSize() const { return m_DataSize; } - - CFX_ByteStringC GetByteString() const; - - void DetachBuffer(); + // Releases ownership of |m_pBuffer| and returns it. + uint8_t* DetachBuffer(); protected: - FX_STRSIZE m_AllocStep; - - uint8_t* m_pBuffer; - - FX_STRSIZE m_DataSize; + void ExpandBuf(FX_STRSIZE size); + FX_STRSIZE m_AllocStep; FX_STRSIZE m_AllocSize; - - void ExpandBuf(FX_STRSIZE size); + FX_STRSIZE m_DataSize; + std::unique_ptr m_pBuffer; }; + class CFX_ByteTextBuf : public CFX_BinaryBuf { public: - void operator=(const CFX_ByteStringC& str); - void AppendChar(int ch) { AppendByte((uint8_t)ch); } + FX_STRSIZE GetLength() const { return m_DataSize; } + CFX_ByteStringC GetByteString() const; CFX_ByteTextBuf& operator<<(int i); - CFX_ByteTextBuf& operator<<(FX_DWORD i); - CFX_ByteTextBuf& operator<<(double f); - CFX_ByteTextBuf& operator<<(const CFX_ByteStringC& lpsz); - CFX_ByteTextBuf& operator<<(const CFX_ByteTextBuf& buf); - - FX_STRSIZE GetLength() const { return m_DataSize; } }; + class CFX_WideTextBuf : public CFX_BinaryBuf { public: - void operator=(const FX_WCHAR* lpsz); - - void operator=(const CFX_WideStringC& str); - void AppendChar(FX_WCHAR wch); - - CFX_WideTextBuf& operator<<(int i); - - CFX_WideTextBuf& operator<<(double f); - - CFX_WideTextBuf& operator<<(const FX_WCHAR* lpsz); - - CFX_WideTextBuf& operator<<(const CFX_WideStringC& str); - CFX_WideTextBuf& operator<<(const CFX_WideString& str); - - CFX_WideTextBuf& operator<<(const CFX_WideTextBuf& buf); - FX_STRSIZE GetLength() const { return m_DataSize / sizeof(FX_WCHAR); } - - FX_WCHAR* GetBuffer() const { return (FX_WCHAR*)m_pBuffer; } + FX_WCHAR* GetBuffer() const { + return reinterpret_cast(m_pBuffer.get()); + } + CFX_WideStringC GetWideString() const; void Delete(int start_index, int count) { CFX_BinaryBuf::Delete(start_index * sizeof(FX_WCHAR), count * sizeof(FX_WCHAR)); } - CFX_WideStringC GetWideString() const; + CFX_WideTextBuf& operator<<(int i); + CFX_WideTextBuf& operator<<(double f); + CFX_WideTextBuf& operator<<(const FX_WCHAR* lpsz); + CFX_WideTextBuf& operator<<(const CFX_WideStringC& str); + CFX_WideTextBuf& operator<<(const CFX_WideString& str); + CFX_WideTextBuf& operator<<(const CFX_WideTextBuf& buf); }; + #ifdef PDF_ENABLE_XFA class CFX_ArchiveSaver { public: @@ -1031,16 +1013,6 @@ class CFX_AutoRestorer { const T m_OldValue; }; -struct FxFreeDeleter { - inline void operator()(void* ptr) const { FX_Free(ptr); } -}; - -// Used with std::unique_ptr to Release() objects that can't be deleted. -template -struct ReleaseDeleter { - inline void operator()(T* ptr) const { ptr->Release(); } -}; - #define FX_DATALIST_LENGTH 1024 template class CFX_SortListArray { diff --git a/core/include/fxcrt/fx_safe_types.h b/core/include/fxcrt/fx_safe_types.h index aec1ca7e96..6ae3ee1054 100644 --- a/core/include/fxcrt/fx_safe_types.h +++ b/core/include/fxcrt/fx_safe_types.h @@ -15,5 +15,6 @@ typedef pdfium::base::CheckedNumeric FX_SAFE_DWORD; typedef pdfium::base::CheckedNumeric FX_SAFE_INT32; typedef pdfium::base::CheckedNumeric FX_SAFE_SIZE_T; typedef pdfium::base::CheckedNumeric FX_SAFE_FILESIZE; +typedef pdfium::base::CheckedNumeric FX_SAFE_STRSIZE; #endif // CORE_INCLUDE_FXCRT_FX_SAFE_TYPES_H_ diff --git a/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp b/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp index 735cd2b38a..d52ef4fd34 100644 --- a/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp +++ b/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp @@ -1873,7 +1873,7 @@ CFX_ByteString CPDF_SyntaxParser::ReadHexString() { if (!GetNextChar(ch)) return CFX_ByteString(); - CFX_BinaryBuf buf; + CFX_ByteTextBuf buf; bool bFirst = true; uint8_t code = 0; while (1) { @@ -1886,7 +1886,7 @@ CFX_ByteString CPDF_SyntaxParser::ReadHexString() { code = val * 16; } else { code += val; - buf.AppendByte((uint8_t)code); + buf.AppendByte(code); } bFirst = !bFirst; } @@ -1895,7 +1895,7 @@ CFX_ByteString CPDF_SyntaxParser::ReadHexString() { break; } if (!bFirst) - buf.AppendByte((uint8_t)code); + buf.AppendByte(code); return buf.GetByteString(); } diff --git a/core/src/fxcodec/codec/fx_codec_fax.cpp b/core/src/fxcodec/codec/fx_codec_fax.cpp index cacbc71456..36b92f06ff 100644 --- a/core/src/fxcodec/codec/fx_codec_fax.cpp +++ b/core/src/fxcodec/codec/fx_codec_fax.cpp @@ -799,9 +799,8 @@ void CCodec_FaxEncoder::Encode(uint8_t*& dest_buf, FX_DWORD& dest_size) { if (dest_bitpos) { m_DestBuf.AppendByte(last_byte); } - dest_buf = m_DestBuf.GetBuffer(); dest_size = m_DestBuf.GetSize(); - m_DestBuf.DetachBuffer(); + dest_buf = m_DestBuf.DetachBuffer(); } FX_BOOL CCodec_FaxModule::Encode(const uint8_t* src_buf, int width, diff --git a/core/src/fxcrt/fx_basic_buffer.cpp b/core/src/fxcrt/fx_basic_buffer.cpp index 4ef86bbf41..0551ef04de 100644 --- a/core/src/fxcrt/fx_basic_buffer.cpp +++ b/core/src/fxcrt/fx_basic_buffer.cpp @@ -5,208 +5,186 @@ // Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com #include +#include #include "core/include/fxcrt/fx_basic.h" +#include "core/include/fxcrt/fx_safe_types.h" -FX_STRSIZE FX_ftoa(FX_FLOAT f, FX_CHAR* buf); CFX_BinaryBuf::CFX_BinaryBuf() - : m_AllocStep(0), m_pBuffer(NULL), m_DataSize(0), m_AllocSize(0) {} + : m_AllocStep(0), m_AllocSize(0), m_DataSize(0) {} + CFX_BinaryBuf::CFX_BinaryBuf(FX_STRSIZE size) - : m_AllocStep(0), m_DataSize(size), m_AllocSize(size) { - m_pBuffer = FX_Alloc(uint8_t, size); -} -CFX_BinaryBuf::~CFX_BinaryBuf() { - FX_Free(m_pBuffer); + : m_AllocStep(0), m_AllocSize(size), m_DataSize(size) { + m_pBuffer.reset(FX_Alloc(uint8_t, size)); } + void CFX_BinaryBuf::Delete(int start_index, int count) { - if (!m_pBuffer || start_index < 0 || start_index + count > m_DataSize) { + if (!m_pBuffer || start_index < 0 || count < 0 || count > m_DataSize || + start_index > m_DataSize - count) { return; } - FXSYS_memmove(m_pBuffer + start_index, m_pBuffer + start_index + count, + FXSYS_memmove(m_pBuffer.get() + start_index, + m_pBuffer.get() + start_index + count, m_DataSize - start_index - count); m_DataSize -= count; } + void CFX_BinaryBuf::Clear() { m_DataSize = 0; } -void CFX_BinaryBuf::DetachBuffer() { + +uint8_t* CFX_BinaryBuf::DetachBuffer() { m_DataSize = 0; - m_pBuffer = NULL; m_AllocSize = 0; + return m_pBuffer.release(); } -void CFX_BinaryBuf::AttachData(void* buffer, FX_STRSIZE size) { - FX_Free(m_pBuffer); + +void CFX_BinaryBuf::AttachData(uint8_t* buffer, FX_STRSIZE size) { + m_pBuffer.reset(buffer); m_DataSize = size; - m_pBuffer = (uint8_t*)buffer; m_AllocSize = size; } -void CFX_BinaryBuf::TakeOver(CFX_BinaryBuf& other) { - AttachData(other.GetBuffer(), other.GetSize()); - other.DetachBuffer(); -} + void CFX_BinaryBuf::EstimateSize(FX_STRSIZE size, FX_STRSIZE step) { m_AllocStep = step; - if (m_AllocSize >= size) { - return; - } - ExpandBuf(size - m_DataSize); + if (m_AllocSize < size) + ExpandBuf(size - m_DataSize); } + void CFX_BinaryBuf::ExpandBuf(FX_STRSIZE add_size) { - FX_STRSIZE new_size = add_size + m_DataSize; - if (m_AllocSize >= new_size) { + FX_SAFE_STRSIZE new_size = m_DataSize; + new_size += add_size; + if (m_AllocSize >= new_size.ValueOrDie()) return; - } - int alloc_step; - if (m_AllocStep == 0) { - alloc_step = m_AllocSize / 4; - if (alloc_step < 128) { - alloc_step = 128; - } - } else { - alloc_step = m_AllocStep; - } - new_size = (new_size + alloc_step - 1) / alloc_step * alloc_step; - uint8_t* pNewBuffer = m_pBuffer; - if (pNewBuffer) { - pNewBuffer = FX_Realloc(uint8_t, m_pBuffer, new_size); - } else { - pNewBuffer = FX_Alloc(uint8_t, new_size); - } - m_pBuffer = pNewBuffer; - m_AllocSize = new_size; -} -void CFX_BinaryBuf::CopyData(const void* pStr, FX_STRSIZE size) { - if (size == 0) { - m_DataSize = 0; - return; - } - if (m_AllocSize < size) { - ExpandBuf(size - m_DataSize); - } - if (!m_pBuffer) { - return; - } - FXSYS_memcpy(m_pBuffer, pStr, size); - m_DataSize = size; + + int alloc_step = std::min(128, m_AllocStep ? m_AllocStep : m_AllocSize / 4); + new_size += alloc_step - 1; // Quantize, don't combine these lines. + new_size /= alloc_step; + new_size *= alloc_step; + m_AllocSize = new_size.ValueOrDie(); + m_pBuffer.reset(m_pBuffer + ? FX_Realloc(uint8_t, m_pBuffer.release(), m_AllocSize) + : FX_Alloc(uint8_t, m_AllocSize)); } + void CFX_BinaryBuf::AppendBlock(const void* pBuf, FX_STRSIZE size) { + if (size <= 0) + return; + ExpandBuf(size); - if (pBuf && m_pBuffer) { - FXSYS_memcpy(m_pBuffer + m_DataSize, pBuf, size); + if (pBuf) { + FXSYS_memcpy(m_pBuffer.get() + m_DataSize, pBuf, size); + } else { + FXSYS_memset(m_pBuffer.get() + m_DataSize, 0, size); } m_DataSize += size; } + void CFX_BinaryBuf::InsertBlock(FX_STRSIZE pos, const void* pBuf, FX_STRSIZE size) { - ExpandBuf(size); - if (!m_pBuffer) { + if (size <= 0) return; - } - FXSYS_memmove(m_pBuffer + pos + size, m_pBuffer + pos, m_DataSize - pos); + + ExpandBuf(size); + FXSYS_memmove(m_pBuffer.get() + pos + size, m_pBuffer.get() + pos, + m_DataSize - pos); if (pBuf) { - FXSYS_memcpy(m_pBuffer + pos, pBuf, size); + FXSYS_memcpy(m_pBuffer.get() + pos, pBuf, size); + } else { + FXSYS_memset(m_pBuffer.get() + pos, 0, size); } m_DataSize += size; } -void CFX_BinaryBuf::AppendFill(uint8_t byte, FX_STRSIZE count) { - ExpandBuf(count); - if (!m_pBuffer) { - return; - } - FXSYS_memset(m_pBuffer + m_DataSize, byte, count); - m_DataSize += count; -} -CFX_ByteStringC CFX_BinaryBuf::GetByteString() const { - return CFX_ByteStringC(m_pBuffer, m_DataSize); + +CFX_ByteStringC CFX_ByteTextBuf::GetByteString() const { + return CFX_ByteStringC(m_pBuffer.get(), m_DataSize); } + CFX_ByteTextBuf& CFX_ByteTextBuf::operator<<(const CFX_ByteStringC& lpsz) { AppendBlock(lpsz.GetPtr(), lpsz.GetLength()); return *this; } + CFX_ByteTextBuf& CFX_ByteTextBuf::operator<<(int i) { char buf[32]; FXSYS_itoa(i, buf, 10); AppendBlock(buf, FXSYS_strlen(buf)); return *this; } + CFX_ByteTextBuf& CFX_ByteTextBuf::operator<<(FX_DWORD i) { char buf[32]; FXSYS_itoa(i, buf, 10); AppendBlock(buf, FXSYS_strlen(buf)); return *this; } + CFX_ByteTextBuf& CFX_ByteTextBuf::operator<<(double f) { char buf[32]; FX_STRSIZE len = FX_ftoa((FX_FLOAT)f, buf); AppendBlock(buf, len); return *this; } + CFX_ByteTextBuf& CFX_ByteTextBuf::operator<<(const CFX_ByteTextBuf& buf) { - AppendBlock(buf.m_pBuffer, buf.m_DataSize); + AppendBlock(buf.m_pBuffer.get(), buf.m_DataSize); return *this; } -void CFX_ByteTextBuf::operator=(const CFX_ByteStringC& str) { - CopyData(str.GetPtr(), str.GetLength()); -} + void CFX_WideTextBuf::AppendChar(FX_WCHAR ch) { - if (m_AllocSize < m_DataSize + (FX_STRSIZE)sizeof(FX_WCHAR)) { - ExpandBuf(sizeof(FX_WCHAR)); - } - ASSERT(m_pBuffer); - *(FX_WCHAR*)(m_pBuffer + m_DataSize) = ch; + ExpandBuf(sizeof(FX_WCHAR)); + *(FX_WCHAR*)(m_pBuffer.get() + m_DataSize) = ch; m_DataSize += sizeof(FX_WCHAR); } + CFX_WideTextBuf& CFX_WideTextBuf::operator<<(const CFX_WideStringC& str) { AppendBlock(str.GetPtr(), str.GetLength() * sizeof(FX_WCHAR)); return *this; } + CFX_WideTextBuf& CFX_WideTextBuf::operator<<(const CFX_WideString& str) { AppendBlock(str.c_str(), str.GetLength() * sizeof(FX_WCHAR)); return *this; } + CFX_WideTextBuf& CFX_WideTextBuf::operator<<(int i) { char buf[32]; FXSYS_itoa(i, buf, 10); FX_STRSIZE len = FXSYS_strlen(buf); - if (m_AllocSize < m_DataSize + (FX_STRSIZE)(len * sizeof(FX_WCHAR))) { - ExpandBuf(len * sizeof(FX_WCHAR)); - } - ASSERT(m_pBuffer); - FX_WCHAR* str = (FX_WCHAR*)(m_pBuffer + m_DataSize); + ExpandBuf(len * sizeof(FX_WCHAR)); + FX_WCHAR* str = (FX_WCHAR*)(m_pBuffer.get() + m_DataSize); for (FX_STRSIZE j = 0; j < len; j++) { *str++ = buf[j]; } m_DataSize += len * sizeof(FX_WCHAR); return *this; } + CFX_WideTextBuf& CFX_WideTextBuf::operator<<(double f) { char buf[32]; FX_STRSIZE len = FX_ftoa((FX_FLOAT)f, buf); - if (m_AllocSize < m_DataSize + (FX_STRSIZE)(len * sizeof(FX_WCHAR))) { - ExpandBuf(len * sizeof(FX_WCHAR)); - } - ASSERT(m_pBuffer); - FX_WCHAR* str = (FX_WCHAR*)(m_pBuffer + m_DataSize); + ExpandBuf(len * sizeof(FX_WCHAR)); + FX_WCHAR* str = (FX_WCHAR*)(m_pBuffer.get() + m_DataSize); for (FX_STRSIZE i = 0; i < len; i++) { *str++ = buf[i]; } m_DataSize += len * sizeof(FX_WCHAR); return *this; } + CFX_WideTextBuf& CFX_WideTextBuf::operator<<(const FX_WCHAR* lpsz) { AppendBlock(lpsz, FXSYS_wcslen(lpsz) * sizeof(FX_WCHAR)); return *this; } + CFX_WideTextBuf& CFX_WideTextBuf::operator<<(const CFX_WideTextBuf& buf) { - AppendBlock(buf.m_pBuffer, buf.m_DataSize); + AppendBlock(buf.m_pBuffer.get(), buf.m_DataSize); return *this; } -void CFX_WideTextBuf::operator=(const CFX_WideStringC& str) { - CopyData(str.GetPtr(), str.GetLength() * sizeof(FX_WCHAR)); -} + CFX_WideStringC CFX_WideTextBuf::GetWideString() const { - return CFX_WideStringC((const FX_WCHAR*)m_pBuffer, + return CFX_WideStringC((const FX_WCHAR*)m_pBuffer.get(), m_DataSize / sizeof(FX_WCHAR)); } -- cgit v1.2.3