Rename CPDF_ReadValidator::Session to ScopedSession
Whilst reading through this code, it took me 30 seconds to realize
that the reason for these session objects was solely to restore state
when they went out of scope, so name them as such to make this clearer
to future readers.
Make members const, then remove const from callers, which also aided
in my reading of the code, since actions happen on cleanup which,
although legal, don't feel like the spirit of const.
Mark the class as stack-allocated only, then install a RetainPtr<>
rather than an UnownedPtr<> to the validator, since there can't
be any cycles and it can't hurt to hold that additional reference.
-- Initialize CPDF_ReadValidator members in header while at it.
-- Mark some CPDF_ReadValidator members const while at it.
Change-Id: I00952e19d9c173072bc0b89a824085c02a2ba9c5
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/76911
Reviewed-by: Hui Yingst <nigi@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
diff --git a/core/fpdfapi/parser/cpdf_cross_ref_avail.cpp b/core/fpdfapi/parser/cpdf_cross_ref_avail.cpp
index 4e6b52f..8895b6b 100644
--- a/core/fpdfapi/parser/cpdf_cross_ref_avail.cpp
+++ b/core/fpdfapi/parser/cpdf_cross_ref_avail.cpp
@@ -38,7 +38,7 @@
if (status_ == CPDF_DataAvail::DataAvailable)
return CPDF_DataAvail::DataAvailable;
- const CPDF_ReadValidator::Session read_session(GetValidator());
+ CPDF_ReadValidator::ScopedSession read_session(GetValidator());
while (true) {
bool check_result = false;
switch (state_) {
diff --git a/core/fpdfapi/parser/cpdf_data_avail.cpp b/core/fpdfapi/parser/cpdf_data_avail.cpp
index d1513a6..1f4d1c3 100644
--- a/core/fpdfapi/parser/cpdf_data_avail.cpp
+++ b/core/fpdfapi/parser/cpdf_data_avail.cpp
@@ -173,7 +173,7 @@
bool CPDF_DataAvail::CheckAndLoadAllXref() {
if (!m_pCrossRefAvail) {
- const CPDF_ReadValidator::Session read_session(GetValidator());
+ CPDF_ReadValidator::ScopedSession read_session(GetValidator());
const FX_FILESIZE last_xref_offset = m_parser.ParseStartXRef();
if (GetValidator()->has_read_problems())
return false;
@@ -221,7 +221,7 @@
RetainPtr<CPDF_Object> pRet;
if (pParser) {
- const CPDF_ReadValidator::Session read_session(GetValidator());
+ CPDF_ReadValidator::ScopedSession read_session(GetValidator());
pRet = pParser->ParseIndirectObject(objnum);
if (GetValidator()->has_read_problems())
return nullptr;
@@ -240,7 +240,7 @@
return true;
}
- const CPDF_ReadValidator::Session read_session(GetValidator());
+ CPDF_ReadValidator::ScopedSession read_session(GetValidator());
m_parser.ParseIndirectObject(dwInfoObjNum);
if (GetValidator()->has_read_problems())
return false;
@@ -256,7 +256,7 @@
return true;
}
- const CPDF_ReadValidator::Session read_session(GetValidator());
+ CPDF_ReadValidator::ScopedSession read_session(GetValidator());
m_pRoot = ToDictionary(m_parser.ParseIndirectObject(dwRootObjNum));
if (GetValidator()->has_read_problems())
return false;
@@ -435,7 +435,7 @@
}
bool CPDF_DataAvail::CheckHintTables() {
- const CPDF_ReadValidator::Session read_session(GetValidator());
+ CPDF_ReadValidator::ScopedSession read_session(GetValidator());
m_pHintTables =
CPDF_HintTables::Parse(GetSyntaxParser(), m_pLinearized.get());
@@ -483,7 +483,7 @@
if (m_bHeaderAvail)
return DocAvailStatus::DataAvailable;
- const CPDF_ReadValidator::Session read_session(GetValidator());
+ CPDF_ReadValidator::ScopedSession read_session(GetValidator());
const Optional<FX_FILESIZE> header_offset = GetHeaderOffset(GetValidator());
if (GetValidator()->has_read_problems())
return DocAvailStatus::DataNotAvailable;
@@ -882,7 +882,7 @@
CPDF_DataAvail::DocAvailStatus CPDF_DataAvail::CheckResources(
CPDF_Dictionary* page) {
ASSERT(page);
- const CPDF_ReadValidator::Session read_session(GetValidator());
+ CPDF_ReadValidator::ScopedSession read_session(GetValidator());
CPDF_Object* resources = GetResourceObject(page);
if (GetValidator()->has_read_problems())
return DocAvailStatus::DataNotAvailable;
@@ -1010,7 +1010,7 @@
std::move(pPageData));
document->AddObserver(this);
- CPDF_ReadValidator::Session read_session(GetValidator());
+ CPDF_ReadValidator::ScopedSession read_session(GetValidator());
CPDF_Parser::Error error =
document->LoadLinearizedDoc(GetValidator(), password);
diff --git a/core/fpdfapi/parser/cpdf_object_avail.cpp b/core/fpdfapi/parser/cpdf_object_avail.cpp
index 171c521..f4ed216 100644
--- a/core/fpdfapi/parser/cpdf_object_avail.cpp
+++ b/core/fpdfapi/parser/cpdf_object_avail.cpp
@@ -60,7 +60,7 @@
return true;
}
- const CPDF_ReadValidator::Session parse_session(validator_);
+ CPDF_ReadValidator::ScopedSession parse_session(validator_);
CPDF_Object* direct = holder_->GetOrParseIndirectObject(ref_obj_num);
if (validator_->has_read_problems())
return false;
@@ -90,7 +90,7 @@
if (!checked_objects.insert(obj_num).second)
continue;
- const CPDF_ReadValidator::Session parse_session(validator_);
+ CPDF_ReadValidator::ScopedSession parse_session(validator_);
const CPDF_Object* direct = holder_->GetOrParseIndirectObject(obj_num);
if (direct == root_)
continue;
@@ -113,7 +113,7 @@
CPDF_ObjectWalker walker(object);
while (const CPDF_Object* obj = walker.GetNext()) {
- const CPDF_ReadValidator::Session parse_session(validator_);
+ CPDF_ReadValidator::ScopedSession parse_session(validator_);
// Skip if this object if it's an inlined root, the parent object or
// explicitily excluded.
diff --git a/core/fpdfapi/parser/cpdf_read_validator.cpp b/core/fpdfapi/parser/cpdf_read_validator.cpp
index 9fca37f..b87d940 100644
--- a/core/fpdfapi/parser/cpdf_read_validator.cpp
+++ b/core/fpdfapi/parser/cpdf_read_validator.cpp
@@ -5,6 +5,7 @@
#include "core/fpdfapi/parser/cpdf_read_validator.h"
#include <algorithm>
+#include <utility>
#include "core/fpdfapi/parser/cpdf_stream.h"
#include "core/fxcrt/fx_safe_types.h"
@@ -26,16 +27,15 @@
} // namespace
-CPDF_ReadValidator::Session::Session(
- const RetainPtr<CPDF_ReadValidator>& validator)
- : validator_(validator.BackPointer()) {
- ASSERT(validator_);
- saved_read_error_ = validator_->read_error_;
- saved_has_unavailable_data_ = validator_->has_unavailable_data_;
+CPDF_ReadValidator::ScopedSession::ScopedSession(
+ RetainPtr<CPDF_ReadValidator> validator)
+ : validator_(std::move(validator)),
+ saved_read_error_(validator_->read_error_),
+ saved_has_unavailable_data_(validator_->has_unavailable_data_) {
validator_->ResetErrors();
}
-CPDF_ReadValidator::Session::~Session() {
+CPDF_ReadValidator::ScopedSession::~ScopedSession() {
validator_->read_error_ |= saved_read_error_;
validator_->has_unavailable_data_ |= saved_has_unavailable_data_;
}
@@ -45,9 +45,6 @@
CPDF_DataAvail::FileAvail* file_avail)
: file_read_(file_read),
file_avail_(file_avail),
- read_error_(false),
- has_unavailable_data_(false),
- whole_file_already_available_(false),
file_size_(file_read->GetSize()) {}
CPDF_ReadValidator::~CPDF_ReadValidator() = default;
diff --git a/core/fpdfapi/parser/cpdf_read_validator.h b/core/fpdfapi/parser/cpdf_read_validator.h
index e330661..4119d29 100644
--- a/core/fpdfapi/parser/cpdf_read_validator.h
+++ b/core/fpdfapi/parser/cpdf_read_validator.h
@@ -11,15 +11,21 @@
class CPDF_ReadValidator : public IFX_SeekableReadStream {
public:
- class Session {
+ class ScopedSession {
public:
- explicit Session(const RetainPtr<CPDF_ReadValidator>& validator);
- ~Session();
+ explicit ScopedSession(RetainPtr<CPDF_ReadValidator> validator);
+ ScopedSession(const ScopedSession& that) = delete;
+ ScopedSession& operator=(const ScopedSession& that) = delete;
+ ~ScopedSession();
private:
- UnownedPtr<CPDF_ReadValidator> validator_;
- bool saved_read_error_;
- bool saved_has_unavailable_data_;
+ // Make stack-allocated.
+ void* operator new(size_t) = delete;
+ void* operator new(size_t, void*) = delete;
+
+ RetainPtr<CPDF_ReadValidator> const validator_;
+ const bool saved_read_error_;
+ const bool saved_has_unavailable_data_;
};
CONSTRUCT_VIA_MAKE_RETAIN;
@@ -34,9 +40,7 @@
}
void ResetErrors();
-
bool IsWholeFileAvailable();
-
bool CheckDataRangeAndRequestIfUnavailable(FX_FILESIZE offset, size_t size);
bool CheckWholeFileAndRequestIfUnavailable();
@@ -55,14 +59,12 @@
void ScheduleDownload(FX_FILESIZE offset, size_t size);
bool IsDataRangeAvailable(FX_FILESIZE offset, size_t size) const;
- RetainPtr<IFX_SeekableReadStream> file_read_;
- UnownedPtr<CPDF_DataAvail::FileAvail> file_avail_;
-
+ RetainPtr<IFX_SeekableReadStream> const file_read_;
+ UnownedPtr<CPDF_DataAvail::FileAvail> const file_avail_;
UnownedPtr<CPDF_DataAvail::DownloadHints> hints_;
-
- bool read_error_;
- bool has_unavailable_data_;
- bool whole_file_already_available_;
+ bool read_error_ = false;
+ bool has_unavailable_data_ = false;
+ bool whole_file_already_available_ = false;
const FX_FILESIZE file_size_;
};
diff --git a/core/fpdfapi/parser/cpdf_read_validator_unittest.cpp b/core/fpdfapi/parser/cpdf_read_validator_unittest.cpp
index d3d019c..abc8603 100644
--- a/core/fpdfapi/parser/cpdf_read_validator_unittest.cpp
+++ b/core/fpdfapi/parser/cpdf_read_validator_unittest.cpp
@@ -169,7 +169,7 @@
auto validator = pdfium::MakeRetain<CPDF_ReadValidator>(file, &file_avail);
validator->SetDownloadHints(&hints);
- const CPDF_ReadValidator::Session read_session(validator);
+ CPDF_ReadValidator::ScopedSession read_session(validator);
ASSERT_FALSE(validator->has_read_problems());
// Data is unavailable
@@ -180,7 +180,7 @@
EXPECT_FALSE(validator->read_error());
{
- const CPDF_ReadValidator::Session read_subsession(validator);
+ CPDF_ReadValidator::ScopedSession read_subsession(validator);
// The read problems should be hidden.
EXPECT_FALSE(validator->has_read_problems());
@@ -207,7 +207,7 @@
auto validator = pdfium::MakeRetain<CPDF_ReadValidator>(file, &file_avail);
validator->SetDownloadHints(&hints);
- const CPDF_ReadValidator::Session read_session(validator);
+ CPDF_ReadValidator::ScopedSession read_session(validator);
ASSERT_FALSE(validator->has_read_problems());
// Data is unavailable
@@ -218,7 +218,7 @@
EXPECT_FALSE(validator->read_error());
{
- const CPDF_ReadValidator::Session read_subsession(validator);
+ CPDF_ReadValidator::ScopedSession read_subsession(validator);
// The read problems should be hidden.
EXPECT_FALSE(validator->has_read_problems());
diff --git a/core/fpdfapi/parser/cpdf_syntax_parser.cpp b/core/fpdfapi/parser/cpdf_syntax_parser.cpp
index 5318efd..f8bf219 100644
--- a/core/fpdfapi/parser/cpdf_syntax_parser.cpp
+++ b/core/fpdfapi/parser/cpdf_syntax_parser.cpp
@@ -465,7 +465,7 @@
}
ByteString CPDF_SyntaxParser::GetNextWord(bool* bIsNumber) {
- const CPDF_ReadValidator::Session read_session(GetValidator());
+ CPDF_ReadValidator::ScopedSession read_session(GetValidator());
GetNextWordInternal(bIsNumber);
ByteString ret;
if (!GetValidator()->has_read_problems())
@@ -488,7 +488,7 @@
RetainPtr<CPDF_Object> CPDF_SyntaxParser::GetObjectBody(
CPDF_IndirectObjectHolder* pObjList) {
- const CPDF_ReadValidator::Session read_session(GetValidator());
+ CPDF_ReadValidator::ScopedSession read_session(GetValidator());
auto result = GetObjectBodyInternal(pObjList, ParseType::kLoose);
if (GetValidator()->has_read_problems())
return nullptr;
@@ -609,7 +609,7 @@
RetainPtr<CPDF_Object> CPDF_SyntaxParser::GetIndirectObject(
CPDF_IndirectObjectHolder* pObjList,
ParseType parse_type) {
- const CPDF_ReadValidator::Session read_session(GetValidator());
+ CPDF_ReadValidator::ScopedSession read_session(GetValidator());
const FX_FILESIZE saved_pos = GetPos();
bool is_number = false;
ByteString word = GetNextWord(&is_number);
@@ -742,7 +742,7 @@
// Note, we allow zero length streams as we need to pass them through when we
// are importing pages into a new document.
if (len >= 0) {
- const CPDF_ReadValidator::Session read_session(GetValidator());
+ CPDF_ReadValidator::ScopedSession read_session(GetValidator());
m_Pos += ReadEOLMarkers(GetPos());
memset(m_WordBuffer, 0, kEndStreamStr.GetLength() + 1);
GetNextWordInternal(nullptr);
