From 2c22507dfb00906cd832fccff89ce75cc9bca399 Mon Sep 17 00:00:00 2001 From: "trop[bot]" <37223003+trop[bot]@users.noreply.github.com> Date: Mon, 20 Jan 2025 10:36:09 -0600 Subject: [PATCH] fix: two possible FSA crashes (#45260) * 5786874: Change Observer: Fix crash when navigating to new page https://chromium-review.googlesource.com/c/chromium/src/+/5786874 Co-authored-by: Shelley Vohr * 5794141: Change Observer: Fix Get*PermissionGrant crash https://chromium-review.googlesource.com/c/chromium/src/+/5794141 Co-authored-by: Shelley Vohr --------- Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr --- .../file_system_access_permission_context.cc | 52 ++++++++++++------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/shell/browser/file_system_access/file_system_access_permission_context.cc b/shell/browser/file_system_access/file_system_access_permission_context.cc index 46c9c3be1952..756aae63e5ed 100644 --- a/shell/browser/file_system_access/file_system_access_permission_context.cc +++ b/shell/browser/file_system_access/file_system_access_permission_context.cc @@ -459,7 +459,7 @@ FileSystemAccessPermissionContext::GetReadPermissionGrant( // but that is exactly what we want. auto& origin_state = active_permissions_map_[origin]; auto*& existing_grant = origin_state.read_grants[path_info.path]; - scoped_refptr new_grant; + scoped_refptr grant; if (existing_grant && existing_grant->handle_type() != handle_type) { // |path| changed from being a directory to being a file or vice versa, @@ -469,18 +469,21 @@ FileSystemAccessPermissionContext::GetReadPermissionGrant( existing_grant = nullptr; } - if (!existing_grant) { - new_grant = base::MakeRefCounted( + bool creating_new_grant = !existing_grant; + if (creating_new_grant) { + grant = base::MakeRefCounted( weak_factory_.GetWeakPtr(), origin, path_info, handle_type, GrantType::kRead, user_action); - existing_grant = new_grant.get(); + existing_grant = grant.get(); + } else { + grant = existing_grant; } // If a parent directory is already readable this new grant should also be // readable. - if (new_grant && + if (creating_new_grant && AncestorHasActivePermission(origin, path_info.path, GrantType::kRead)) { - existing_grant->SetStatus(PermissionStatus::GRANTED); + grant->SetStatus(PermissionStatus::GRANTED); } else { switch (user_action) { case UserAction::kOpen: @@ -492,7 +495,7 @@ FileSystemAccessPermissionContext::GetReadPermissionGrant( [[fallthrough]]; case UserAction::kDragAndDrop: // Drag&drop grants read access for all handles. - existing_grant->SetStatus(PermissionStatus::GRANTED); + grant->SetStatus(PermissionStatus::GRANTED); break; case UserAction::kLoadFromStorage: case UserAction::kNone: @@ -500,7 +503,7 @@ FileSystemAccessPermissionContext::GetReadPermissionGrant( } } - return existing_grant; + return grant; } scoped_refptr @@ -514,7 +517,7 @@ FileSystemAccessPermissionContext::GetWritePermissionGrant( // but that is exactly what we want. auto& origin_state = active_permissions_map_[origin]; auto*& existing_grant = origin_state.write_grants[path_info.path]; - scoped_refptr new_grant; + scoped_refptr grant; if (existing_grant && existing_grant->handle_type() != handle_type) { // |path| changed from being a directory to being a file or vice versa, @@ -524,23 +527,26 @@ FileSystemAccessPermissionContext::GetWritePermissionGrant( existing_grant = nullptr; } - if (!existing_grant) { - new_grant = base::MakeRefCounted( + bool creating_new_grant = !existing_grant; + if (creating_new_grant) { + grant = base::MakeRefCounted( weak_factory_.GetWeakPtr(), origin, path_info, handle_type, GrantType::kWrite, user_action); - existing_grant = new_grant.get(); + existing_grant = grant.get(); + } else { + grant = existing_grant; } // If a parent directory is already writable this new grant should also be // writable. - if (new_grant && + if (creating_new_grant && AncestorHasActivePermission(origin, path_info.path, GrantType::kWrite)) { - existing_grant->SetStatus(PermissionStatus::GRANTED); + grant->SetStatus(PermissionStatus::GRANTED); } else { switch (user_action) { case UserAction::kSave: // Only automatically grant write access for save dialogs. - existing_grant->SetStatus(PermissionStatus::GRANTED); + grant->SetStatus(PermissionStatus::GRANTED); break; case UserAction::kOpen: case UserAction::kDragAndDrop: @@ -550,7 +556,7 @@ FileSystemAccessPermissionContext::GetWritePermissionGrant( } } - return existing_grant; + return grant; } bool FileSystemAccessPermissionContext::IsFileTypeDangerous( @@ -864,12 +870,22 @@ void FileSystemAccessPermissionContext::RevokeActiveGrants( auto origin_it = active_permissions_map_.find(origin); if (origin_it != active_permissions_map_.end()) { OriginState& origin_state = origin_it->second; - for (auto& grant : origin_state.read_grants) { + for (auto grant_iter = origin_state.read_grants.begin(), + grant_end = origin_state.read_grants.end(); + grant_iter != grant_end;) { + // The grant may be removed from `read_grants`, so increase the iterator + // before continuing. + auto& grant = *(grant_iter++); if (file_path.empty() || grant.first == file_path) { grant.second->SetStatus(PermissionStatus::ASK); } } - for (auto& grant : origin_state.write_grants) { + for (auto grant_iter = origin_state.write_grants.begin(), + grant_end = origin_state.write_grants.end(); + grant_iter != grant_end;) { + // The grant may be removed from `write_grants`, so increase the iterator + // before continuing. + auto& grant = *(grant_iter++); if (file_path.empty() || grant.first == file_path) { grant.second->SetStatus(PermissionStatus::ASK); }