Skip to content

Commit

Permalink
Add dump without crash to suspicious code.
Browse files Browse the repository at this point in the history 
Since this is the latest code that started to return
Status::kErrorCorrupted, let me add dump to confirm it is affected or
not.

Bug: 1423325
Change-Id: I17fdae312a71bc58e854ac51b7c59bc8e50fe7cb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4409463
Reviewed-by: Shunya Shishido <sisidovski@chromium.org>
Reviewed-by: Minoru Chikamune <chikamune@chromium.org>
Commit-Queue: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1128505}
  • Loading branch information
@yoshisatoyanagisawa
yoshisatoyanagisawa authored and Chromium LUCI CQ committed Apr 11, 2023
1 parent a7a36b3 commit 9da6e11
Showing 1 changed file with 69 additions and 9 deletions.
78 changes: 69 additions & 9 deletions components/services/storage/service_worker/service_worker_database.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "base/command_line.h"
#include "base/debug/crash_logging.h"
#include "base/debug/dump_without_crashing.h"
#include "base/files/file_util.h"
#include "base/location.h"
#include "base/logging.h"
Expand Down Expand Up @@ -248,8 +249,11 @@ ServiceWorkerDatabase::Status ParseId(const std::string& serialized,
int64_t* out) {
DCHECK(out);
int64_t id;
if (!base::StringToInt64(serialized, &id) || id < 0)
if (!base::StringToInt64(serialized, &id) || id < 0) {
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
return ServiceWorkerDatabase::Status::kErrorCorrupted;
}
*out = id;
return ServiceWorkerDatabase::Status::kOk;
}
Expand All @@ -262,12 +266,15 @@ ServiceWorkerDatabase::Status LevelDBStatusToServiceWorkerDBStatus(
return ServiceWorkerDatabase::Status::kErrorNotFound;
else if (status.IsIOError())
return ServiceWorkerDatabase::Status::kErrorIOError;
else if (status.IsCorruption())
else if (status.IsCorruption()) {
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
return ServiceWorkerDatabase::Status::kErrorCorrupted;
else if (status.IsNotSupportedError())
} else if (status.IsNotSupportedError()) {
return ServiceWorkerDatabase::Status::kErrorNotSupported;
else
} else {
return ServiceWorkerDatabase::Status::kErrorFailed;
}
}

