Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can we lift the name length limitation on semaphores? #15399

Closed
poizan42 opened this issue Oct 9, 2015 · 9 comments
Closed

Can we lift the name length limitation on semaphores? #15399

poizan42 opened this issue Oct 9, 2015 · 9 comments
Labels
area-System.Threading enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@poizan42
Copy link
Contributor

poizan42 commented Oct 9, 2015

Now that we have more-or-less gotten rid of the path length limitation (#645), could we also get rid of the semaphore name length restriction? The documentation for CreateSemaphore claims that the name is limited to MAX_PATH characters, however testing this shows that the documentation is wrong (at least on Windows 10):

using Microsoft.Win32.SafeHandles;
using System;
using System.ComponentModel;
using System.Runtime.InteropServices;

namespace SemaphoreTest
{
    class Program
    {
        [DllImport("kernel32.dll", BestFitMapping = false, CharSet = CharSet.Unicode, SetLastError = true)]
        private static extern SafeWaitHandle CreateSemaphore(IntPtr lpSecurityAttributes, int initialCount, int maximumCount, string name);
        [DllImport("kernel32.dll", SetLastError = true)]
        [return: MarshalAs(UnmanagedType.Bool)]
        private static extern bool CloseHandle(SafeHandle hObject);
        private static void TestCreateSemaphore(string longName)
        {
            var sem = CreateSemaphore(IntPtr.Zero, 0, 1, longName);
            if (sem.IsInvalid)
            {
                Console.WriteLine("Failed: " + new Win32Exception(Marshal.GetLastWin32Error()));
            }
            else
            {
                Console.WriteLine("OK: " + sem.DangerousGetHandle());
                CloseHandle(sem);
            }
        }
        static void Main(string[] args)
        {
            var longName =
                "!!!!!!!!!!012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789" + // 100
                "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789" + // 200
                "012345678901234567890123456789012345678901234567890123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMN"; // 260,300
            TestCreateSemaphore(longName);
        }
    }
}

Running this prints OK followed by the handle. Setting a breakpoint before CloseHandle and inspecting the handles shows that the semaphore is indeed created with the full name:
Process Hacker showing that the path isn't truncated

Edit: It would be a good idea if I posted the same code that I ran when I took the screenshot :)

@poizan42 poizan42 changed the title Can we lift the name length limitation in semaphores? Can we lift the name length limitation on semaphores? Oct 9, 2015
@joshfree
Copy link
Member

joshfree commented Oct 9, 2015

/cc @JeremyKuhne @ericeil

@ericeil
Copy link
Contributor

ericeil commented Oct 9, 2015

I think this would be nice, but am concerned about relying on undocumented OS behavior. Also, this doesn't seem as compelling to me as the long file paths work; file paths are user-visible, so there are good reasons for them to be long. Semaphore names are not visible, and so can be generated any number of ways, including the hashing scheme @poizan42 has already suggested.

@poizan42
Copy link
Contributor Author

I understand the concern about undocumented behaviour. On the other hand this is a compatibility constraint. There is already going to be applications out there that will stop working if the length limit is enforced, and I don't really see a reason why it should be in the future.

Maybe one of you Microsoft insiders could ask the relevant team whether it's ok to rely on (and in that case the documentation could be updated)?.

@aevitas
Copy link

aevitas commented Oct 14, 2015

As an aside, while a direct call to CreateSemaphore does work, calling the .NET equivalent: new Semaphore(1,1, "Very long name here"); will indeed throw an ArgumentException if the name is more than 260 characters. Isn't this limitation imposed here by ValidateNewName simply arbitrary at this point if the platform invoked call does work for names longer than MAX_PATH (at least on Windows)?

@poizan42
Copy link
Contributor Author

Yes that is my point. However the documentation for CreateSemaphore claims that is should have a limit of 260 characters even though that isn't enforced. The real limit is probably 2^15 - 30 or a bit less depending on whether it's a global or local semaphore and what the session id is.

@aevitas
Copy link

aevitas commented Oct 14, 2015

@poizan42 Agree, it's even more conspicuous that enforcing this limitation or not differs depending on how you create the Semaphore. It's a huge inconsitency at the very least.

@karelz
Copy link
Member

karelz commented Oct 13, 2016

We are fine with removing the 'artificial' MAX_PATH enforcement in .NET Core code and leave the limit check on underlying OS.
Just make sure (incl. tests) that the exception is same as it used to be with the MAX_PATH check.

@JeremyKuhne
Copy link
Member

I'm fine with removing this as well. Just make sure there is a test that creates a semaphore with a length greater than MAX_PATH (1K would be a good choice) and add a comment that we're letting the API kick back names that are too long rather than pre-validating them.

If, for some reason, we get back an error that says the name is too long (say, with a 10K name or on Windows 7) we should make sure it maps to ArgumentException.

@kouvel kouvel self-assigned this Feb 10, 2017
@kouvel kouvel removed their assignment Apr 11, 2017
@stephentoub
Copy link
Member

stephentoub commented Jun 11, 2018

Fixed by dotnet/coreclr#18402
cc: @jkotas

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jan 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Threading enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

9 participants