SQL: fix fuzzer-discovered issue in recovery module

The fix is in cursor.cc, where a page ID was directly compared to
kInvalidPageId instead of being validated by the `IsValidPageId()` fn.

The rest of the modifications in this change are:
* changing some DCHECKs to CHECKs for improved security and bug
  discovery
* application of `IsValidPageId()` rather than direct comparison in
  more places, for correctness and not out of strict necessity

Bug: 1508758
Change-Id: Ic8d344f710b2008cf7d45bab608195d1d487f681
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5101280
Reviewed-by: Ayu Ishii <ayui@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1235108}

sql/recover_module/btree.cc

@@ -63,7 +63,7 @@ int InnerPageDecoder::TryAdvance() {
     // TODO(pwnall): UMA the error code.
 
     next_read_index_ = cell_count_ + 1;  // End the reading process.
-    return DatabasePageReader::kInvalidPageId;
+    return DatabasePageReader::kHighestInvalidPageId;
   }
 
   const uint8_t* const page_data = db_reader_->page_data();
@@ -85,11 +85,11 @@ int InnerPageDecoder::TryAdvance() {
     // Each cell needs 1 byte for the rowid varint, in addition to the 4 bytes
     // for the child page number that will be read below. Skip cells that
     // obviously go over the page end.
-    return DatabasePageReader::kInvalidPageId;
+    return DatabasePageReader::kHighestInvalidPageId;
   }
   if (cell_pointer < kFirstCellOfsetInnerPageOffset) {
     // The pointer points into the cell's header.
-    return DatabasePageReader::kInvalidPageId;
+    return DatabasePageReader::kHighestInvalidPageId;
   }
 
   return LoadBigEndianInt32(page_data + cell_pointer);

sql/recover_module/cursor.cc

@@ -72,8 +72,9 @@ int VirtualCursor::Next() {
       continue;
     }
     int next_page_id = inner_decoder->TryAdvance();
-    if (next_page_id == DatabasePageReader::kInvalidPageId)
+    if (!DatabasePageReader::IsValidPageId(next_page_id)) {
       continue;
+    }
     AppendPageDecoder(next_page_id);
   }
 

sql/recover_module/pager.cc

@@ -6,13 +6,14 @@
 
 #include <limits>
 
