Fix WindowsFormsApplicationBase.IsSingleInstance

[elachlan]

Add currentUserSID to GetApplicationInstanceID for use in TryCreatePipeServer to avoid system wide blocking. Used in conjunction with PipeOptions.CurrentUserOnly.

Fixes #3715
Fixes #11232
Fixes #11365

Microsoft Reviewers: Open in CodeFlow

[elachlan]

@weltkante had this suggestion and I ran with it. I think the concerns raised by @zanchey are technically valid, but block a pragmatic fix for most use cases. It might be mitigated by us using PipeOptions.CurrentUserOnly as well.

@Olina-Zhang could your team test this? I don't have a multi-user environment I can easily utilize for testing.

[codecov]

Codecov Report

❌ Patch coverage is 95.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.27371%. Comparing base (48a3d7c) to head (03230e4).
⚠️ Report is 1857 commits behind head on main.

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main      #11258         +/-   ##
===================================================
- Coverage   74.34305%   74.27371%   -0.06935%     
===================================================
  Files           3012        3025         +13     
  Lines         625885      626888       +1003     
  Branches       46553       46743        +190     
===================================================
+ Hits          465302      465613        +311     
- Misses        157190      157923        +733     
+ Partials        3393        3352         -41     

Flag	Coverage Δ
Debug	74.27371% <95.00000%> (-0.06935%)	⬇️
integration	17.99401% <0.00000%> (-0.34132%)	⬇️
production	47.00072% <87.50000%> (+0.01172%)	⬆️
test	96.99432% <100.00000%> (-0.04596%)	⬇️
unit	43.97653% <87.50000%> (+0.07251%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[paul1956]

Can you remove ByVal and make private function parameters start with lower case character. If touching code, it should conform to current VB Style otherwise it will conflict with other PR's trying to clean up VB Code Style.

[Olina-Zhang]

@elachlan tested this PR using following 2 apps(one is Winforms VB app, another is Winforms C#) in .Net 9.0 SDK build on a server with multi-users. Every logged in user can launch the app, issue is fixed. Thanks!
VB_SingleInstance_true.zip
SingleInstanceTrue_CSharp.zip

[elachlan]

Is MD5 okay here? really we are just taking a path and making a guid. I don't expect it to be an issue. Because its in the startup path I believe its quite important to have a fast hashing algorithm here as well.

[weltkante]

that depends on whether you are treating the input (the file path) as a secret and/or want to prevent colissions with forged inputs - md5 is fine against accidental colissions - personally I don't think there's a security issue here, and if it were, then it needs to be adressed at the permission level since the named objects are visible and accessible to non-dotnet code as well, malicious code could interact with the named objects directly instead of going through the file path hashing

[A9G-Data-Droid]

MD5 is not okay, in general, because it is blocked by setting FIPS mode at the OS level. FIPS mode can't know that you are using MD5 in a non-security context, it just blocks any application using that algorithm. This is a pain for everyone in the government space.

[elachlan]

What would be appropriate then?

[A9G-Data-Droid]

'SHA1()' is going to be the least expensive option. Looks like they will allow it until 2030, at which point SHA2 will be required.

[elachlan]

I suggest using one of these:
https://learn.microsoft.com/en-us/dotnet/api/system.io.hashing

[elachlan]

I have some concerns about using the filepath of the assembly because we are calling it using Assembly.GetCallingAssembly().

This needs to be tested with self-contained single file and AOT scenarios. As far as I know for self contained single file applications I've had to use Assembly.GetEntryAssembly() to get the correct location of where the exe is located. If Assembly.GetCallingAssembly() is used it might get the location of the assembly that is extracted to the users appdata.

I am unsure if this is still the case.

@Olina-Zhang maybe your team could test this scenario?

[Olina-Zhang]

@Olina-Zhang maybe your team could test this scenario?

Use your new updated code to test these issues again, right? Or publish app with self-contained to test?

[elachlan]

The main issue to test is #11365 using the updated code.

Then additionally try #11365 but on a single file published app.

[Olina-Zhang]

Hi @elachlan, tested the updated code, here is the result:

1. GH issue WindowsFormsApplicationBase.IsSingleInstance applications are global on a machine, not per user #3715 and GH issue Winforms VB application with Single Instance setting crashes for multiple users on terminal server #11232 - Fixed
2. For GH issue: Single instance app doesn't work when same app is in different folders #11365, the scenarios customer reported about launch exe from 2 different folders with one user - Fixed, we can see 2 app instances
3. For GH issue: Single instance app doesn't work when same app is in different folders #11365, copy it on a server machine in 2 different folders, then launch exe from 2 different folders with different users - Launch exe successfully with different users
4. For GH issue: Single instance app doesn't work when same app is in different folders #11365, published app with self-contained, put it in 2 different folders, then launch exe from different folders with one user - app can launch from one folder, and app cannot launch from another folder, get a message that "it is already installed from a different location" --- it is expected, same as the .Net framework's behavior
5. For GH issue: Single instance app doesn't work when same app is in different folders #11365, published app with self-contained, copy it on a server machine in one folder, then launch exe with different users --- first user can install and launch, second user can install but cannot launch, get following exception in EventViewer:

Application: WinFormsApp1.exe
CoreCLR Version: 9.0.24.25601
.NET Version: 9.0.0-preview.5.24256.1
Description: The process was terminated due to an unhandled exception.
Exception Info: Microsoft.VisualBasic.ApplicationServices.CantStartSingleInstanceException: This single-instance application could not connect to the original instance.
   at Microsoft.VisualBasic.ApplicationServices.WindowsFormsApplicationBase.Run(String[] commandLine)
   at WinFormsApp1.Program.Main()

[elachlan]

That last scenario with published self-contained app and two users is interesting and unexpected.

We will need to investigate further.

[elachlan]

@Olina-Zhang can you retest the published self-contained failure scenario with the latest? The event viewer will have a bit more detail hopefully.

[Olina-Zhang]

@Olina-Zhang can you retest the published self-contained failure scenario with the latest? The event viewer will have a bit more detail hopefully.

I didn't see any difference with updated code, same as Yesterday's:

[elachlan]

My change was to pass in the GetApplicationInstanceID value to the exception so we could see what it was. But it seems not to have worked.

[elachlan]

@lonitra I cannot proceed with this PR as I don't have the resources for testing. I suggest we have @LeafShi1 and team take over this work as they are better resourced. I'd also advocate for a backport to 9.0 as its blocking migrations.

[elachlan]

@Tanya-Solyanik can someone please revisit this? I think most of the leg work is done.

[KlausLoeffelmann]

I read through this - what's the current status here.
@elachlan, could you rebase this so it's current?