Simplify IFX_SeekableWriteStream::WriteBlockAtOffset()
All of its non-test callers always pass in the same offset value. So
simplify it and change WriteBlockAtOffset() to AppendBlock(). Update all
the WriteBlockAtOffset() implementations.
Remove the CFX_MemoryStream unit tests that directly call
WriteBlockAtOffset(), which are trying to do sparse writes. Replace them
with more tests for CFX_MemoryStream::AppendBlock(). Improve
CFX_MemoryStream::ReadBlockAtOffset() testing along the way.
Change-Id: I123adeab54cc4f23d378d8e7fe46963303be17a9
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/123490
Reviewed-by: Tom Sepez <tsepez@google.com>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
diff --git a/core/fpdfapi/parser/cpdf_seekablemultistream.cpp b/core/fpdfapi/parser/cpdf_seekablemultistream.cpp
index 80e6dd5..7700834 100644
--- a/core/fpdfapi/parser/cpdf_seekablemultistream.cpp
+++ b/core/fpdfapi/parser/cpdf_seekablemultistream.cpp
@@ -75,8 +75,6 @@
NOTREACHED_NORETURN();
}
-bool CPDF_SeekableMultiStream::WriteBlockAtOffset(
- pdfium::span<const uint8_t> buffer,
- FX_FILESIZE offset) {
+bool CPDF_SeekableMultiStream::AppendBlock(pdfium::span<const uint8_t> buffer) {
NOTREACHED_NORETURN();
}
diff --git a/core/fpdfapi/parser/cpdf_seekablemultistream.h b/core/fpdfapi/parser/cpdf_seekablemultistream.h
index fc659a6..c227df9 100644
--- a/core/fpdfapi/parser/cpdf_seekablemultistream.h
+++ b/core/fpdfapi/parser/cpdf_seekablemultistream.h
@@ -28,8 +28,7 @@
size_t ReadBlock(pdfium::span<uint8_t> buffer) override;
bool ReadBlockAtOffset(pdfium::span<uint8_t> buffer,
FX_FILESIZE offset) override;
- bool WriteBlockAtOffset(pdfium::span<const uint8_t> buffer,
- FX_FILESIZE offset) override;
+ bool AppendBlock(pdfium::span<const uint8_t> buffer) override;
bool Flush() override;
private:
diff --git a/core/fxcrt/cfx_memorystream.cpp b/core/fxcrt/cfx_memorystream.cpp
index 6595184..2933902 100644
--- a/core/fxcrt/cfx_memorystream.cpp
+++ b/core/fxcrt/cfx_memorystream.cpp
@@ -67,18 +67,15 @@
return nRead;
}
-bool CFX_MemoryStream::WriteBlockAtOffset(pdfium::span<const uint8_t> buffer,
- FX_FILESIZE offset) {
- if (offset < 0)
- return false;
-
+bool CFX_MemoryStream::AppendBlock(pdfium::span<const uint8_t> buffer) {
if (buffer.empty())
return true;
FX_SAFE_SIZE_T safe_new_pos = buffer.size();
- safe_new_pos += offset;
- if (!safe_new_pos.IsValid())
+ safe_new_pos += m_nCurSize;
+ if (!safe_new_pos.IsValid()) {
return false;
+ }
size_t new_pos = safe_new_pos.ValueOrDie();
if (new_pos > m_data.size()) {
@@ -95,10 +92,7 @@
}
m_nCurPos = new_pos;
- // Safe to cast `offset` because it was used to calculate `safe_new_pos`
- // above, and `safe_new_pos` is valid.
- fxcrt::Copy(buffer,
- pdfium::make_span(m_data).subspan(static_cast<size_t>(offset)));
+ fxcrt::Copy(buffer, pdfium::make_span(m_data).subspan(m_nCurSize));
m_nCurSize = std::max(m_nCurSize, m_nCurPos);
return true;
diff --git a/core/fxcrt/cfx_memorystream.h b/core/fxcrt/cfx_memorystream.h
index 1709589..fcced54 100644
--- a/core/fxcrt/cfx_memorystream.h
+++ b/core/fxcrt/cfx_memorystream.h
@@ -23,8 +23,7 @@
size_t ReadBlock(pdfium::span<uint8_t> buffer) override;
bool ReadBlockAtOffset(pdfium::span<uint8_t> buffer,
FX_FILESIZE offset) override;
- bool WriteBlockAtOffset(pdfium::span<const uint8_t> buffer,
- FX_FILESIZE offset) override;
+ bool AppendBlock(pdfium::span<const uint8_t> buffer) override;
bool Flush() override;
pdfium::span<const uint8_t> GetSpan() const;
diff --git a/core/fxcrt/cfx_memorystream_unittest.cpp b/core/fxcrt/cfx_memorystream_unittest.cpp
index 1e6fbef..6ce47cc 100644
--- a/core/fxcrt/cfx_memorystream_unittest.cpp
+++ b/core/fxcrt/cfx_memorystream_unittest.cpp
@@ -8,44 +8,43 @@
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
-namespace {
-
-const uint8_t kSomeText[] = "Lets make holes in streams";
-const size_t kSomeTextLen = sizeof(kSomeText) - 1;
-
-} // namespace
-
-TEST(CFXMemoryStreamTest, SparseBlockWrites) {
+TEST(CFXMemoryStreamTest, ReadBlockAtOffset) {
auto stream = pdfium::MakeRetain<CFX_MemoryStream>();
- for (FX_FILESIZE offset = 0; offset <= 200000; offset += 100000) {
- stream->WriteBlockAtOffset(pdfium::make_span(kSomeText).first(kSomeTextLen),
- offset);
- }
- EXPECT_EQ(200000 + kSomeTextLen, static_cast<size_t>(stream->GetSize()));
+ const uint8_t kData1[] = {'a', 'b', 'c', 'd', 'e', 'f'};
+ ASSERT_TRUE(stream->WriteBlock(kData1));
+ ASSERT_THAT(stream->GetSpan(),
+ testing::ElementsAre('a', 'b', 'c', 'd', 'e', 'f'));
+
+ uint8_t buffer[3];
+ ASSERT_TRUE(stream->ReadBlockAtOffset(buffer, 2));
+ ASSERT_THAT(buffer, testing::ElementsAre('c', 'd', 'e'));
+
+ ASSERT_TRUE(stream->ReadBlockAtOffset(buffer, 0));
+ ASSERT_THAT(buffer, testing::ElementsAre('a', 'b', 'c'));
+
+ ASSERT_TRUE(stream->ReadBlockAtOffset(buffer, 3));
+ ASSERT_THAT(buffer, testing::ElementsAre('d', 'e', 'f'));
+
+ ASSERT_FALSE(stream->ReadBlockAtOffset(buffer, 4));
+ ASSERT_THAT(buffer, testing::ElementsAre('d', 'e', 'f'));
}
-TEST(CFXMemoryStreamTest, OverlappingBlockWrites) {
+TEST(CFXMemoryStreamTest, AppendBlock) {
auto stream = pdfium::MakeRetain<CFX_MemoryStream>();
- for (FX_FILESIZE offset = 0; offset <= 100; ++offset) {
- stream->WriteBlockAtOffset(pdfium::make_span(kSomeText).first(kSomeTextLen),
- offset);
- }
- EXPECT_EQ(100 + kSomeTextLen, static_cast<size_t>(stream->GetSize()));
-}
+ ASSERT_TRUE(stream->AppendBlock({}));
+ ASSERT_TRUE(stream->GetSpan().empty());
-TEST(CFXMemoryStreamTest, ReadWriteBlockAtOffset) {
- auto stream = pdfium::MakeRetain<CFX_MemoryStream>();
const uint8_t kData1[] = {'a', 'b', 'c'};
ASSERT_TRUE(stream->WriteBlock(kData1));
ASSERT_THAT(stream->GetSpan(), testing::ElementsAre('a', 'b', 'c'));
- ASSERT_TRUE(stream->WriteBlockAtOffset(kData1, 5));
+ ASSERT_TRUE(stream->AppendBlock(kData1));
ASSERT_THAT(stream->GetSpan(),
- testing::ElementsAre('a', 'b', 'c', '\0', '\0', 'a', 'b', 'c'));
+ testing::ElementsAre('a', 'b', 'c', 'a', 'b', 'c'));
- uint8_t buffer[4];
- ASSERT_TRUE(stream->ReadBlockAtOffset(buffer, 2));
- ASSERT_THAT(buffer, testing::ElementsAre('c', '\0', '\0', 'a'));
+ ASSERT_TRUE(stream->AppendBlock({}));
+ ASSERT_THAT(stream->GetSpan(),
+ testing::ElementsAre('a', 'b', 'c', 'a', 'b', 'c'));
}
TEST(CFXMemoryStreamTest, WriteZeroBytes) {
diff --git a/core/fxcrt/fx_stream.cpp b/core/fxcrt/fx_stream.cpp
index 05e2b51..775e6e0 100644
--- a/core/fxcrt/fx_stream.cpp
+++ b/core/fxcrt/fx_stream.cpp
@@ -28,9 +28,8 @@
size_t ReadBlock(pdfium::span<uint8_t> buffer) override {
return m_pFile->Read(buffer);
}
- bool WriteBlockAtOffset(pdfium::span<const uint8_t> buffer,
- FX_FILESIZE offset) override {
- return !!m_pFile->WritePos(buffer, offset);
+ bool AppendBlock(pdfium::span<const uint8_t> buffer) override {
+ return !!m_pFile->WritePos(buffer, GetSize());
}
bool Flush() override { return m_pFile->Flush(); }
@@ -75,7 +74,7 @@
}
bool IFX_SeekableWriteStream::WriteBlock(pdfium::span<const uint8_t> buffer) {
- return WriteBlockAtOffset(buffer, GetSize());
+ return AppendBlock(buffer);
}
bool IFX_SeekableReadStream::IsEOF() {
@@ -91,5 +90,5 @@
}
bool IFX_SeekableStream::WriteBlock(pdfium::span<const uint8_t> buffer) {
- return WriteBlockAtOffset(buffer, GetSize());
+ return AppendBlock(buffer);
}
diff --git a/core/fxcrt/fx_stream.h b/core/fxcrt/fx_stream.h
index e9caeb7..c1430bb 100644
--- a/core/fxcrt/fx_stream.h
+++ b/core/fxcrt/fx_stream.h
@@ -49,8 +49,7 @@
bool WriteBlock(pdfium::span<const uint8_t> buffer) override;
virtual bool Flush() = 0;
- virtual bool WriteBlockAtOffset(pdfium::span<const uint8_t> data,
- FX_FILESIZE offset) = 0;
+ virtual bool AppendBlock(pdfium::span<const uint8_t> buffer) = 0;
};
class IFX_SeekableReadStream : virtual public Retainable,
diff --git a/fpdfsdk/cpdfsdk_helpers.cpp b/fpdfsdk/cpdfsdk_helpers.cpp
index 209ad66..e63860a 100644
--- a/fpdfsdk/cpdfsdk_helpers.cpp
+++ b/fpdfsdk/cpdfsdk_helpers.cpp
@@ -102,8 +102,7 @@
size_t ReadBlock(pdfium::span<uint8_t> buffer) override;
bool ReadBlockAtOffset(pdfium::span<uint8_t> buffer,
FX_FILESIZE offset) override;
- bool WriteBlockAtOffset(pdfium::span<const uint8_t> buffer,
- FX_FILESIZE offset) override;
+ bool AppendBlock(pdfium::span<const uint8_t> buffer) override;
bool Flush() override;
void SetPosition(FX_FILESIZE pos) { m_nCurPos = pos; }
@@ -172,19 +171,19 @@
return 0;
}
-bool FPDF_FileHandlerContext::WriteBlockAtOffset(
- pdfium::span<const uint8_t> buffer,
- FX_FILESIZE offset) {
- if (!m_pFS || !m_pFS->WriteBlock)
+bool FPDF_FileHandlerContext::AppendBlock(pdfium::span<const uint8_t> buffer) {
+ if (!m_pFS || !m_pFS->WriteBlock) {
return false;
-
- if (m_pFS->WriteBlock(m_pFS->clientData, static_cast<FPDF_DWORD>(offset),
- buffer.data(),
- static_cast<FPDF_DWORD>(buffer.size())) == 0) {
- m_nCurPos = offset + buffer.size();
- return true;
}
- return false;
+
+ if (m_pFS->WriteBlock(m_pFS->clientData, static_cast<FPDF_DWORD>(GetSize()),
+ buffer.data(),
+ static_cast<FPDF_DWORD>(buffer.size())) != 0) {
+ return false;
+ }
+
+ m_nCurPos = GetSize() + buffer.size();
+ return true;
}
bool FPDF_FileHandlerContext::Flush() {
