Commit e7cd3589 authored by Victor Costan's avatar Victor Costan Committed by Chromium LUCI CQ

sql: Fix potential integer multiplication overflow in database recovery.

Before this CL, DatabasePageReader::ReadPage() computed `read_offset`,
which is a byte offset in a SQLite database file, by multiplying two
ints, a page ID and a page size. The multiplication result is an int,
which may overflow. Even on 64-bit Unix platforms, int is 32-bit wide.

The overflow may produce a negative offset, which breaks the API
preconditions of SQLite's low-level reading functions. On Unix
platforms, the xRead() function called by ReadPage() is implemented by
unixRead(). When SQLite's mmap support is enabled (it is in Chrome),
passing a negative offset to xRead() causes it to memcpy() from an
invalid memory address.

This CL fixes the overflow by casting one of the multiplication inputs
to int64_t, which causes the multiplication result to be an int64_t.
This CL is likely to fix the attached bug, because a few sample crash
dumps are consistent with the hypothesis of a memcpy() from an invalid
offset.

Bug: 1015800
Change-Id: Ibc8e8dab52eef447acdb65d0ee9ddaeab1e160df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2605970
Auto-Submit: Victor Costan <pwnall@chromium.org>
Reviewed-by: default avatarDarwin Huang <huangdarwin@chromium.org>
Commit-Queue: Darwin Huang <huangdarwin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841034}
parent cf449d36
......@@ -50,8 +50,9 @@ int DatabasePageReader::ReadPage(int page_id) {
DCHECK_GE(page_size_, kMinUsablePageSize);
DCHECK_LE(page_size_, kMaxPageSize);
const int64_t read_offset = (page_id - 1) * page_size + page_offset;
static_assert((kMaxPageId - 1) * static_cast<int64_t>(kMaxPageSize) +
const int64_t read_offset =
static_cast<int64_t>(page_id - 1) * page_size + page_offset;
static_assert(static_cast<int64_t>(kMaxPageId - 1) * kMaxPageSize +
kDatabaseHeaderSize <=
std::numeric_limits<int64_t>::max(),
"The |read_offset| computation above may overflow");
......@@ -118,4 +119,4 @@ int DatabasePageReader::RawRead(sqlite3_file* sqlite_file,
}
} // namespace recover
} // namespace sql
\ No newline at end of file
} // namespace sql
......@@ -45,7 +45,7 @@ class DatabasePageReader {
// after the database header.
static constexpr int kMinUsablePageSize = kMinPageSize - kDatabaseHeaderSize;
// Maximum number of pages in a SQLite database.
// Largest valid page ID in a SQLite database.
//
// This is the maximum value of SQLITE_MAX_PAGE_COUNT plus 1, because page IDs
// start at 1. The numerical value, which is the same as
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment