commit 8de8494f80f3ba9beeb9a68d2fc8d1221b16b381
author Lei Zhang <thestig@chromium.org> Wed Oct 20 23:45:53 2021 +0000
committer Pdfium LUCI CQ <pdfium-scoped@luci-project-accounts.iam.gserviceaccount.com> Wed Oct 20 23:45:53 2021 +0000
tree 8e0a4adae3245a0694f33a56ee0067a95afe27c3
parent 116c227e40a9eae4ee706d0c6ccb0d9c0c51f411

Remove test friends and protected member from CPDF_Parser.

Add protected methods instead of friend classes. This is more scalable
as more test cases get added.

Also remove the protected member and make it private. Add a protected
method to set the member instead.

For CPDF_Parser::ObjectType and CPDF_Parser::ObjectInfo, make them
public, as they are really just aliases for public bits in
CPDF_CrossRefTable.

Change-Id: I1655f9bc6d6b714223e75ba26e3a4ab3603d5903
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/86180
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>

core/fpdfapi/parser/cpdf_parser.cpp

diff --git a/core/fpdfapi/parser/cpdf_parser.cpp b/core/fpdfapi/parser/cpdf_parser.cpp
index 1a5f10a..3e456e8 100644
--- a/core/fpdfapi/parser/cpdf_parser.cpp
+++ b/core/fpdfapi/parser/cpdf_parser.cpp
@@ -1105,3 +1105,8 @@
       return CPDF_Parser::ObjectType::kNull;
   }
 }
+
+void CPDF_Parser::SetSyntaxParserForTesting(
+    std::unique_ptr<CPDF_SyntaxParser> parser) {
+  m_pSyntax = std::move(parser);
+}

core/fpdfapi/parser/cpdf_parser.h

diff --git a/core/fpdfapi/parser/cpdf_parser.h b/core/fpdfapi/parser/cpdf_parser.h
index dc658e0..513f6a4 100644
--- a/core/fpdfapi/parser/cpdf_parser.h
+++ b/core/fpdfapi/parser/cpdf_parser.h
@@ -35,6 +35,9 @@
 
 class CPDF_Parser {
  public:
+  using ObjectType = CPDF_CrossRefTable::ObjectType;
+  using ObjectInfo = CPDF_CrossRefTable::ObjectInfo;
+
   class ParsedObjectsHolder : public CPDF_IndirectObjectHolder {
    public:
     virtual bool TryInit() = 0;
@@ -124,19 +127,15 @@
       std::unique_ptr<CPDF_LinearizedHeader> pLinearized);
 
  protected:
-  using ObjectType = CPDF_CrossRefTable::ObjectType;
-  using ObjectInfo = CPDF_CrossRefTable::ObjectInfo;
-
   bool LoadCrossRefV4(FX_FILESIZE pos, bool bSkip);
   bool RebuildCrossRef();
+  Error StartParseInternal();
+  FX_FILESIZE ParseStartXRef();
+  std::unique_ptr<CPDF_LinearizedHeader> ParseLinearizedHeader();
 
-  std::unique_ptr<CPDF_SyntaxParser> m_pSyntax;
+  void SetSyntaxParserForTesting(std::unique_ptr<CPDF_SyntaxParser> parser);
 
  private:
-  friend class cpdf_parser_BadStartXrefShouldNotBuildCrossRefTable_Test;
-  friend class cpdf_parser_ParseStartXRefWithHeaderOffset_Test;
-  friend class cpdf_parser_ParseStartXRef_Test;
-  friend class cpdf_parser_ParseLinearizedWithHeaderOffset_Test;
   friend class CPDF_DataAvail;
 
   struct CrossRefObjData {
@@ -144,8 +143,6 @@
     ObjectInfo info;
   };
 
-  Error StartParseInternal();
-  FX_FILESIZE ParseStartXRef();
   bool LoadAllCrossRefV4(FX_FILESIZE xref_offset);
   bool LoadAllCrossRefV5(FX_FILESIZE xref_offset);
   bool LoadCrossRefV5(FX_FILESIZE* pos, bool bMainXRef);
@@ -156,7 +153,6 @@
   bool LoadLinearizedAllCrossRefV5(FX_FILESIZE main_xref_offset);
   Error LoadLinearizedMainXRefTable();
   const CPDF_ObjectStream* GetObjectStream(uint32_t object_number);
-  std::unique_ptr<CPDF_LinearizedHeader> ParseLinearizedHeader();
   void ShrinkObjectMap(uint32_t size);
   // A simple check whether the cross reference table matches with
   // the objects.
@@ -178,6 +174,7 @@
   ObjectType GetObjectTypeFromCrossRefStreamType(
       uint32_t cross_ref_stream_type) const;
 
+  std::unique_ptr<CPDF_SyntaxParser> m_pSyntax;
   std::unique_ptr<ParsedObjectsHolder> m_pOwnedObjectsHolder;
   UnownedPtr<ParsedObjectsHolder> m_pObjectsHolder;
 

core/fpdfapi/parser/cpdf_parser_unittest.cpp

diff --git a/core/fpdfapi/parser/cpdf_parser_unittest.cpp b/core/fpdfapi/parser/cpdf_parser_unittest.cpp
index 9e0cbd9..ce484f6 100644
--- a/core/fpdfapi/parser/cpdf_parser_unittest.cpp
+++ b/core/fpdfapi/parser/cpdf_parser_unittest.cpp
@@ -44,15 +44,15 @@
       return false;
 
     // For the test file, the header is set at the beginning.
-    m_pSyntax = std::make_unique<CPDF_SyntaxParser>(pFileAccess);
+    SetSyntaxParserForTesting(std::make_unique<CPDF_SyntaxParser>(pFileAccess));
     return true;
   }
 
   // Setup reading from a buffer and initial states.
   bool InitTestFromBufferWithOffset(pdfium::span<const uint8_t> buffer,
                                     FX_FILESIZE header_offset) {
-    m_pSyntax = CPDF_SyntaxParser::CreateForTesting(
-        pdfium::MakeRetain<CFX_ReadOnlyMemoryStream>(buffer), header_offset);
+    SetSyntaxParserForTesting(CPDF_SyntaxParser::CreateForTesting(
+        pdfium::MakeRetain<CFX_ReadOnlyMemoryStream>(buffer), header_offset));
     return true;
   }
 
@@ -60,14 +60,12 @@
     return InitTestFromBufferWithOffset(buffer, 0 /*header_offset*/);
   }
 
- private:
-  // Add test cases here as private friend so that protected members in
-  // CPDF_Parser can be accessed by test cases.
-  // Need to access RebuildCrossRef.
-  FRIEND_TEST(cpdf_parser, RebuildCrossRefCorrectly);
-  FRIEND_TEST(cpdf_parser, RebuildCrossRefFailed);
-  // Need to access LoadCrossRefV4.
-  FRIEND_TEST(cpdf_parser, LoadCrossRefV4);
+  // Expose protected CPDF_Parser methods for testing.
+  using CPDF_Parser::LoadCrossRefV4;
+  using CPDF_Parser::ParseLinearizedHeader;
+  using CPDF_Parser::ParseStartXRef;
+  using CPDF_Parser::RebuildCrossRef;
+  using CPDF_Parser::StartParseInternal;
 };
 
 TEST(cpdf_parser, RebuildCrossRefCorrectly) {