+#include "base/logging.h"
 #include "sql/recover_module/table.h"
 #include "third_party/sqlite/sqlite3.h"
 
 namespace sql {
 namespace recover {
 
-constexpr int DatabasePageReader::kInvalidPageId;
+constexpr int DatabasePageReader::kHighestInvalidPageId;
 constexpr int DatabasePageReader::kMinPageSize;
 constexpr int DatabasePageReader::kMaxPageSize;
 constexpr int DatabasePageReader::kDatabaseHeaderSize;
@@ -25,16 +26,16 @@ static_assert(DatabasePageReader::kMaxPageId <= std::numeric_limits<int>::max(),
 DatabasePageReader::DatabasePageReader(VirtualTable* table)
     : page_data_(std::make_unique<uint8_t[]>(table->page_size())),
       table_(table) {
-  DCHECK(table != nullptr);
-  DCHECK(IsValidPageSize(table->page_size()));
+  CHECK(table != nullptr);
+  CHECK(IsValidPageSize(table->page_size()));
 }
 
 DatabasePageReader::~DatabasePageReader() = default;
 
 int DatabasePageReader::ReadPage(int page_id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK_GT(page_id, kInvalidPageId);
-  DCHECK_LE(page_id, kMaxPageId);
+  CHECK(IsValidPageId(page_id));
+  CHECK_LE(page_id, kMaxPageId);
 
   if (page_id_ == page_id)
     return SQLITE_OK;
@@ -47,8 +48,8 @@ int DatabasePageReader::ReadPage(int page_id) {
                 "The |read_size| computation above may overflow");
 
   page_size_ = read_size;
-  DCHECK_GE(page_size_, kMinUsablePageSize);
-  DCHECK_LE(page_size_, kMaxPageSize);
+  CHECK_GE(page_size_, kMinUsablePageSize);
+  CHECK_LE(page_size_, kMaxPageSize);
 
   const int64_t read_offset =
       static_cast<int64_t>(page_id - 1) * page_size + page_offset;
@@ -60,10 +61,10 @@ int DatabasePageReader::ReadPage(int page_id) {
   int sqlite_status =
       RawRead(sqlite_file, read_size, read_offset, page_data_.get());
 
-  // |page_id_| needs to be set to kInvalidPageId if the read failed.
+  // |page_id_| needs to be set to kHighestInvalidPageId if the read failed.
   // Otherwise, future ReadPage() calls with the previous |page_id_| value
   // would return SQLITE_OK, but the page data buffer might be trashed.
-  page_id_ = (sqlite_status == SQLITE_OK) ? page_id : kInvalidPageId;
+  page_id_ = (sqlite_status == SQLITE_OK) ? page_id : kHighestInvalidPageId;
   return sqlite_status;
 }
 
@@ -72,10 +73,10 @@ int DatabasePageReader::RawRead(sqlite3_file* sqlite_file,
                                 int read_size,
                                 int64_t read_offset,
                                 uint8_t* result_buffer) {
-  DCHECK(sqlite_file != nullptr);
-  DCHECK_GE(read_size, 0);
-  DCHECK_GE(read_offset, 0);
-  DCHECK(result_buffer != nullptr);
+  CHECK(sqlite_file != nullptr);
+  CHECK_GE(read_size, 0);
+  CHECK_GE(read_offset, 0);
+  CHECK(result_buffer != nullptr);
 
   // Retry the I/O operations a few times if they fail. This is especially
   // useful when recovering from database corruption.

sql/recover_module/pager.h

@@ -29,8 +29,9 @@ class VirtualTable;
 // cursors. Instances are not thread-safe.
 class DatabasePageReader {
  public:
-  // Guaranteed to be an invalid page number.
-  static constexpr int kInvalidPageId = 0;
+  // Guaranteed to be an invalid page number. NB: use `IsValidPageId()` to
+  // validate a page id.
+  static constexpr int kHighestInvalidPageId = 0;
 
   // Minimum database page size supported by SQLite.
   static constexpr int kMinPageSize = 512;
@@ -70,7 +71,7 @@ class DatabasePageReader {
   // ReadPage() was never called.
   const uint8_t* page_data() const {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-    DCHECK_NE(page_id_, kInvalidPageId)
+    CHECK(IsValidPageId(page_id_))
         << "Successful ReadPage() required before accessing pager state";
     return page_data_.get();
   }
@@ -86,10 +87,10 @@ class DatabasePageReader {
   // ReadPage() was never called.
   int page_size() const {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-    DCHECK_NE(page_id_, kInvalidPageId)
+    CHECK(IsValidPageId(page_id_))
         << "Successful ReadPage() required before accessing pager state";
-    DCHECK_GE(page_size_, kMinUsablePageSize);
-    DCHECK_LE(page_size_, kMaxPageSize);
+    CHECK_GE(page_size_, kMinUsablePageSize);
+    CHECK_LE(page_size_, kMaxPageSize);
     return page_size_;
   }
 
@@ -99,7 +100,7 @@ class DatabasePageReader {
   // ReadPage() was never called.
   int page_id() const {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-    DCHECK_NE(page_id_, kInvalidPageId)
+    CHECK(IsValidPageId(page_id_))
         << "Successful ReadPage() required before accessing pager state";
     return page_id_;
   }
@@ -122,7 +123,7 @@ class DatabasePageReader {
   //
   // Valid page IDs are positive 32-bit integers.
   static constexpr bool IsValidPageId(int64_t page_id) noexcept {
-    return page_id > kInvalidPageId && page_id <= kMaxPageId;
+    return page_id > kHighestInvalidPageId && page_id <= kMaxPageId;
   }
 
   // Low-level read wrapper. Returns a SQLite error code.
@@ -135,8 +136,8 @@ class DatabasePageReader {
 
  private:
   // Points to the last page successfully read by ReadPage().
-  // Set to kInvalidPageId if the last read was unsuccessful.
-  int page_id_ = kInvalidPageId;
+  // Set to kHighestInvalidPageId if the last read was unsuccessful.
+  int page_id_ = kHighestInvalidPageId;
   // Stores the bytes of the last page successfully read by ReadPage().
   // The content is undefined if the last call to ReadPage() did not succeed.
   const std::unique_ptr<uint8_t[]> page_data_;

sql/recover_module/payload.cc

@@ -125,7 +125,7 @@ int MaxOverflowPayloadSize(int page_size) {
 }  // namespace
 
 LeafPayloadReader::LeafPayloadReader(DatabasePageReader* db_reader)
-    : db_reader_(db_reader), page_id_(DatabasePageReader::kInvalidPageId) {}
+    : db_reader_(db_reader) {}
 
 LeafPayloadReader::~LeafPayloadReader() = default;
 
@@ -194,7 +194,7 @@ bool LeafPayloadReader::Initialize(int64_t payload_size, int payload_offset) {
       page_size) {
     // Corruption can result in overly large payload sizes. Reject the obvious
     // case where the in-page payload extends past the end of the page.
-    page_id_ = DatabasePageReader::kInvalidPageId;
+    page_id_ = DatabasePageReader::kHighestInvalidPageId;
     return false;
   }
 
@@ -205,7 +205,7 @@ bool LeafPayloadReader::Initialize(int64_t payload_size, int payload_offset) {
 
 bool LeafPayloadReader::IsInitialized() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return page_id_ != DatabasePageReader::kInvalidPageId;
+  return DatabasePageReader::IsValidPageId(page_id_);
 }
 
 bool LeafPayloadReader::ReadPayload(int64_t offset,
@@ -220,7 +220,7 @@ bool LeafPayloadReader::ReadPayload(int64_t offset,
   DCHECK(buffer != nullptr);
 
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(page_id_ != DatabasePageReader::kInvalidPageId)
+  DCHECK(IsInitialized())
       << "Initialize() not called, or last call did not succeed";
 
   if (offset < inline_payload_size_) {
@@ -337,4 +337,4 @@ bool LeafPayloadReader::PopulateNextOverflowPageId() {
 }
 
 }  // namespace recover
-}  // namespace sql
+}  // namespace sql

sql/recover_module/payload.h

@@ -60,7 +60,7 @@ class LeafPayloadReader {
   // payload_size().
   int inline_payload_size() const {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-    DCHECK(page_id_ != DatabasePageReader::kInvalidPageId)
+    DCHECK(IsInitialized())
         << "Initialize() not called, or last call did not succeed";
     DCHECK_LE(inline_payload_size_, payload_size_);
     return inline_payload_size_;
@@ -74,7 +74,7 @@ class LeafPayloadReader {
   // The return value is guaranteed to be at least inline_payload_size().
   int payload_size() const {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-    DCHECK(page_id_ != DatabasePageReader::kInvalidPageId)
+    DCHECK(IsInitialized())
         << "Initialize() not called, or last call did not succeed";
     DCHECK_LE(inline_payload_size_, payload_size_);
     return payload_size_;
@@ -116,8 +116,8 @@ class LeafPayloadReader {
 
   // The ID of the B-tree page containing the current payload's inline bytes.
   //
-  // Set to kInvalidPageId if the reader wasn't successfully initialized.
-  int page_id_ = DatabasePageReader::kInvalidPageId;
+  // Set to kHighestInvalidPageId if the reader wasn't successfully initialized.
+  int page_id_ = DatabasePageReader::kHighestInvalidPageId;
 
   // The start of the current payload's inline bytes on the B-tree page.
   //