int64_t AccumulateResourceSizeInBytes(
Expand Down Expand Up @@ -395,6 +402,9 @@ ServiceWorkerDatabase::GetStorageKeysWithRegistrations(
absl::optional<blink::StorageKey> key =
blink::StorageKey::Deserialize(key_str);
if (!key) {
// TODO(crbug.com/1423325): remove the code when the reason is
// clarified.
base::debug::DumpWithoutCrashing();
status = Status::kErrorCorrupted;
keys->clear();
break;
Expand Down Expand Up @@ -628,8 +638,11 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::ReadRegistration(
return status;

// ResourceRecord must contain the ServiceWorker's main script.
if (resources->empty())
if (resources->empty()) {
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
return Status::kErrorCorrupted;
}

return Status::kOk;
}
Expand Down Expand Up @@ -663,6 +676,8 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::ReadRegistrationStorageKey(
absl::optional<blink::StorageKey> parsed =
blink::StorageKey::Deserialize(value);
if (!parsed) {
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
status = Status::kErrorCorrupted;
HandleReadResult(FROM_HERE, status);
return status;
Expand Down Expand Up @@ -914,6 +929,8 @@ ServiceWorkerDatabase::UpdateResourceSha256Checksums(
std::set<int64_t> updated_resource_ids;
for (const auto& resource : resources) {
if (!updated_resource_ids.insert(resource->resource_id).second) {
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
// The database wrongly contains the same resource id.
return Status::kErrorCorrupted;
}
Expand Down Expand Up @@ -1337,6 +1354,9 @@ ServiceWorkerDatabase::ReadUserDataForAllRegistrationsByKeyPrefix(
std::string(1, service_worker_internals::kKeySeparator),
base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
if (parts.size() != 2) {
// TODO(crbug.com/1423325): remove the code when the reason is
// clarified.
base::debug::DumpWithoutCrashing();
status = Status::kErrorCorrupted;
user_data->clear();
break;
Expand Down Expand Up @@ -1404,8 +1424,11 @@ ServiceWorkerDatabase::DeleteUserDataForAllRegistrationsByKeyPrefix(
user_data_name_with_id,
std::string(1, service_worker_internals::kKeySeparator),
base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
if (parts.size() != 2)
if (parts.size() != 2) {
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
return Status::kErrorCorrupted;
}

int64_t registration_id;
status = ParseId(parts[1], &registration_id);
Expand Down Expand Up @@ -1716,8 +1739,11 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::ParseRegistrationData(
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(out);
ServiceWorkerRegistrationData data;
if (!data.ParseFromString(serialized))
if (!data.ParseFromString(serialized)) {
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
return Status::kErrorCorrupted;
}

GURL scope_url(data.scope_url());
GURL script_url(data.script_url());
Expand All @@ -1728,6 +1754,8 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::ParseRegistrationData(
DLOG(ERROR) << "Scope URL '" << data.scope_url() << "' and/or script url '"
<< data.script_url() << "' and/or the storage key's origin '"
<< key.origin() << "' are invalid or have mismatching origins.";
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
return Status::kErrorCorrupted;
}

Expand All @@ -1738,6 +1766,8 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::ParseRegistrationData(
DLOG(ERROR) << "Registration id " << data.registration_id()
<< " and/or version id " << data.version_id()
<< " is higher than the next available id.";
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
return Status::kErrorCorrupted;
}

Expand All @@ -1759,12 +1789,16 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::ParseRegistrationData(
DLOG(ERROR)
<< "has_fetch_handler must be true if fetch_handler_skippable_type"
<< " is set.";
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
return Status::kErrorCorrupted;
}
if (!ServiceWorkerRegistrationData_FetchHandlerSkippableType_IsValid(
data.fetch_handler_skippable_type())) {
DLOG(ERROR) << "Fetch handler type '"
<< data.fetch_handler_skippable_type() << "' is not valid.";
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
return Status::kErrorCorrupted;
}
switch (data.fetch_handler_skippable_type()) {
Expand Down Expand Up @@ -1817,6 +1851,8 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::ParseRegistrationData(
auto value = data.script_type();
if (!ServiceWorkerRegistrationData_ServiceWorkerScriptType_IsValid(value)) {
DLOG(ERROR) << "Worker script type '" << value << "' is not valid.";
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
return Status::kErrorCorrupted;
}
(*out)->script_type = static_cast<blink::mojom::ScriptType>(value);
Expand All @@ -1832,6 +1868,8 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::ParseRegistrationData(
if (!ServiceWorkerRegistrationData_ServiceWorkerUpdateViaCacheType_IsValid(
value)) {
DLOG(ERROR) << "Update via cache mode '" << value << "' is not valid.";
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
return Status::kErrorCorrupted;
}
(*out)->update_via_cache =
Expand All @@ -1848,6 +1886,8 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::ParseRegistrationData(
DLOG(ERROR)
<< "Cross origin embedder policy in policy container policies '"
<< data.cross_origin_embedder_policy_value() << "' is not valid.";
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
return Status::kErrorCorrupted;
}
switch (data.cross_origin_embedder_policy_value()) {
Expand Down Expand Up @@ -1906,6 +1946,8 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::ParseRegistrationData(
data.ancestor_frame_type())) {
DLOG(ERROR) << "Ancestor frame type '" << data.ancestor_frame_type()
<< "' is not valid.";
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
return Status::kErrorCorrupted;
}
switch (data.ancestor_frame_type()) {
Expand All @@ -1931,6 +1973,9 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::ParseRegistrationData(
policies.referrer_policy())) {
DLOG(ERROR) << "Referrer policy in policy container policies '"
<< policies.referrer_policy() << "' is not valid.";
// TODO(crbug.com/1423325): remove the code when the reason is
// clarified.
base::debug::DumpWithoutCrashing();
return Status::kErrorCorrupted;
}
(*out)->policy_container_policies->referrer_policy =
Expand All @@ -1947,6 +1992,9 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::ParseRegistrationData(
policies.ip_address_space())) {
DLOG(ERROR) << "IP address space in policy container policies '"
<< policies.ip_address_space() << "' is not valid.";
// TODO(crbug.com/1423325): remove the code when the reason is
// clarified.
base::debug::DumpWithoutCrashing();
return Status::kErrorCorrupted;
}
(*out)->policy_container_policies->ip_address_space =
Expand Down Expand Up @@ -2179,6 +2227,8 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::ReadResourceRecords(
// |resources| should contain the main script.
if (!has_main_resource) {
resources->clear();
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
status = Status::kErrorCorrupted;
}

Expand All @@ -2192,16 +2242,24 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::ParseResourceRecord(
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(out);
ServiceWorkerResourceRecord record;
if (!record.ParseFromString(serialized))
if (!record.ParseFromString(serialized)) {
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
return Status::kErrorCorrupted;
}

GURL url(record.url());
if (!url.is_valid())
if (!url.is_valid()) {
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
return Status::kErrorCorrupted;
}

if (record.resource_id() >= next_avail_resource_id_) {
// The stored resource should not have a higher resource id than the next
// available resource id.
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
return Status::kErrorCorrupted;
}

Expand Down Expand Up @@ -2426,6 +2484,8 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::ReadDatabaseVersion(
if (!base::StringToInt64(value, db_version) ||
*db_version < kFirstValidVersion ||
service_worker_internals::kCurrentSchemaVersion < *db_version) {
// TODO(crbug.com/1423325): remove the code when the reason is clarified.
base::debug::DumpWithoutCrashing();
status = Status::kErrorCorrupted;
HandleReadResult(FROM_HERE, status);
return status;
Expand Down

0 comments on commit 9da6e11

Please sign in to comment.