commit dc4e60e8e8f5184d9489512e92dd4bcf07001dc2
author Tom Sepez <tsepez@chromium.org> Thu Oct 05 22:40:36 2023 +0000
committer Pdfium LUCI CQ <pdfium-scoped@luci-project-accounts.iam.gserviceaccount.com> Thu Oct 05 22:40:36 2023 +0000
tree ee22c9a2b01efa263e694218125bad5d51f131de
parent 72637154dff671e01bb903520c636ee28df010c0

Fix dangling ptrs in embeddertest found by BRP dangling ptr detector.

These were missed by the PDFium probing-style detector because the
raw_ptr<> were never destroyed. LSAN could not find it either, since
the memory was not leaked, just the dangling references were not
cleaned up beforehand.

The reason is that we have C++ classes inheriting from C-style
structs, hence there can not be virtual destructors. When holding an
unique_ptr<> to the C-style struct, the subclass is never destructed.

After this CL lands, we should be able to make the build bots set
the GN variable `enable_dangling_raw_ptr_checks=true`. If that were
set today, hopefully it would flag the issue.

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

testing/fake_file_access.cpp

diff --git a/testing/fake_file_access.cpp b/testing/fake_file_access.cpp
index 4bd8fa1..7d89680 100644
--- a/testing/fake_file_access.cpp
+++ b/testing/fake_file_access.cpp
@@ -9,8 +9,6 @@
 #include "core/fxcrt/fx_system.h"
 #include "third_party/base/check.h"
 
-namespace {
-
 class FileAccessWrapper final : public FPDF_FILEACCESS {
  public:
   explicit FileAccessWrapper(FakeFileAccess* simulator)
@@ -69,8 +67,6 @@
   UnownedPtr<FakeFileAccess> simulator_;
 };
 
-}  // namespace
-
 FakeFileAccess::FakeFileAccess(FPDF_FILEACCESS* file_access)
     : file_access_(file_access),
       file_access_wrapper_(std::make_unique<FileAccessWrapper>(this)),
@@ -81,6 +77,18 @@
 
 FakeFileAccess::~FakeFileAccess() = default;
 
+FPDF_FILEACCESS* FakeFileAccess::GetFileAccess() const {
+  return file_access_wrapper_.get();
+}
+
+FX_FILEAVAIL* FakeFileAccess::GetFileAvail() const {
+  return file_avail_.get();
+}
+
+FX_DOWNLOADHINTS* FakeFileAccess::GetDownloadHints() const {
+  return download_hints_.get();
+}
+
 FPDF_BOOL FakeFileAccess::IsDataAvail(size_t offset, size_t size) const {
   return available_data_.Contains(RangeSet::Range(offset, offset + size));
 }

testing/fake_file_access.h

diff --git a/testing/fake_file_access.h b/testing/fake_file_access.h
index 6a0f3cf..c0b3689 100644
--- a/testing/fake_file_access.h
+++ b/testing/fake_file_access.h
@@ -12,14 +12,18 @@
 #include "public/fpdfview.h"
 #include "testing/range_set.h"
 
+class DownloadHintsImpl;
+class FileAccessWrapper;
+class FileAvailImpl;
+
 class FakeFileAccess {
  public:
   explicit FakeFileAccess(FPDF_FILEACCESS* file_access);
   ~FakeFileAccess();
 
-  FPDF_FILEACCESS* GetFileAccess() const { return file_access_wrapper_.get(); }
-  FX_FILEAVAIL* GetFileAvail() const { return file_avail_.get(); }
-  FX_DOWNLOADHINTS* GetDownloadHints() const { return download_hints_.get(); }
+  FPDF_FILEACCESS* GetFileAccess() const;
+  FX_FILEAVAIL* GetFileAvail() const;
+  FX_DOWNLOADHINTS* GetDownloadHints() const;
 
   FPDF_BOOL IsDataAvail(size_t offset, size_t size) const;
   void AddSegment(size_t offset, size_t size);
@@ -32,9 +36,9 @@
 
  private:
   UnownedPtr<FPDF_FILEACCESS> file_access_;
-  std::unique_ptr<FPDF_FILEACCESS> file_access_wrapper_;
-  std::unique_ptr<FX_FILEAVAIL> file_avail_;
-  std::unique_ptr<FX_DOWNLOADHINTS> download_hints_;
+  std::unique_ptr<FileAccessWrapper> file_access_wrapper_;
+  std::unique_ptr<FileAvailImpl> file_avail_;
+  std::unique_ptr<DownloadHintsImpl> download_hints_;
   RangeSet available_data_;
   RangeSet requested_data_;
 };