Enforce single install across winget processes #3118
Conversation
| else | ||
| { | ||
| // Best effort when no VT (arbitrary number of spaces that seems to work) | ||
| m_out << "\r \r"; |
There was a problem hiding this comment.
Nit: How about something like m_out << '\r' << std::string(N, ' ') << '\r'; so that we know how many spaces there are and can extend it if needed?
There was a problem hiding this comment.
Changed it to get the current console width.
|
|
||
| CrossProcessInstallLock::CrossProcessInstallLock() | ||
| { | ||
| m_mutex.create(L"WinGetCrossProcessInstallLock", 0, SYNCHRONIZE); |
There was a problem hiding this comment.
Should we be worried about some third party acquiring this named mutex and just making winget unusable?
There was a problem hiding this comment.
We could add a random suffix to the mutex (and any other named mutexes), but we would have to write that information into a location that the unelevated user token can access. While it would make it slightly harder to execute this attack, it wouldn't be any harder from a security perspective.
I suppose that we could put a conditional ACE on it to require our package identity (when packaged) to even read from it. Even that is just another minor bump in the road that can be overcome with more code (but not any more security access).
| @@ -1821,4 +1821,7 @@ Please specify one of them using the --source option to proceed.</value> | |||
| <value>Reboot required to fully enable the Windows Feature(s); proceeding due to --force</value> | |||
| <comment>"Windows Feature(s)" is the name of the options Windows features setting.</comment> | |||
| </data> | |||
| <data name="InstallWaitingOnAnother" xml:space="preserve"> | |||
| <value>Waiting on another install or uninstall to complete...</value> | |||
There was a problem hiding this comment.
Nit: Shouldn't it be "Waiting for"?
There was a problem hiding this comment.
Turns out that my southern US origins may be the source of this based on a brief look. They both are "correct" but various sources give differing opinions, but that one vs the other is colloquial. I have no problem changing it though.
| @@ -24,7 +24,7 @@ TEST_CASE("CPRWL_MultipleReaders", "[CrossProcessReaderWriteLock]") | |||
| otherThread.detach(); | |||
|
|
|||
| // Wait up to a second for the other thread to do one thing... | |||
| REQUIRE(signal.wait(500)); | ||
| } | ||
|
|
||
| TEST_CASE("CPIL_BlocksOthers", "[CrossProcessInstallLock]") |
There was a problem hiding this comment.
Nit: Do we really need to abbreviate CPIL?
|
|
||
| std::thread otherThread([&signal, &progress]() { | ||
| CrossProcessInstallLock otherThreadLock; | ||
| otherThreadLock.Acquire(progress); |
There was a problem hiding this comment.
Should we REQUIRE(!otherThreadLock.Acquire(progres);?
Change
This change uses a named mutex to enforce a single install/uninstall operation is occurring for the entire session. The current behavior is that every process was able to install a single thing at a time, but as we have more usage this could lead to a potential issue.
The mutex is acquired and held for the workflow task that routes out to the installer type specific tasks. Some installer types are explicitly excluded, as they are known to contain their own synchronization internally.
In addition to the mutex, the ability to set a message to show with the indefinite progress (spinner) was added. This allows for the message to be removed when the wait is over. The message was not implemented into the progress bar, nor is it being sent to COM progress.
The COM scheduling was not made aware of the mutex, and so it will execute as it has in the past, blocking on the mutex when appropriate. This could be improved in the future to enable the parallel capable installer types to operate. One potential option would be a second installer processing thread that only operated on the excluded types, and thus would never block.
Validation
Manually validated the user experience with an interactive install.
Automated tests are added to ensure that the lock is held during the installation workflow.
Microsoft Reviewers: Open in CodeFlow