Skip to content

Commit 68f0326

Browse files
tyrielvtyrielv
committed
Fix review findings: exception safety, FileShare, and seed TOCTOU
1. Restore exception safety net in InProcessMount.HandleRequest. Any unhandled exception in a pipe request handler was previously caught and logged; the refactoring accidentally removed this, causing mount process termination on transient I/O errors. 2. Open registry file with FileShare.ReadWrite | FileShare.Delete so concurrent WriteRegistry (write-to-temp + rename) succeeds while another process is reading. Required for multi-process model where no service serializes access. 3. Fix TOCTOU race in SeedFromSystemRegistryIfNeeded: re-check file existence before writing and merge with any entries created by a concurrent TryRegisterRepo call. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
1 parent 345be5f commit 68f0326

2 files changed

Lines changed: 82 additions & 50 deletions

File tree

‎GVFS/GVFS.Common/LocalRepoRegistry.cs‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -349,10 +349,29 @@ public void SeedFromSystemRegistryIfNeeded()
349349
}
350350
}
351351

352-
// Write accessible repos to user registry
352+
// Write accessible repos to user registry. Re-check whether the file
353+
// was created by another process (TOCTOU race) and merge if so.
353354
if (accessibleRepos.Count > 0)
354355
{
355-
this.WriteRegistry(accessibleRepos);
356+
if (this.fileSystem.FileExists(registryPath))
357+
{
358+
// Another process created the registry between our check and now.
359+
// Merge: read what's there, add our seeded entries (don't overwrite).
360+
Dictionary<string, LocalRepoRegistration> existingRepos = this.ReadRegistryFrom(this.registryDirectory);
361+
foreach (KeyValuePair<string, LocalRepoRegistration> entry in accessibleRepos)
362+
{
363+
if (!existingRepos.ContainsKey(entry.Key))
364+
{
365+
existingRepos[entry.Key] = entry.Value;
366+
}
367+
}
368+
369+
this.WriteRegistry(existingRepos);
370+
}
371+
else
372+
{
373+
this.WriteRegistry(accessibleRepos);
374+
}
356375

357376
EventMetadata metadata = new EventMetadata
358377
{
@@ -405,7 +424,7 @@ private Dictionary<string, LocalRepoRegistration> ReadRegistryFrom(string regist
405424
registryFilePath,
406425
FileMode.OpenOrCreate,
407426
FileAccess.Read,
408-
FileShare.Read,
427+
FileShare.ReadWrite | FileShare.Delete,
409428
callFlushFileBuffers: false))
410429
using (StreamReader reader = new StreamReader(stream))
411430
{

‎GVFS/GVFS.Mount/InProcessMount.cs‎

Lines changed: 60 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -475,68 +475,81 @@ private void HandleRequest(ITracer tracer, string request, NamedPipeServer.Conne
475475
{
476476
NamedPipeMessages.Message message = NamedPipeMessages.Message.FromString(request);
477477

478-
switch (message.Header)
478+
try
479479
{
480-
case NamedPipeMessages.GetStatus.Request:
481-
this.HandleGetStatusRequest(connection);
482-
break;
480+
switch (message.Header)
481+
{
482+
case NamedPipeMessages.GetStatus.Request:
483+
this.HandleGetStatusRequest(connection);
484+
break;
483485

484-
case NamedPipeMessages.Unmount.Request:
485-
this.HandleUnmountRequest(connection);
486-
break;
486+
case NamedPipeMessages.Unmount.Request:
487+
this.HandleUnmountRequest(connection);
488+
break;
487489

488-
case NamedPipeMessages.AcquireLock.AcquireRequest:
489-
this.HandleLockRequest(message.Body, connection);
490-
break;
490+
case NamedPipeMessages.AcquireLock.AcquireRequest:
491+
this.HandleLockRequest(message.Body, connection);
492+
break;
491493

492-
case NamedPipeMessages.ReleaseLock.Request:
493-
this.HandleReleaseLockRequest(message.Body, connection);
494-
break;
494+
case NamedPipeMessages.ReleaseLock.Request:
495+
this.HandleReleaseLockRequest(message.Body, connection);
496+
break;
495497

496-
case NamedPipeMessages.DownloadObject.DownloadRequest:
497-
this.HandleDownloadObjectRequest(message, connection);
498-
break;
498+
case NamedPipeMessages.DownloadObject.DownloadRequest:
499+
this.HandleDownloadObjectRequest(message, connection);
500+
break;
499501

500-
case NamedPipeMessages.ModifiedPaths.ListRequest:
501-
this.HandleModifiedPathsListRequest(message, connection);
502-
break;
502+
case NamedPipeMessages.ModifiedPaths.ListRequest:
503+
this.HandleModifiedPathsListRequest(message, connection);
504+
break;
503505

504-
case NamedPipeMessages.PostIndexChanged.NotificationRequest:
505-
this.HandlePostIndexChangedRequest(message, connection);
506-
break;
506+
case NamedPipeMessages.PostIndexChanged.NotificationRequest:
507+
this.HandlePostIndexChangedRequest(message, connection);
508+
break;
507509

508-
case NamedPipeMessages.PrepareForUnstage.Request:
509-
this.HandlePrepareForUnstageRequest(message, connection);
510-
break;
510+
case NamedPipeMessages.PrepareForUnstage.Request:
511+
this.HandlePrepareForUnstageRequest(message, connection);
512+
break;
511513

512-
case NamedPipeMessages.RunPostFetchJob.PostFetchJob:
513-
this.HandlePostFetchJobRequest(message, connection);
514-
break;
514+
case NamedPipeMessages.RunPostFetchJob.PostFetchJob:
515+
this.HandlePostFetchJobRequest(message, connection);
516+
break;
515517

516-
case NamedPipeMessages.DehydrateFolders.Dehydrate:
517-
this.HandleDehydrateFolders(message, connection);
518-
break;
518+
case NamedPipeMessages.DehydrateFolders.Dehydrate:
519+
this.HandleDehydrateFolders(message, connection);
520+
break;
519521

520-
case NamedPipeMessages.PrefetchCommits.Request:
521-
this.HandlePrefetchCommitsRequest(connection);
522-
break;
522+
case NamedPipeMessages.PrefetchCommits.Request:
523+
this.HandlePrefetchCommitsRequest(connection);
524+
break;
523525

524-
case NamedPipeMessages.PrefetchBlobs.RequestHeader:
525-
this.HandlePrefetchBlobsRequest(message, connection);
526-
break;
526+
case NamedPipeMessages.PrefetchBlobs.RequestHeader:
527+
this.HandlePrefetchBlobsRequest(message, connection);
528+
break;
527529

528-
case NamedPipeMessages.HydrationStatus.Request:
529-
this.HandleGetHydrationStatusRequest(connection);
530-
break;
530+
case NamedPipeMessages.HydrationStatus.Request:
531+
this.HandleGetHydrationStatusRequest(connection);
532+
break;
531533

532-
default:
533-
EventMetadata metadata = new EventMetadata();
534-
metadata.Add("Area", "Mount");
535-
metadata.Add("Header", message.Header);
536-
this.tracer.RelatedError(metadata, "HandleRequest: Unknown request");
534+
default:
535+
EventMetadata metadata = new EventMetadata();
536+
metadata.Add("Area", "Mount");
537+
metadata.Add("Header", message.Header);
538+
this.tracer.RelatedError(metadata, "HandleRequest: Unknown request");
537539

538-
connection.TrySendResponse(NamedPipeMessages.UnknownRequest);
539-
break;
540+
connection.TrySendResponse(NamedPipeMessages.UnknownRequest);
541+
break;
542+
}
543+
}
544+
catch (Exception e) when (e is not OutOfMemoryException)
545+
{
546+
EventMetadata metadata = new EventMetadata();
547+
metadata.Add("Area", "Mount");
548+
metadata.Add("Header", message.Header);
549+
metadata.Add("Exception", e.ToString());
550+
this.tracer.RelatedError(metadata, "HandleRequest: Unhandled exception in request handler");
551+
552+
connection.TrySendResponse(NamedPipeMessages.UnknownRequest);
540553
}
541554
}
542555

0 commit comments

Comments
 (0)