+1

This is going need a bit of reworking, but I think it is the correct approach. It looks like we'd need to elevate the Interop.ErrorInfo out of the loop and after the loop body move the throw Interop.GetExceptionForIoErrno(error, sharedMemoryFilePath); that is currently being preempted by the new continue logic.

Cool, I figured the same thing. Felt it was good to push this up as a placeholder

A different idea to consider. Please take it with a grain of salt as I don't have the context:

• when createIfNotExist is true, attempt in a loop until fd.IsValid:
  • create it with O_CREAT | O_EXCL. A success means we just created it and are the owner.
  • if creation failed with EEXIST, try to open it without creation
  • if opening without creation failed with ENOENT, it means it got removed in the meantime. Continue the loop
• attempt to open an existing file

Pseudocode:

UnixFileMode permissionsMask = id.IsUserScope
    ? PermissionsMask_OwnerUser_ReadWrite
    : PermissionsMask_AllUsers_ReadWrite;

const Interop.Sys.OpenFlags mandatoryFlags = Interop.Sys.OpenFlags.O_RDWR | Interop.Sys.OpenFlags.O_CLOEXEC
SafeFileHandle fd = new();
while (createIfNotExist && fd.IsInvalid)
{
    // Use O_EXCL which provides a guarantee that the file is created by this call.
    fd = Interop.Sys.Open(
        sharedMemoryFilePath,
        mandatoryFlags | Interop.Sys.OpenFlags.O_CREAT | Interop.Sys.OpenFlags.O_EXCL,
        (int)permissionsMask);

    if (fd.IsInvalid)
    {
        Interop.ErrorInfo error = Interop.Sys.GetLastErrorInfo();
        fd.Dispose();

        if (error.Error == Interop.Error.EEXIST)
        {
            fd = Interop.Sys.Open(sharedMemoryFilePath, mandatoryFlags, 0);
            if (fd.IsInvalid)
            {
                error = Interop.Sys.GetLastErrorInfo();
                fd.Dispose();

                // The file could have been deleted after the first open attempt, in which case we should retry creating the file.
                if (error.Error == Interop.Error.ENOENT)
                {
                    continue;
                }
            }
        }

        throw Interop.GetExceptionForIoErrno(error, sharedMemoryFilePath);
    }
    else
    {
        createdFile = true;
    }
}

if (fd.IsInvalid)
{
    Debug.Assert(!createIfNotExist);
    fd = Interop.Sys.Open(sharedMemoryFilePath, mandatoryFlags, 0);

    if (fd.IsInvalid)
    {
        Interop.ErrorInfo error = Interop.Sys.GetLastErrorInfo();
        fd.Dispose();
        throw Interop.GetExceptionForIoErrno(error, sharedMemoryFilePath);
    }
}

// If we got here, fd is a valid file handle.

I think this approach would work.

The main difference would be that we're optimizing (in terms of number of syscalls) for the create case here instead of the open existing case. Not that that really matters given all the other infrastructure here IMO.

I think some minor modifications to the current one is easier to grep than this one. @adamsitnik what prompted your suggestions is there some optimization inherent in this alternative approach?

I think that Adam's implementation, while a bigger diff from the existing code, is much easier to read and understand when looking at at a glance.

Adam's implementation also better follows Jared's advice (ie don't check for existence as existence may change. Just do the action and handle the errors when the file does not exist.
) https://blog.paranoidcoding.org/2009/12/10/the-file-system-is-unpredictable.html

I think some minor modifications to the current one is easier to grep than this one. @adamsitnik what prompted your suggestions is there some optimization inherent in this alternative approach?

The problem we are trying to solve is very similar to some File.OpenHandle issues (and this is an API I own) and I personally prefer to use O_EXCL when dealing with TOCTOU because it's atomic.

But again, as I wrote I don't have the context (is the most common case to create the file? or open an existing one?)

and this is an API I own

Then we can absolutely go in that direction.

But again, as I wrote I don't have the context (is the most common case to create the file? or open an existing one?)

I don't have enough information to answer that either. I agree it does look like a pessimization with respect to existing paths, but as @jkoritzinsky points out there is a ton of machinery here so I'm not sure how much that matters.

Do we have any of these APIs in the perf lab? Is a there a microbenchmark we could run locally that would help see the cost?

Do we have any of these APIs in the perf lab? Is a there a microbenchmark we could run locally that would help see the cost?

If we don't, we should add it.

Do we have any of these APIs in the perf lab? Is a there a microbenchmark we could run locally that would help see the cost?

@caaavik-msft @DrewScoggins

We do not ☹️

Maybe. If we can benchmark, perhaps that would tell us if this is better.

Maybe. If we can benchmark, perhaps that would tell us if this is better.

You can use the @EgorBot to run the benchmarks via GH comment. An example: #125452 (comment)

It will run provided microbenchmark against a local build of dotnet/runtime and report the results as a comment.