Fix WindowsFormsApplicationBase.IsSingleInstance #11258

elachlan · 2024-04-23T22:42:48Z

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

…r use in TryCreatePipeServer to avoid system wide blocking. Used in conjunction with PipeOptions.CurrentUserOnly

elachlan · 2024-04-23T22:47:01Z

@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 · 2024-04-24T01:22:30Z

Codecov Report

Attention: Patch coverage is 95.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 74.27371%. Comparing base (48a3d7c) to head (03230e4).
Report is 60 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.

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 · 2024-04-24T03:05:37Z

@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 · 2024-05-14T00:27:21Z

...sualBasic.Forms/src/Microsoft/VisualBasic/ApplicationServices/WindowsFormsApplicationBase.vb

+
+#Disable Warning CA5351 ' We want speed over collision resistance.
+        Private Shared Function GetFilePathAsGuid(filePath As String) As Guid
+            Using md5 As MD5 = MD5.Create()


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.

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

elachlan · 2024-05-14T01:38:23Z

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 · 2024-05-14T08:06:41Z

@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 · 2024-05-14T08:25:32Z

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

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

Olina-Zhang · 2024-05-14T09:37:36Z

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

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
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
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
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
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 · 2024-05-14T10:47:47Z

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

We will need to investigate further.

elachlan · 2024-05-15T03:02:51Z

@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 · 2024-05-15T03:52:44Z

@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 · 2024-05-16T01:08:46Z

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.

Fixes dotnet#3715 - Add currentUserSID to GetApplicationInstanceID fo…

22668d1

…r use in TryCreatePipeServer to avoid system wide blocking. Used in conjunction with PipeOptions.CurrentUserOnly

elachlan requested a review from a team as a code owner April 23, 2024 22:42

dotnet-policy-service bot assigned elachlan Apr 23, 2024

elachlan requested a review from KlausLoeffelmann April 23, 2024 22:42

elachlan mentioned this pull request Apr 23, 2024

Winforms VB application with Single Instance setting crashes for multiple users on terminal server #11232

Open

Fix tests

1edb42e

paul1956 suggested changes Apr 24, 2024

View reviewed changes

dotnet-policy-service bot added the 📭 waiting-author-feedback The team requires more information from the author label Apr 24, 2024

Fix Code style

bb2322e

dotnet-policy-service bot removed the 📭 waiting-author-feedback The team requires more information from the author label Apr 24, 2024

elachlan added the waiting-review This item is waiting on review by one or more members of team label Apr 25, 2024

elachlan requested review from GrabYourPitchforks and Shyam-Gupta April 25, 2024 02:17

Add fix for dotnet#11365

083c991

elachlan commented May 14, 2024

View reviewed changes

Small refactor and fix tests

c91531d

small change for debug

03230e4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix WindowsFormsApplicationBase.IsSingleInstance #11258

Fix WindowsFormsApplicationBase.IsSingleInstance #11258

elachlan commented Apr 23, 2024 •

edited

elachlan commented Apr 23, 2024

codecov bot commented Apr 24, 2024 •

edited

paul1956 left a comment •

edited

Olina-Zhang commented Apr 24, 2024

elachlan May 14, 2024

weltkante May 14, 2024 •

edited

elachlan commented May 14, 2024 •

edited

Olina-Zhang commented May 14, 2024 •

edited

elachlan commented May 14, 2024

Olina-Zhang commented May 14, 2024 •

edited

elachlan commented May 14, 2024

elachlan commented May 15, 2024

Olina-Zhang commented May 15, 2024

elachlan commented May 16, 2024

Fix WindowsFormsApplicationBase.IsSingleInstance #11258

Are you sure you want to change the base?

Fix WindowsFormsApplicationBase.IsSingleInstance #11258

Conversation

elachlan commented Apr 23, 2024 • edited

Microsoft Reviewers: Open in CodeFlow

elachlan commented Apr 23, 2024

codecov bot commented Apr 24, 2024 • edited

Codecov Report

paul1956 left a comment • edited

Choose a reason for hiding this comment

Olina-Zhang commented Apr 24, 2024

elachlan May 14, 2024

Choose a reason for hiding this comment

weltkante May 14, 2024 • edited

Choose a reason for hiding this comment

elachlan commented May 14, 2024 • edited

Olina-Zhang commented May 14, 2024 • edited

elachlan commented May 14, 2024

Olina-Zhang commented May 14, 2024 • edited

elachlan commented May 14, 2024

elachlan commented May 15, 2024

Olina-Zhang commented May 15, 2024

elachlan commented May 16, 2024

elachlan commented Apr 23, 2024 •

edited

codecov bot commented Apr 24, 2024 •

edited

paul1956 left a comment •

edited

weltkante May 14, 2024 •

edited

elachlan commented May 14, 2024 •

edited

Olina-Zhang commented May 14, 2024 •

edited

Olina-Zhang commented May 14, 2024 •

edited