Mutex name null causes FailFast in chk on Unix

[danmoseley]

This caused a test failure in CoreCLR CI:

  FailFast: 
  
     at System.Diagnostics.Debug.Assert(Boolean condition, String message, String detailMessage) in /mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/src/mscorlib/shared/System/Diagnostics/Debug.cs:line 97
     at System.Threading.Mutex.CreateMutexCore(Boolean initiallyOwned, String name, Boolean& createdNew) in /mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/src/mscorlib/src/System/Threading/Mutex.cs:line 108
     at System.IO.IsolatedStorage.Helper.CreateMutexNotOwned(String pathName) in /mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/_/fx/src/System.IO.IsolatedStorage/src/System/IO/IsolatedStorage/Helper.Win32Unix.cs:line 146
     at System.IO.IsolatedStorage.Helper.GetRandomDirectory(String rootDirectory, IsolatedStorageScope scope) in /mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/_/fx/src/System.IO.IsolatedStorage/src/System/IO/IsolatedStorage/Helper.Win32Unix.cs:line 86

dotnet/coreclr#14181 left a Debug.Assert upstream of the ArgumentException:

        private void CreateMutexCore(bool initiallyOwned, string name, out bool createdNew)
        {
            Debug.Assert(name == null || name.Length <= Path.MaxPath);

            uint mutexFlags = initiallyOwned ? Win32Native.CREATE_MUTEX_INITIAL_OWNER : 0;

            SafeWaitHandle mutexHandle = Win32Native.CreateMutexEx(null, name, mutexFlags, AccessRights);
            int errorCode = Marshal.GetLastWin32Error();

            if (mutexHandle.IsInvalid)
            {
                mutexHandle.SetHandleAsInvalid();
#if PLATFORM_UNIX
                if (errorCode == Interop.Errors.ERROR_FILENAME_EXCED_RANGE)
                    // On Unix, length validation is done by CoreCLR's PAL after converting to utf-8
                    throw new ArgumentException(SR.Format(SR.Argument_WaitHandleNameTooLong, Interop.Sys.MaxName), nameof(name));
#endif

The fix may be to remove the assert (if the resulting error for null is also acceptable). Plus make sure we have for long and null mutex names in both Unix and Windows.

[danmoseley]

@ViktorHofer

[danmoseley]

Also please change the exceptions to include the actual name if it's invalid, especially this one

                throw new ArgumentException(SR.Format(SR.Argument_WaitHandleNameTooLong, Interop.Sys.MaxName), nameof(name));

[danmoseley]

@wtgodbe could you please take care of this one?

[danmoseley]

cc @karelz

[wtgodbe]

@danmosemsft I'm confused as to why we're asserting that name==null at https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Threading/Mutex.cs#L105

[danmoseley]

@wtgodbe Mutexes can be anonymous, you indicate that by passing null. https://msdn.microsoft.com/en-us/library/f55ddskf(v=vs.110).aspx

It looks like this assert was to verify that VerifyNameForCreate was called already. That has #if !PLATFORM_UNIX around it. It looks like the assert should have that too.

Then you can also upgrade the "invalid name" exception messages as I suggested.

[ViktorHofer]

@danmosemsft I guess you wanted me to look at it but changed your mind and assigned @wtgodbe later?

[danmoseley]

@ViktorHofer please disregard, I just put you down as you made the original change so you might have context

[wtgodbe]

I'm confused how we could trigger that assert, then - The check for name.Length <= Path.MaxPath is already surrounded by #if !PLATFORM_UNIX - https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Threading/Mutex.cs#L102

[karelz]

I think the else-branch assert is triggered on UNIX. It says that on UNIX, you can have only null name for mutex. That is wrong.
I think it was a mistake introduced here: https://github.com/dotnet/coreclr/pull/15229/files#diff-eccefb2b4e8c0df9169261509103c566R105

[danmoseley]

My bad, this repo was not synced, hence my misstatements above.. What @karelz says sounds right.

[wtgodbe]

Fixed by dotnet/coreclr#15734