avoid dropping installer file handle to prevent race#10768
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR changes Windows autoupdate relaunch to clone the installer path without taking ownership of the TempPath, preventing RAII cleanup from deleting the installer before the spawned Inno Setup process has opened it.
Concerns
- No blocking correctness or security concerns found in the changed hunk.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
lucieleblanc
left a comment
lucieleblanc
left a comment
There was a problem hiding this comment.
This fix makes sense to me!
Left a question about another race condition cited in the code.
There was a problem hiding this comment.
Is this code at all related to the issue? Should we clean this up?
There was a problem hiding this comment.
oh yes! ready to be deleted
2 participants
Description
This PR avoids deletion of the inno setup installer during auto-update.
The linked issue shows that in some cases, auto-update fails with a "file not found" error dialog. This is because of a race condition between dropping the
TempPath, an RAII wrapper around the installer exe, and the installer itself actually starting up. It may have seemed like the installer should have started up sequentially first by calling a blocking call tospawn. This is not so b/c the exe actually spawns another process and exits immediately. To resolve this race, we simply do not delete the installer. There is no need, as the installer lives in%TEMP%anyways.Linked Issue
This PR addresses #10767. Master issue is #10044.
Testing
Untested due to the lack of a robust reproduction case.