SQLite now uses SQLITE_FCNTL_LAST_ERRNO to get the LastErrno, so we
should update the Chromium SQL code to use this now, since the previous
name is a deprecated name.
Also update the comment in the header to refer reader to implementation
rather than documentation since documentation is nonexistent.
Bug: 40272342
Change-Id: I6f63f0bb8fcde1cfa32db705b5b76667dd1cef17
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5414914
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Vikram Pasupathy <vpasupathy@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1281428}
When attempting a recovery, if the meta table is corrupted then the
`key` column can be missing, leading to errors like:
SQL compilation error: table meta has no column named key.
Statement: INSERT OR REPLACE INTO meta(key,value) VALUES(?,?)
Current code goes on to attempt to run this statement anyway, which
leads to a fatal DLOG. Intercept this problem earlier for a more
graceful exit.
Bug: 331970403
Change-Id: I3c20d0fdcdbd927df22011c9a4f6d27e96293d42
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5408209
Reviewed-by: Nathan Memmott <memmott@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1280898}
I wanted to write a DCHECK death test where the Database is destroyed on
an unexpected sequence, but had trouble getting EXPECT_DCHECK_DEATH_WITH
to capture the crash.
Bug: 326498393
Change-Id: Ie28624addfe329d7867446806a01957c321cd81e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5333056
Commit-Queue: Dan McArdle <dmcardle@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1267050}
Now, Transaction checks whether the Database has been destroyed before
using it.
This CL also adds stricter sequence checking to Database, which found a
few places that Database was passed between sequences:
(1) storage::DatabaseTracker was using and destroying sql::Database on
different sequences. Now, it simply destroys the Database instance
in storage::DatabaseTracker::Shutdown(), which already runs on the
correct sequence.
(1) history::InMemoryDatabase was "slurping" data from disk on one
sequence, then querying the data on a different sequence. To support
this use case, we added added sql:Database::DetachFromSequence().
Now, InMemoryDatabase effectively annotates the cut point where the
Database is handed off to another sequence.
Bug: 326498393
Change-Id: I866061feaf08d48607d97731a512bc02ce497f71
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5320373
Reviewed-by: manuk hovanesian <manukh@chromium.org>
Commit-Queue: Dan McArdle <dmcardle@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Auto-Submit: Dan McArdle <dmcardle@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1267047}
WriteBigEndian() is converted to the byte_conversions functions
in base::numerics or into a BigEndianWriter.
BigEndianWriter now uses a span internally, and can be constructed
from a span.
Bug: 1490484
Change-Id: I4d6726651e4c6eece2cb8d8c74bdf766d86a8beb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5202348
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Owners-Override: danakj <danakj@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1266703}
For some uses cases, POSIX_FADV_SEQUENTIAL is substantially faster than
POSIX_FADV_WILLNEED. This change adds an option to base::PreReadFile()
to hint at sequential file reading. This was not added as the default
since it looks like other callers may not be doing sequential reads
(e.g. database files).
Bug: 326240401
Change-Id: I578b277afc99f2bd1725a79161609e0471800cfa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5325390
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: John Rummell <jrummell@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1266208}
Now that Windows 10 is the minimum requirement, PrefetchVirtualMemory()
is always available. Thus there is no need for the PreReadFileSlow()
fallback on Windows. Make the PreReadFileSlow() fallback POSIX-only, and
remove the Windows-specific bits within its implementation.
Update the comments for one of the PreReadFile() callers to stop
mentioning Windows 7.
Bug: 40246975
Change-Id: I0dbd87d17ba37303eef095534249a92acdad2990
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5321084
Reviewed-by: Will Harris <wfh@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1265553}
This is is fixing some issues with unittests that were not supporting
the exclusive mode.
The default Database constructor was assuming non-exclusive mode which
is not aligned with the explicit constructor.
We do recommend to use exclusive mode for performance/stability (unless there is a valid reason otherwise).
Bug: 1120969
Change-Id: I114a60c1ee290789fc8f3d2f2ea69cba2843a283
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5029223
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Reviewed-by: Andrew Paseltiner <apaseltiner@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1254628}
The bespoke recovery code is now disabled on ToT. Although that code is
not yet fully removed in case we need to temporarily revert the
migration, the fuzzer for the old code has been ignored for some months
now and we can save bot cycles by removing it.
Fixed: 1386218,1417151,1429058,1513310
Change-Id: I8f0cb7cae0a90b4800c537ea9c18073ed454fe81
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5244874
Reviewed-by: Austin Sullivan <asully@chromium.org>
Auto-Submit: Evan Stade <estade@chromium.org>
Commit-Queue: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1253352}
This CL adds an early return to Database::OpenInternal() in the event
that "PRAGMA journal_mode=TRUNCATE" fails. This prevents a null
dereference down the line in Database::GetSqliteVfsFile().
It also adds a comment that explains some of the historical context and
reasoning to help out future code archaeologists.
Test: Run sql_built_in_recovery_lpm_fuzzer on the fuzzer testcase
Change-Id: I50d75bcf61a4a6dde4edc098fb8434b69957eac3
Fixed: b/321842458
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5232014
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Ayu Ishii <ayui@chromium.org>
Commit-Queue: Ayu Ishii <ayui@chromium.org>
Auto-Submit: Dan McArdle <dmcardle@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1252811}
//sql uses a wrapper around the default VFS to accomplish certain
custom, OS-specific tasks (see vfs_wrapper.h). This wrapper is used
in `sql::Database::Open()` but is not used in other contexts such as
`sql::Database::Recover()` or `sql::Database::Delete()`.
This change makes that wrapper the default VFS so it's used
automatically for all SQLite library calls that don't specify a VFS
instead of just sqlite3_open_v2.
Bug: 1385500
Change-Id: I9b8fdba9897f961605914a0629beb32c1c518454
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5234121
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1252429}
The fuzzer was accidentally reusing the `queries` field of its input
protobuf and ignoring the `queries_after_open` field of
`RecoveryFuzzerTestCase` (see sql_disk_corruption.proto).
This is a behavior change and will likely cause Clusterfuzz to mark its
existing crbugs as fixed. For the ones that we care about, we can
trivially create the equivalent new test case by copying the value of
the `queries` field into the `queries_after_open` field. It's also
likely the Clusterfuzz will rediscover the same crashes because I assume
copying a field is not a complex mutation.
Bug: 1520447
Change-Id: I07ddd3fa94fd5860adb615ffe535d0b1876ea8be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5225482
Reviewed-by: Ayu Ishii <ayui@chromium.org>
Auto-Submit: Dan McArdle <dmcardle@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Ayu Ishii <ayui@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1251721}
We already have sql_built_in_recovery_fuzzer, but anecdotally I never
see it reach recovery internals, e.g. Recovery::Init(). I think the main
issue is that its approach is to treat the fuzzer input as a SQLite
file.
I think there are two impediments to sql_built_in_recovery_fuzzer's
fuzzing speed and coverage:
(1) The database file must have high fidelity or else it won't exercise
interesting paths in the recovery module.
The new fuzzer addresses the fidelity issue by bootstrapping the
database file with some syntactically-correct, fuzzer-generated SQL
statements, then corrupting parts of that file with fuzzer-generated
"mutations".
(2) It doesn't even attempt recovery unless Database::Open() saw an
egregious-enough error.
The new fuzzer attempts recovery when *any* error occurs.
Running sql_built_in_recovery_lpm_fuzzer locally, I see ~20 exec/s,
which is admittedly slower than sql_built_in_recovery_fuzzer, which runs
at ~50 exec/s. However, I believe it's covering more code per execution.
If I add logging statements to Recovery::Init(), it finds it in just a
few seconds.
Change-Id: I49903246ea66f528f76ee2cc912623d12d700d39
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5153917
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Ayu Ishii <ayui@chromium.org>
Commit-Queue: Dan McArdle <dmcardle@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1249516}
DLOG(DCHECK) is just confusing, and in most cases it means DLOG(FATAL)
anyways. DLOGs are enabled in DCHECK-enabled builds.
If we wanted DLOG(DCHECK) to not be fatal, we should probably make
DLOG(FATAL) and LOG(DFATAL) non-fatal as well under the same build
conditions.
This removes the ability to use LOG(DCHECK) as well.
Bug: None
Change-Id: I7fc31ee13d7f2f57591e3caba76b26af21d819d8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5201856
Commit-Queue: Peter Boström <pbos@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Owners-Override: danakj <danakj@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Auto-Submit: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1249112}
Chrome tries to propagate TimeMachine exclusions from the main
database file to its journal file. In some cases, the database
file doesn't exist, which causes a DCHECK. This change papers
over that by verifying the database file exists before calling
base utilities on it.
It's not clear if the existence of a -journal file without
the database file is an error case or can happen normally.
If it is an error case, then we rely on other SQLite code to
notice and error out (but not crash). If it is not an error
case, then the worst thing that will happen due to this change
is that a -journal file may not be excluded from TimeMachine.
The journal file is much smaller than the database file, so
this outcome, while not ideal, is hardly terrible.
crbug.com/25959 has the original justification for this
behavior, and in that example the journal files are less than
2% of the size of the database files.
Bug: 1493504
Change-Id: Ib15a97572b46f54c03699568cf46999cb80c78aa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5187142
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1245920}
The legacy recovery module does not support WAL databases. We'd like to
enable WAL mode more broadly, which increases the urgency of our
migration to the new recovery module
Bug: 1385500
Change-Id: I61da303c356923e53da0f1ed4c319ee6c39edaed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5173125
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1244889}
A new schema is introduced into CdmStorageDatabase to add last_modified
and file_size to be able to support time-based deletion and time-based
usage calculation.
Methods were implemented to delete by time frame, by storage key, and
to calculate file size using SQL queries for time frames by storing
the file size into the CdmStorageDatabase to be a one time operation.
This CL also provides the functionality that automatically deletes the
database if a Delete function for a subset (i.e file, storage key,
time-frame) ends up removing entries such that there are no more entries
in the database.
An SQL statement is used to alter the version1 database so that we don't
run into crashes when a user that already has the CdmStorageDatabase
v1.0 in their profile runs into queries that are made for the v2.0, such
as writing into the column file_size, since file_size is only in v2.0.
Testing: unittests +
Manual Testing Steps for the Schema Alter code:
1. Build chrome on main branch.
2. Go to chrome://flags and enable kCdmStorageDatabase and
kCdmStorageDatabaseMigration.
3. Open a new profile, name it CdmStorageDatabase (or anything you want)
4. Play this website: https://integration.staging.widevine.com/player?contentUrl=https://storage.googleapis.com/wvtemp/hmchen/poc/tears_hd_cenc.mpd&proxyServerUrl=https://proxy.uat.widevine.com/proxy?video_id=GTS_CAN_PLAY_CLIENT_TOKEN%26provider=widevine_test%26full_response=true&persistentStateRequired=true
5. Log the kVersionNumber (should be 1) (I did this through LOG(ERROR)).
6. A value should be written.
7. Quit Chrome.
8. Switch your branch to these changes using git and build chrome.
9. Open chrome, verify the same flags are still enabled, and open
the same profile you used as before.
10. Play same website as step #4.
11. log the kVersionNumber (should be 2) (I did this through LOG(ERROR))
12. View in chrome://histograms/Media.EME that the
IncompatibleDatabaseDetected histogram was triggered.
13. Verify that playback plays continuously and Chrome does not crash.
OBSOLETE_HISTOGRAMS= Media.EME.CdmStorageManager.DeleteDatabaseError.Incognito, Media.EME.CdmStorageManager.DeleteDatabaseError.NonIncognito.
Bug: 1454512
Change-Id: I64f9dff580114545ececf5d7b4f03124b1f55192
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4994868
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Evan Liu <evliu@google.com>
Reviewed-by: John Rummell <jrummell@chromium.org>
Commit-Queue: Vikram Pasupathy <vpasupathy@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1242731}
Currently, Database::Open() incorrectly returns true when both attempts
at opening the database fail. We're chalking this behavior up to a
simple oversight in Database::OpenInternal()'s error handling. See the
linked crbug for more details.
This CL addresses the root cause by unifying Database::Open()'s error
handling and retry logic. In doing so, it resolves a preexisting TODO.
As a side effect, Database::Open() no longer invokes the error callback
6 times in the double-error scenario, which was why I originally filed
the crbug.
Bug: 1509891
Change-Id: Ib0b2f295a2932cf8612f74763db11896d11d6227
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5141149
Reviewed-by: Nan Lin <linnan@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Dan McArdle <dmcardle@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Angela Yoeurng <yoangela@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1242432}
This CL is an adding UMA metric for each database that represent
the amount of SQLite VM steps. This is a good proxy metrics for
estimating the overhead of database' queries.
We are using VM Steps as a proxy instead of CPU time since most
of the databases code are executed on a background sequence; which
is highly affected by Thread priority and OS scheduling. VM Steps
should be stable and comparable between databases.
Bug: 1508662
Change-Id: Ibdf419c5c2b68e5aec9130b297235612f14bdd28
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5089633
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Reviewed-by: Ayu Ishii <ayui@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1238200}
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}
The corruptSizeInHeader function is used in unittests to
corrupt the database on disk.
If the database is opened in WAL, it requires a checkpoint to
ensures the data is moved from the journal to the actual database
(a.k.a sqlite3_wal_checkpoint_v2(...)).
This CL is ensuring the database is opened in exclusive mode;
which means there is no other opened instance of that database.
If there was instances, CorruptSizeInHeader(...) will fail and
return false. The corresponding unittests will fail and should be
fixed.
Bug: 1120969
Change-Id: Id0d05aef4a7a272effc54ec637b57962f91677a0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018152
Reviewed-by: Greg Thompson <grt@chromium.org>
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1224931}
Due to a C language bug, memcpy with nullptr is UB even when the count
is zero. sqlite3OsGetLastError calls xGetLastError with nullptr and 0.
The C++ STL functions do not have this bug, so use them instead. This
avoids a UBSan warning in some Blink test.
Bug: 1394755
Change-Id: I0a5c0b98f467a48eab66c0371393896ec11d60ac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5012992
Auto-Submit: David Benjamin <davidben@chromium.org>
Commit-Queue: Ayu Ishii <ayui@chromium.org>
Reviewed-by: Ayu Ishii <ayui@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1223759}
