Browse Source

fix: two possible FSA crashes (#45233)

* 5786874: Change Observer: Fix crash when navigating to new page

https://chromium-review.googlesource.com/c/chromium/src/+/5786874

* 5794141: Change Observer: Fix Get*PermissionGrant crash

https://chromium-review.googlesource.com/c/chromium/src/+/5794141
Shelley Vohr 3 months ago
parent
commit
fa5de40f86

+ 34 - 18
shell/browser/file_system_access/file_system_access_permission_context.cc

@@ -461,7 +461,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<PermissionGrantImpl> new_grant;
+  scoped_refptr<PermissionGrantImpl> grant;
 
   if (existing_grant && existing_grant->handle_type() != handle_type) {
     // |path| changed from being a directory to being a file or vice versa,
@@ -471,18 +471,21 @@ FileSystemAccessPermissionContext::GetReadPermissionGrant(
     existing_grant = nullptr;
   }
 
-  if (!existing_grant) {
-    new_grant = base::MakeRefCounted<PermissionGrantImpl>(
+  bool creating_new_grant = !existing_grant;
+  if (creating_new_grant) {
+    grant = base::MakeRefCounted<PermissionGrantImpl>(
         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:
@@ -494,7 +497,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:
@@ -502,7 +505,7 @@ FileSystemAccessPermissionContext::GetReadPermissionGrant(
     }
   }
 
-  return existing_grant;
+  return grant;
 }
 
 scoped_refptr<content::FileSystemAccessPermissionGrant>
@@ -516,7 +519,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<PermissionGrantImpl> new_grant;
+  scoped_refptr<PermissionGrantImpl> grant;
 
   if (existing_grant && existing_grant->handle_type() != handle_type) {
     // |path| changed from being a directory to being a file or vice versa,
@@ -526,23 +529,26 @@ FileSystemAccessPermissionContext::GetWritePermissionGrant(
     existing_grant = nullptr;
   }
 
-  if (!existing_grant) {
-    new_grant = base::MakeRefCounted<PermissionGrantImpl>(
+  bool creating_new_grant = !existing_grant;
+  if (creating_new_grant) {
+    grant = base::MakeRefCounted<PermissionGrantImpl>(
         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:
@@ -552,7 +558,7 @@ FileSystemAccessPermissionContext::GetWritePermissionGrant(
     }
   }
 
-  return existing_grant;
+  return grant;
 }
 
 bool FileSystemAccessPermissionContext::IsFileTypeDangerous(
@@ -869,12 +875,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);
       }