Commit 6fb10c6a authored by Marijn Kruisselbrink's avatar Marijn Kruisselbrink Committed by Commit Bot

[FileAPI] Ensure we're not trying to create blob with bogus data.

The file system provider API passed metadata back from the extensions
without any kind of sanitization. This means that for example "valid"
blobs with negative sizes could exist. We don't want to have to deal
with these weird beasts on the blob size, so add some assertions to the
blob code to make sure this doesn't happen, and sanitize negative sizes
on the file system provider side before passing them back.

This is part of a series of changes to improve consistency surrounding how
snapshot state of files/blobs is implemented.

Bug: 844874
Change-Id: Ie3dd1de9d49b071b615e969ec68529bf3074c875
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1976431Reviewed-by: default avatarDaniel Murphy <dmurph@chromium.org>
Reviewed-by: default avatarNaoki Fukino <fukino@chromium.org>
Reviewed-by: default avatarJeremy Roman <jbroman@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#728006}
parent 0710ebc3
...@@ -73,7 +73,7 @@ void OnGetFileInfo(int fields, ...@@ -73,7 +73,7 @@ void OnGetFileInfo(int fields,
if (fields & storage::FileSystemOperation::GET_METADATA_FIELD_IS_DIRECTORY) if (fields & storage::FileSystemOperation::GET_METADATA_FIELD_IS_DIRECTORY)
file_info.is_directory = *metadata->is_directory; file_info.is_directory = *metadata->is_directory;
if (fields & storage::FileSystemOperation::GET_METADATA_FIELD_SIZE) if (fields & storage::FileSystemOperation::GET_METADATA_FIELD_SIZE)
file_info.size = *metadata->size; file_info.size = std::max(int64_t{0}, *metadata->size);
if (fields & storage::FileSystemOperation::GET_METADATA_FIELD_LAST_MODIFIED) { if (fields & storage::FileSystemOperation::GET_METADATA_FIELD_LAST_MODIFIED) {
file_info.last_modified = *metadata->modification_time; file_info.last_modified = *metadata->modification_time;
......
...@@ -1481,8 +1481,10 @@ TEST(V8ScriptValueSerializerTest, RoundTripFileNonNativeSnapshot) { ...@@ -1481,8 +1481,10 @@ TEST(V8ScriptValueSerializerTest, RoundTripFileNonNativeSnapshot) {
// Preserving behavior, filesystem URL is not preserved across cloning. // Preserving behavior, filesystem URL is not preserved across cloning.
V8TestingScope scope; V8TestingScope scope;
KURL url("filesystem:http://example.com/isolated/hash/non-native-file"); KURL url("filesystem:http://example.com/isolated/hash/non-native-file");
FileMetadata metadata;
metadata.length = 0;
File* file = File* file =
File::CreateForFileSystemFile(url, FileMetadata(), File::kIsUserVisible); File::CreateForFileSystemFile(url, metadata, File::kIsUserVisible);
v8::Local<v8::Value> wrapper = ToV8(file, scope.GetScriptState()); v8::Local<v8::Value> wrapper = ToV8(file, scope.GetScriptState());
v8::Local<v8::Value> result = RoundTrip(wrapper, scope); v8::Local<v8::Value> result = RoundTrip(wrapper, scope);
ASSERT_TRUE(V8File::HasInstance(result, scope.GetIsolate())); ASSERT_TRUE(V8File::HasInstance(result, scope.GetIsolate()));
......
...@@ -128,8 +128,10 @@ TEST_F(DataObjectTest, fileSystemId) { ...@@ -128,8 +128,10 @@ TEST_F(DataObjectTest, fileSystemId) {
data_object_->AddFilename(file_path, String(), String()); data_object_->AddFilename(file_path, String(), String());
data_object_->AddFilename(file_path, String(), "fileSystemIdForFilename"); data_object_->AddFilename(file_path, String(), "fileSystemIdForFilename");
FileMetadata metadata;
metadata.length = 0;
data_object_->Add( data_object_->Add(
File::CreateForFileSystemFile(url, FileMetadata(), File::kIsUserVisible), File::CreateForFileSystemFile(url, metadata, File::kIsUserVisible),
"fileSystemIdForFileSystemFile"); "fileSystemIdForFileSystemFile");
ASSERT_EQ(3U, data_object_->length()); ASSERT_EQ(3U, data_object_->length());
......
...@@ -271,9 +271,9 @@ File::File(const KURL& file_system_url, ...@@ -271,9 +271,9 @@ File::File(const KURL& file_system_url,
name_(DecodeURLEscapeSequences(file_system_url.LastPathComponent(), name_(DecodeURLEscapeSequences(file_system_url.LastPathComponent(),
DecodeURLMode::kUTF8OrIsomorphic)), DecodeURLMode::kUTF8OrIsomorphic)),
file_system_url_(file_system_url), file_system_url_(file_system_url),
snapshot_size_(metadata.length),
snapshot_modification_time_(metadata.modification_time) { snapshot_modification_time_(metadata.modification_time) {
if (metadata.length >= 0) DCHECK_GE(metadata.length, 0);
snapshot_size_ = metadata.length;
} }
File::File(const File& other) File::File(const File& other)
......
...@@ -42,8 +42,10 @@ TEST(FileListTest, pathsForUserVisibleFiles) { ...@@ -42,8 +42,10 @@ TEST(FileListTest, pathsForUserVisibleFiles) {
{ {
KURL url( KURL url(
"filesystem:http://example.com/isolated/hash/visible-non-native-file"); "filesystem:http://example.com/isolated/hash/visible-non-native-file");
file_list->Append(File::CreateForFileSystemFile(url, FileMetadata(), FileMetadata metadata;
File::kIsUserVisible)); metadata.length = 0;
file_list->Append(
File::CreateForFileSystemFile(url, metadata, File::kIsUserVisible));
} }
// Not user visible file system URL file. // Not user visible file system URL file.
...@@ -51,8 +53,10 @@ TEST(FileListTest, pathsForUserVisibleFiles) { ...@@ -51,8 +53,10 @@ TEST(FileListTest, pathsForUserVisibleFiles) {
KURL url( KURL url(
"filesystem:http://example.com/isolated/hash/" "filesystem:http://example.com/isolated/hash/"
"not-visible-non-native-file"); "not-visible-non-native-file");
file_list->Append(File::CreateForFileSystemFile(url, FileMetadata(), FileMetadata metadata;
File::kIsNotUserVisible)); metadata.length = 0;
file_list->Append(
File::CreateForFileSystemFile(url, metadata, File::kIsNotUserVisible));
} }
Vector<base::FilePath> paths = file_list->PathsForUserVisibleFiles(); Vector<base::FilePath> paths = file_list->PathsForUserVisibleFiles();
......
...@@ -214,6 +214,7 @@ TEST(FileTest, FileSystemFileWithApocalypseTimestamp) { ...@@ -214,6 +214,7 @@ TEST(FileTest, FileSystemFileWithApocalypseTimestamp) {
TEST(FileTest, fileSystemFileWithoutNativeSnapshot) { TEST(FileTest, fileSystemFileWithoutNativeSnapshot) {
KURL url("filesystem:http://example.com/isolated/hash/non-native-file"); KURL url("filesystem:http://example.com/isolated/hash/non-native-file");
FileMetadata metadata; FileMetadata metadata;
metadata.length = 0;
File* const file = File* const file =
File::CreateForFileSystemFile(url, metadata, File::kIsUserVisible); File::CreateForFileSystemFile(url, metadata, File::kIsUserVisible);
EXPECT_FALSE(file->HasBackingFile()); EXPECT_FALSE(file->HasBackingFile());
...@@ -239,6 +240,7 @@ TEST(FileTest, hsaSameSource) { ...@@ -239,6 +240,7 @@ TEST(FileTest, hsaSameSource) {
KURL url_a("filesystem:http://example.com/isolated/hash/non-native-file-A"); KURL url_a("filesystem:http://example.com/isolated/hash/non-native-file-A");
KURL url_b("filesystem:http://example.com/isolated/hash/non-native-file-B"); KURL url_b("filesystem:http://example.com/isolated/hash/non-native-file-B");
FileMetadata metadata; FileMetadata metadata;
metadata.length = 0;
File* const file_system_file_a1 = File* const file_system_file_a1 =
File::CreateForFileSystemFile(url_a, metadata, File::kIsUserVisible); File::CreateForFileSystemFile(url_a, metadata, File::kIsUserVisible);
File* const file_system_file_a2 = File* const file_system_file_a2 =
......
...@@ -103,7 +103,7 @@ RawData::RawData() = default; ...@@ -103,7 +103,7 @@ RawData::RawData() = default;
BlobData::BlobData(FileCompositionStatus composition) BlobData::BlobData(FileCompositionStatus composition)
: file_composition_(composition) {} : file_composition_(composition) {}
BlobData::~BlobData() {} BlobData::~BlobData() = default;
Vector<mojom::blink::DataElementPtr> BlobData::ReleaseElements() { Vector<mojom::blink::DataElementPtr> BlobData::ReleaseElements() {
return std::move(elements_); return std::move(elements_);
...@@ -173,6 +173,7 @@ void BlobData::AppendFile( ...@@ -173,6 +173,7 @@ void BlobData::AppendFile(
"create a blob with a single file with unknown size, use " "create a blob with a single file with unknown size, use "
"BlobData::createForFileWithUnknownSize. Otherwise please provide the " "BlobData::createForFileWithUnknownSize. Otherwise please provide the "
"file size."; "file size.";
DCHECK_GE(length, 0);
// Skip zero-byte items, as they don't matter for the contents of the blob. // Skip zero-byte items, as they don't matter for the contents of the blob.
if (length == 0) if (length == 0)
return; return;
...@@ -201,6 +202,7 @@ void BlobData::AppendFileSystemURL( ...@@ -201,6 +202,7 @@ void BlobData::AppendFileSystemURL(
const base::Optional<base::Time>& expected_modification_time) { const base::Optional<base::Time>& expected_modification_time) {
DCHECK_EQ(file_composition_, FileCompositionStatus::NO_UNKNOWN_SIZE_FILES) DCHECK_EQ(file_composition_, FileCompositionStatus::NO_UNKNOWN_SIZE_FILES)
<< "Blobs with a unknown-size file cannot have other items."; << "Blobs with a unknown-size file cannot have other items.";
DCHECK_GE(length, 0);
// Skip zero-byte items, as they don't matter for the contents of the blob. // Skip zero-byte items, as they don't matter for the contents of the blob.
if (length == 0) if (length == 0)
return; return;
...@@ -359,8 +361,7 @@ BlobDataHandle::BlobDataHandle( ...@@ -359,8 +361,7 @@ BlobDataHandle::BlobDataHandle(
DCHECK(blob_remote_.is_valid()); DCHECK(blob_remote_.is_valid());
} }
BlobDataHandle::~BlobDataHandle() { BlobDataHandle::~BlobDataHandle() = default;
}
mojo::PendingRemote<mojom::blink::Blob> BlobDataHandle::CloneBlobRemote() { mojo::PendingRemote<mojom::blink::Blob> BlobDataHandle::CloneBlobRemote() {
MutexLocker locker(blob_remote_mutex_); MutexLocker locker(blob_remote_mutex_);
......
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