Skip to content

Commit

Permalink
Revert "SQLite: Set printf() precision limits in production."
Browse files Browse the repository at this point in the history
This reverts commit 389b0c0.

Reason for revert: https://crbug.com/1212989 -- Turns out printf() is used internally in SQLite's schema parsing, and precision limits break it.

Original change's description:
> SQLite: Set printf() precision limits in production.
>
> This CL sets an explicit precision limit on printf() according to the
> recommendations at
> https://www.sqlite.org/compile.html#printf_precision_limit.
>
> printf() is Web-exposed in WebSQL since Chrome 91, so we have a short
> window of opportunity to set limits, before Web properties start
> depending on the function. These limits reduce the chance that a bug in
> a site will accidentally OOM a renderer.
>
> Bug: 1211778
> Change-Id: Iff7221705761d5d5a1523051f4d6ca932da34492
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2911717
> Commit-Queue: Victor Costan <pwnall@chromium.org>
> Commit-Queue: Darwin Huang <huangdarwin@chromium.org>
> Auto-Submit: Victor Costan <pwnall@chromium.org>
> Reviewed-by: Darwin Huang <huangdarwin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#886041}

Bug: 1211778
Change-Id: I61292fe3467e10f28d57b01c77efcfb3516ff1f4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2917235
Auto-Submit: Victor Costan <pwnall@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#886323}
  • Loading branch information
pwnall authored and Chromium LUCI CQ committed May 25, 2021
1 parent 510e529 commit 1141e10
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 129 deletions.
41 changes: 0 additions & 41 deletions sql/sqlite_features_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -297,47 +297,6 @@ TEST_F(SQLiteFeaturesTest, CachedRegexp) {
EXPECT_EQ(7, s.ColumnInt(0));
}

TEST_F(SQLiteFeaturesTest, PrintfPrecision_WidthAboveLimit) {
sql::Statement statement(
db_.GetUniqueStatement("SELECT printf('%1001d', 1)"));
ASSERT_TRUE(statement.Step());
EXPECT_EQ(std::string(999, ' ') + "1", statement.ColumnString(0));
}

TEST_F(SQLiteFeaturesTest, PrintfPrecision_WidthAtLimit) {
sql::Statement statement(
db_.GetUniqueStatement("SELECT printf('%1000d', 1)"));
ASSERT_TRUE(statement.Step());
EXPECT_EQ(std::string(999, ' ') + "1", statement.ColumnString(0));
}

TEST_F(SQLiteFeaturesTest, PrintfPrecision_WidthBelowLimit) {
sql::Statement statement(db_.GetUniqueStatement("SELECT printf('%999d', 1)"));
ASSERT_TRUE(statement.Step());
EXPECT_EQ(std::string(998, ' ') + "1", statement.ColumnString(0));
}

TEST_F(SQLiteFeaturesTest, PrintfPrecision_PrecisionAboveLimit) {
sql::Statement statement(
db_.GetUniqueStatement("SELECT printf('%.1001d', 1)"));
ASSERT_TRUE(statement.Step());
EXPECT_EQ(std::string(999, '0') + "1", statement.ColumnString(0));
}

TEST_F(SQLiteFeaturesTest, PrintfPrecision_PrecisionAtLimit) {
sql::Statement statement(
db_.GetUniqueStatement("SELECT printf('%.1000d', 1)"));
ASSERT_TRUE(statement.Step());
EXPECT_EQ(std::string(999, '0') + "1", statement.ColumnString(0));
}

TEST_F(SQLiteFeaturesTest, PrintfPrecision_PrecisionBelowLimit) {
sql::Statement statement(
db_.GetUniqueStatement("SELECT printf('%.999d', 1)"));
ASSERT_TRUE(statement.Step());
EXPECT_EQ(std::string(998, '0') + "1", statement.ColumnString(0));
}

#if defined(OS_MAC)
// If a database file is marked to be excluded from Time Machine, verify that
// journal files are also excluded.
Expand Down
61 changes: 0 additions & 61 deletions third_party/blink/web_tests/storage/websql/printf-oom.html

This file was deleted.

43 changes: 16 additions & 27 deletions third_party/sqlite/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -56,44 +56,33 @@ config("common_sqlite3_compile_options") {
defines += [ "SQLITE_ENABLE_LOCKING_STYLE=0" ]
}

# The width and precision fields in SQLite's printf() function may easily lead
# to accidental OOMs.
#
# In production builds, we limit the fields to the value recommended at
# https://www.sqlite.org/compile.html#printf_precision_limit. Fuzzing builds
# use a larger limit, because our fuzzers use printf()'s ability to create
# large buffers to test edge cases in SQLite's buffer handling code.
#
# See also https://sqlite.org/printf.html
if (use_fuzzing_engine) {
defines += [ "SQLITE_PRINTF_PRECISION_LIMIT=128000000" ] # 128 MB
} else {
defines += [ "SQLITE_PRINTF_PRECISION_LIMIT=1000" ]
}

if (use_fuzzing_engine) {
# Limit max length of data blobs and queries to 128 MB for fuzzing builds.
if (using_sanitizer) {
defines += [
# Limit max length of data blobs and queries to 128 MB for fuzzing builds.
"SQLITE_MAX_LENGTH=128000000",
"SQLITE_MAX_SQL_LENGTH=128000000",
"SQLITE_PRINTF_PRECISION_LIMIT=1280000",
]

# During fuzz testing, valid SQL queries generated by fuzzing engine may
# lead to large memory allocations. If that happens, fuzzer reports an
# out-of-memory error. However, such errors are not valid bugs.
# To avoid hitting those irrelevant OOMs, we limit max number of memory
# pages, so fuzzer will not crash when reaching the limit.
defines += [
"SQLITE_MAX_PAGE_COUNT=16384",

# Used to deserialize a database from a libfuzzer-provided data blob.
# This is to fuzz SQLite's resilience to database corruption.
"SQLITE_ENABLE_DESERIALIZE",
]
# Apply this for fuzzing builds only, not for all builds with sanitizers.
if (use_fuzzing_engine) {
defines += [
"SQLITE_MAX_PAGE_COUNT=16384",

# Used to deserialize a database from a libfuzzer-provided data blob.
# This is to fuzz SQLite's resilience to database corruption.
"SQLITE_ENABLE_DESERIALIZE",
]

# The progress callback is used in fuzzing to cancel long-running queries
# so we don't spend too much time on them.
defines -= [ "SQLITE_OMIT_PROGRESS_CALLBACK" ]
# The progress callback is used in fuzzing to cancel long-running queries
# so we don't spend too much time on them.
defines -= [ "SQLITE_OMIT_PROGRESS_CALLBACK" ]
}
}

if (is_debug || dcheck_always_on) {
Expand Down

0 comments on commit 1141e10

Please sign in to comment.