[Bug]: Crash using app.requestSingleInstanceLock in different Windows user sessions

[jeremyspiegel]

Preflight Checklist

• I have read the Contributing Guidelines for this project.
• I agree to follow the Code of Conduct that this project adheres to.
• I have searched the issue tracker for a bug report that matches the one I want to file, without success.

Electron Version

18 (any version, through at least 18.2.1)

What operating system are you using?

Windows

Operating System Version

Windows 10 21H1

What arch are you using?

x64

Last Known Working Electron version

17.4.1

Expected Behavior

App shouldn't crash when run in multiple Windows user sessions (running the app as two separate Windows users) with app.requestSingleInstanceLock. Each separate Windows user session should support its own separate "single" app instance.

Actual Behavior

The second instance in a separate user session calls CreateNamedPipe at https://github.com/electron/electron/pull/31460/files#diff-aeb3ae283fcbd712d19bf743d031b51f2b2fcd9e6122e5d5303a12acfdeeb447R506 and sees INVALID_HANDLE_VALUE with ERROR_ACCESS_DENIED, and crashes on the subsequent CHECK.

Testcase Gist URL

No response

Additional Information

Here is a simple app that enforces a single instance per Windows user session and demonstrates the issue:

const {app} = require('electron');
if (!app.requestSingleInstanceLock()) {
  console.log('second instance exiting');
  app.quit();
} else {
  console.log('first instance running');
}

In Electron 17.4.1, you can run it in separate Windows user sessions and they'll both print "first instance running". In Electron 18 (any version), the second one will crash.

The problem is that named pipes are not session-specific like the window and mutex objects created in process_singleton_win.cc that are used to synchronize/communicate between processes.

Even if the named pipe were session-specific, there is still a race condition where if multiple "second instances" of different Electron apps (or of the same app) run at the same time, they could get acks meant for other ones. Rather than have a single named pipe name that's shared across all apps and instances, would it make sense to have the second instance pass the first instance a duplicated handle to either 1) an anonymous pipe or 2) shared memory + a mutex or event? I think this would be safer than the suggestion in #33777 (comment) of having the second instance have its own message window (presumably that would have to be found using the same user data directory string window lookup mechanism, and that would still have problems with multiple second instances running simultaneously that share the same user data directory).

cc @rzhao271 @deepak1556 @nornagon

[deepak1556]

Thanks for creating the issue!

(presumably that would have to be found using the same user data directory string window lookup mechanism, and that would still have problems with multiple second instances running simultaneously that share the same user data directory).

Yes searching by the user data directory would result in race conditions like you have mentioned, what I suggested is for the second instance to pass handle to its own message window when attempting to send a message to the first instance. I prefer to use the message window style of communication since it is already tested in the singleton codepath and reduces the patch complexity.

[jeremyspiegel]

what I suggested is for the second instance to pass handle to its own message window when attempting to send a message to the first instance

Ah okay, I didn't see that explicitly stated in #33777 (comment) but that solution sounds good to me.

I think there's a security concern here with the current code as well. I believe an unprivileged user could intercept an ack meant for a second instance run by a more privileged user, and similarly, an unprivileged user could craft their own ack that is delivered to a second instance of a more privileged user. Is this worth creating a separate issue?

[rzhao271]

I plan on switching over the implementation to use the message window instead of named pipes next month. I feel that'll also resolve the security concern above, since we'd be dealing with specific handles rather than a named pipe.

I'm not sure whether there's a similar way of intercepting SendMessageTimeout calls, though, which is what Chromium currently uses along with FindWindowEx when sending data from the second instance to the first instance: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/win/chrome_process_finder.cc

[jeremyspiegel]

I'm not sure whether there's a similar way of intercepting SendMessageTimeout calls

I don't think there is, since FindWindowEx would only search windows in the current desktop. The interception I brought up is only for the other direction (communicating from the first instance back to the second), and is possible because of relying on the first instance to communicate with the second instance through a well-known name, rather than having the second instance pass a handle to use to communicate back with.

I'm seeing another crash, a backward compatibility issue that doesn't require multiple Windows sessions to reproduce. Run the simple app.requestSingleInstanceLock code above using Electron 17, then run a second instance using Electron 18, and there's a crash in ReadAck at https://github.com/electron/electron/pull/31460/files#diff-aeb3ae283fcbd712d19bf743d031b51f2b2fcd9e6122e5d5303a12acfdeeb447R468.

[VerteDinde]

@jeremyspiegel I just want to confirm, because it was released on the same day this issue was filed - does Electron v18.2.0 also present this issue for you?

Even if it does, I can leave this issue open to address the comments you and Deepak are discussing, just wanted to clarify for triage purposes.

[jeremyspiegel]

@VerteDinde there aren't any changes in 18.2.0 or main that would have fixed this so I would expect the issue to be present there as well (this file needs to change: https://github.com/electron/electron/blob/main/patches/chromium/feat_add_data_transfer_to_requestsingleinstancelock.patch)

[jeremyspiegel]

I just verified with 18.2.1 to be sure and the issue is still present.

[sofianguy]

Fixed #34139

[VerteDinde]

@jeremyspiegel Thanks again for reporting this and including such a detailed breakdown of the issue, it was a huge help! Let me know if this issue is still present for you in either 18.2.2 or later - if it is, we can re-open this issue.

[jeremyspiegel]

Happy to help! I see the issue is fixed in 18.2.3, thanks!