From cd5e12a9ea397b48056643a7b65126395eec3174 Mon Sep 17 00:00:00 2001 From: tsepez Date: Wed, 7 Dec 2016 13:45:41 -0800 Subject: Catch stray Retains() and Releases() outside of RetainPtr<>. The previous CLs made the code clean, so now we can mark more things private, and add friends as appropriate. Review-Url: https://codereview.chromium.org/2560783003 --- core/fxcrt/cfx_retain_ptr.h | 17 ++++++++++++++--- core/fxcrt/fx_extension.cpp | 27 +++++++++++++++------------ core/fxcrt/fx_xml_parser.cpp | 20 +++++++++++++------- xfa/fgas/crt/fgas_stream.cpp | 14 ++++++++++---- xfa/fxfa/parser/cxfa_widetextread.cpp | 2 ++ xfa/fxfa/parser/cxfa_widetextread.h | 7 ++++++- 6 files changed, 60 insertions(+), 27 deletions(-) diff --git a/core/fxcrt/cfx_retain_ptr.h b/core/fxcrt/cfx_retain_ptr.h index 8c7bf289a7..bff1b9691c 100644 --- a/core/fxcrt/cfx_retain_ptr.h +++ b/core/fxcrt/cfx_retain_ptr.h @@ -60,15 +60,24 @@ class CFX_RetainPtr { // Trivial implementation - internal ref count with virtual destructor. class CFX_Retainable { public: + bool HasOneRef() const { return m_nRefCount == 1; } + + protected: + virtual ~CFX_Retainable() {} + + private: + template + friend struct ReleaseDeleter; + + template + friend class CFX_RetainPtr; + void Retain() { ++m_nRefCount; } void Release() { if (--m_nRefCount == 0) delete this; } - bool HasOneRef() const { return m_nRefCount == 1; } - protected: - virtual ~CFX_Retainable() {} intptr_t m_nRefCount = 0; }; @@ -76,6 +85,8 @@ namespace pdfium { // Helper to make a CFX_RetainPtr along the lines of std::make_unique<>(), // or pdfium::MakeUnique<>(). Arguments are forwarded to T's constructor. +// Classes managed by CFX_RetainPtr should have protected (or private) +// constructors, and should friend this function. template CFX_RetainPtr MakeRetain(Args&&... args) { return CFX_RetainPtr(new T(std::forward(args)...)); diff --git a/core/fxcrt/fx_extension.cpp b/core/fxcrt/fx_extension.cpp index cbbb86f26b..630d4da2aa 100644 --- a/core/fxcrt/fx_extension.cpp +++ b/core/fxcrt/fx_extension.cpp @@ -74,8 +74,8 @@ bool CFX_CRTFileAccess::Init(const CFX_WideStringC& wsPath) { class CFX_CRTFileStream final : public IFX_SeekableStream { public: - explicit CFX_CRTFileStream(std::unique_ptr pFA); - ~CFX_CRTFileStream() override; + template + friend CFX_RetainPtr pdfium::MakeRetain(Args&&... args); // IFX_SeekableStream: FX_FILESIZE GetSize() override; @@ -87,6 +87,9 @@ class CFX_CRTFileStream final : public IFX_SeekableStream { bool Flush() override; private: + explicit CFX_CRTFileStream(std::unique_ptr pFA); + ~CFX_CRTFileStream() override; + std::unique_ptr m_pFile; }; @@ -133,9 +136,8 @@ bool CFX_CRTFileStream::Flush() { class CFX_MemoryStream final : public IFX_MemoryStream { public: - explicit CFX_MemoryStream(bool bConsecutive); - CFX_MemoryStream(uint8_t* pBuffer, size_t nSize, bool bTakeOver); - ~CFX_MemoryStream() override; + template + friend CFX_RetainPtr pdfium::MakeRetain(Args&&... args); // IFX_MemoryStream FX_FILESIZE GetSize() override; @@ -154,6 +156,10 @@ class CFX_MemoryStream final : public IFX_MemoryStream { void DetachBuffer() override; private: + explicit CFX_MemoryStream(bool bConsecutive); + CFX_MemoryStream(uint8_t* pBuffer, size_t nSize, bool bTakeOver); + ~CFX_MemoryStream() override; + bool ExpandBlocks(size_t size); CFX_ArrayTemplate m_Blocks; @@ -396,8 +402,7 @@ CFX_RetainPtr IFX_SeekableStream::CreateFromFilename( std::unique_ptr pFA(IFXCRT_FileAccess::Create()); if (!pFA->Open(filename, dwModes)) return nullptr; - return CFX_RetainPtr( - new CFX_CRTFileStream(std::move(pFA))); + return pdfium::MakeRetain(std::move(pFA)); } // static @@ -407,8 +412,7 @@ CFX_RetainPtr IFX_SeekableStream::CreateFromFilename( std::unique_ptr pFA(IFXCRT_FileAccess::Create()); if (!pFA->Open(filename, dwModes)) return nullptr; - return CFX_RetainPtr( - new CFX_CRTFileStream(std::move(pFA))); + return pdfium::MakeRetain(std::move(pFA)); } // static @@ -421,13 +425,12 @@ IFX_SeekableReadStream::CreateFromFilename(const FX_CHAR* filename) { CFX_RetainPtr IFX_MemoryStream::Create(uint8_t* pBuffer, size_t dwSize, bool bTakeOver) { - return CFX_RetainPtr( - new CFX_MemoryStream(pBuffer, dwSize, bTakeOver)); + return pdfium::MakeRetain(pBuffer, dwSize, bTakeOver); } // static CFX_RetainPtr IFX_MemoryStream::Create(bool bConsecutive) { - return CFX_RetainPtr(new CFX_MemoryStream(bConsecutive)); + return pdfium::MakeRetain(bConsecutive); } FX_FLOAT FXSYS_tan(FX_FLOAT a) { diff --git a/core/fxcrt/fx_xml_parser.cpp b/core/fxcrt/fx_xml_parser.cpp index ab30926387..1b562c2083 100644 --- a/core/fxcrt/fx_xml_parser.cpp +++ b/core/fxcrt/fx_xml_parser.cpp @@ -70,8 +70,8 @@ bool g_FXCRT_XML_IsNameChar(uint8_t ch) { class CXML_DataBufAcc : public IFX_BufferedReadStream { public: - CXML_DataBufAcc(const uint8_t* pBuffer, size_t size); - ~CXML_DataBufAcc() override; + template + friend CFX_RetainPtr pdfium::MakeRetain(Args&&... args); // IFX_BufferedReadStream bool IsEOF() override; @@ -83,6 +83,9 @@ class CXML_DataBufAcc : public IFX_BufferedReadStream { FX_FILESIZE GetBlockOffset() override; private: + CXML_DataBufAcc(const uint8_t* pBuffer, size_t size); + ~CXML_DataBufAcc() override; + const uint8_t* m_pBuffer; size_t m_dwSize; size_t m_dwCurPos; @@ -130,9 +133,8 @@ FX_FILESIZE CXML_DataBufAcc::GetBlockOffset() { class CXML_DataStmAcc : public IFX_BufferedReadStream { public: - explicit CXML_DataStmAcc( - const CFX_RetainPtr& pFileRead); - ~CXML_DataStmAcc() override; + template + friend CFX_RetainPtr pdfium::MakeRetain(Args&&... args); // IFX_BufferedReadStream bool IsEOF() override; @@ -144,6 +146,10 @@ class CXML_DataStmAcc : public IFX_BufferedReadStream { FX_FILESIZE GetBlockOffset() override; private: + explicit CXML_DataStmAcc( + const CFX_RetainPtr& pFileRead); + ~CXML_DataStmAcc() override; + CFX_RetainPtr m_pFileRead; uint8_t* m_pBuffer; FX_FILESIZE m_nStart; @@ -215,12 +221,12 @@ CXML_Parser::CXML_Parser() CXML_Parser::~CXML_Parser() {} bool CXML_Parser::Init(uint8_t* pBuffer, size_t size) { - m_pDataAcc.Reset(new CXML_DataBufAcc(pBuffer, size)); + m_pDataAcc = pdfium::MakeRetain(pBuffer, size); return Init(); } bool CXML_Parser::Init(const CFX_RetainPtr& pFileRead) { - m_pDataAcc.Reset(new CXML_DataStmAcc(pFileRead)); + m_pDataAcc = pdfium::MakeRetain(pFileRead); return Init(); } diff --git a/xfa/fgas/crt/fgas_stream.cpp b/xfa/fgas/crt/fgas_stream.cpp index 966f615676..424b191cb8 100644 --- a/xfa/fgas/crt/fgas_stream.cpp +++ b/xfa/fgas/crt/fgas_stream.cpp @@ -194,8 +194,8 @@ enum FX_STREAMTYPE { class CFGAS_Stream : public IFGAS_Stream { public: - CFGAS_Stream(); - ~CFGAS_Stream() override; + template + friend CFX_RetainPtr pdfium::MakeRetain(Args&&... args); bool LoadFile(const FX_WCHAR* pszSrcFileName, uint32_t dwAccess); bool LoadBuffer(uint8_t* pData, int32_t iTotalSize, uint32_t dwAccess); @@ -227,6 +227,9 @@ class CFGAS_Stream : public IFGAS_Stream { int32_t iLength) override; protected: + CFGAS_Stream(); + ~CFGAS_Stream() override; + FX_STREAMTYPE m_eStreamType; IFGAS_StreamImp* m_pStreamImp; uint32_t m_dwAccess; @@ -239,8 +242,8 @@ class CFGAS_Stream : public IFGAS_Stream { class CFGAS_TextStream : public IFGAS_Stream { public: - explicit CFGAS_TextStream(const CFX_RetainPtr& pStream); - ~CFGAS_TextStream() override; + template + friend CFX_RetainPtr pdfium::MakeRetain(Args&&... args); // IFGAS_Stream uint32_t GetAccessModes() const override; @@ -262,6 +265,9 @@ class CFGAS_TextStream : public IFGAS_Stream { int32_t iLength) override; protected: + explicit CFGAS_TextStream(const CFX_RetainPtr& pStream); + ~CFGAS_TextStream() override; + void InitStream(); uint16_t m_wCodePage; diff --git a/xfa/fxfa/parser/cxfa_widetextread.cpp b/xfa/fxfa/parser/cxfa_widetextread.cpp index 54442df7c0..34fa6b9c5e 100644 --- a/xfa/fxfa/parser/cxfa_widetextread.cpp +++ b/xfa/fxfa/parser/cxfa_widetextread.cpp @@ -14,6 +14,8 @@ CXFA_WideTextRead::CXFA_WideTextRead(const CFX_WideString& wsBuffer) : m_wsBuffer(wsBuffer), m_iPosition(0) {} +CXFA_WideTextRead::~CXFA_WideTextRead() {} + uint32_t CXFA_WideTextRead::GetAccessModes() const { return FX_STREAMACCESS_Read | FX_STREAMACCESS_Text; } diff --git a/xfa/fxfa/parser/cxfa_widetextread.h b/xfa/fxfa/parser/cxfa_widetextread.h index d3d3b3792a..2ccb042908 100644 --- a/xfa/fxfa/parser/cxfa_widetextread.h +++ b/xfa/fxfa/parser/cxfa_widetextread.h @@ -7,11 +7,13 @@ #ifndef XFA_FXFA_PARSER_CXFA_WIDETEXTREAD_H_ #define XFA_FXFA_PARSER_CXFA_WIDETEXTREAD_H_ +#include "core/fxcrt/cfx_retain_ptr.h" #include "xfa/fgas/crt/fgas_stream.h" class CXFA_WideTextRead : public IFGAS_Stream { public: - explicit CXFA_WideTextRead(const CFX_WideString& wsBuffer); + template + friend CFX_RetainPtr pdfium::MakeRetain(Args&&... args); // IFGAS_Stream uint32_t GetAccessModes() const override; @@ -35,6 +37,9 @@ class CXFA_WideTextRead : public IFGAS_Stream { CFX_WideString GetSrcText() const; protected: + explicit CXFA_WideTextRead(const CFX_WideString& wsBuffer); + ~CXFA_WideTextRead() override; + CFX_WideString m_wsBuffer; int32_t m_iPosition; }; -- cgit v1.2.3