refactor(windows/uninstall-gc): refactor Windows uninstall GC APIs

[Cloud0310]

part of #5056, first of Windows GC code fixes.
Including:

1. refactor for unsafe code
2. inline a helper function and GCErr that only used once

[rami3l]

Question: Would you mind explaining to me why this refactoring action produces equivalent code? It's not immediately clear to me at least when looking at this...

View changes since the review

[Cloud0310]

The key is preserving the delete-on-close handle through this chain:

  File
  -> Command::stdin(file)
  -> DuplicateHandle(..., inherit=true, DUPLICATE_SAME_ACCESS)
  -> GC’s stdin
  -> inherited by net

Rust’s Windows implementation explicitly uses:
Stdio::Handle(ref handle) => handle.duplicate(0, true, c::DUPLICATE_SAME_ACCESS),
ref

Then, internally it calls:

  sys::c::DuplicateHandle(
  cur_proc,
  handle,
  cur_proc,
  &mut ret,
  access,
  inherit as sys::c::BOOL,
  options,)

Keeping the handle and options. ref

The duplicate references the same file object, and FILE_FLAG_DELETE_ON_CLOSE explicitly includes duplicated handles in its cleanup lifetime.

FILE_FLAG_DELETE_ON_CLOSE: The file is to be deleted immediately after all of its handles are closed, which includes the specified handle and any other open or duplicated handles.
If there are existing open handles to a file, the call fails unless they were all opened with the FILE_SHARE_DELETE share mode. Windows documentation

The named Command retains the parent’s handle through the existing sleep, replacing the scope guard. Removing net’s Stdio::null() lets it inherit the handle from GC.

So this preserves the intended cleanup-handle lifetime, while changing stdin into the handle-passing mechanism; GC does not read it.

[Cloud0310]

Also added comment for this in code.

[ChrisDenton]

To be honest this does feel like a surprising hack. Would it not be clearer to call DuplicateHandle ourselves? It's a bit of unsafe but not too much.

[Cloud0310]

To be honest this does feel like a surprising hack. Would it not be clearer to call DuplicateHandle ourselves? It's a bit of unsafe but not too much.

Maybe I was trying too hard for explain this by telling the whole underlaying implementation details here, and this might be the reason why you think using DuplicateHandle with unsafe is more intuitive here.

Basically,we're doing two things with old code:

1. Creating a handle with FILE_FLAG_DELETE_ON_CLOSE.
2. Letting the sub command inherit this handle.

In the new code, the first thing is done with:

    let gc_handle = OpenOptions::new()
        .read(true)
        .share_mode(FILE_SHARE_READ | FILE_SHARE_DELETE)
        .custom_flags(FILE_FLAG_DELETE_ON_CLOSE)
        .open(&gc_exe)
        .context(CliError::WindowsUninstallMadness)?;

The second thing (THE ODD "HACK") is done with:

    let mut command = Command::new(gc_exe);
    command
        .stdin(gc_handle)

By passing the stdin a handle is just what Command::stdin docs does:

Configuration for the child process’s standard input (stdin) handle.
Defaults to inherit when used with spawn or status, and defaults to piped when used with output.

Then it's completely ok for passing it a handle with configured options, then it inherits.

So, naturally, we do the same as old code.
However, I do think I should refine the comments more on this for justifying why passing Stdio a file handle is the doing the inherition here. Thanks for letting me knowing this :) @ChrisDenton

[Cloud0310]

BTW, when thinking of this old code piece, I nocited there might be a further improvement: replace old scopeguard::guard with std::os::windows::io::OwnedHandle.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[rustbot]

☔ The latest upstream changes (possibly #5091) made this pull request unmergeable. Please resolve the merge conflicts.

[Cloud0310]

New findings on windows uninstallation:

1. Since rust 1.86.0, std::fs::remove_file would call DeleteFileW first, then upon ERROR_ACCESS_DENIED, it would use new posix_delete API.
   BTW, it's @ChrisDenton for implementing this in std API, many thanks. ref

2. posix_delete (which uses FILE_DISPOSITION_POSIX_SEMANTICS flag) is supported since Windows 10 1607 released on July 29, 2015, ref

Note

FILE_DISPOSITION_POSIX_SEMANTICS is not supported on FAT32 filesystem and older versions of Windows
However, system installation on FAT32 is not typical and not officially supported by MSFT, and the oldest version supporting of Windows this flag at least 11 years old.

So maybe we can safely remove all the madness about Windows unintallation and self replacement.
I will test locally then PR, hopefully, we can end this nightmare forever.
BTW, this new way of removing running binary is told by @baka-gourd , many thanks for insights on this.

[ChrisDenton]

Rustup already does not support FAT32 so I'm not worried on that front.

However, I don't think posix semantics completely removes the need for some degree of extra ceremony. It does not bypass locks and anti-malware software can still cause havoc. Though I think most of this can be mitigate using hard links to rustup's tmp directory so at least one link remains (and cleaning up tmp can be postponed if necessary).

[Cloud0310]

However, I don't think posix semantics completely removes the need for some degree of extra ceremony. It does not bypass locks and anti-malware software can still cause havoc.
Yes, testing gives out the same result.

Though I think most of this can be mitigate using hard links to rustup's tmp directory so at least one link remains (and cleaning up tmp can be postponed if necessary).

On the cleanup side, I don't think moving this to tmp is a better option, this would require further time order and further code bloat.

However, there's a mechanism of Windows NTFS we can make use of: ADS (Alternate Data Streams), which is used for creating alternative backups of a data, and it's removed once the main file is gone. This also allows for self delete, and seld replacement. Still, the idea is shared by @baka-gourd and it has a C demo here: https://gist.github.com/codehz/fa3340806d67eca90aec89a33852d707

[ChrisDenton]

I wouldn't call something "bloat" if it's a necessary part of supporting users, however unfortunate it may be.

However, there's a mechanism of Windows NTFS we can make use of: ADS (Alternate Data Streams), which is used for creating alternative backups of a data, and it's removed once the main file is gone. This also allows for self delete, and seld replacement. Still, the idea is shared by @baka-gourd and it has a C demo here: https://gist.github.com/codehz/fa3340806d67eca90aec89a33852d707

I'm not sure that solves a problem we have. Currently the hack we use is to ultimately piggyback on a system executable. Not great but it seems to work out ok. We could replace that with the directory trick but, aside from being more complex, it has most of the same issues. We still need to try to delete many files that may be in use or have locks or have A/V inspecting them. Plus hiding executables is a very malware thing to do so I worry that could trigger heuristics.

If we wanted to simplify things a bit we could make use of the MoveFileEx ability to delay deletion until next boot. So we write a gc executable to the user's temporary directory and have the OS clean it up.

[Cloud0310]

I guess by just copying rustup.exe into it's ADS here would make us no longer need to consider where to place the binary also we don't need further modification on the removing order of directories.

Another point is that, we don't need to open another program net.exe for inheriting and eliminate the handles, then wait for gc to self delete.

Basically the old way is:

1. Create handle with given options
2. Copy gc program to somewhere else, exclude this location
3. Inherits the handle, remove main program
4. Open net.exe for final handle cleanup

The new way in this would be:

1. Create handle with options
2. Copy gc program into ADS
3. Clean up rustup.exe which also removed gc

But I do agree with your opinion upon:

Plus hiding executables is a very malware thing to do so I worry that could trigger heuristics.
I would try searching for how common Windows anti-virus software's compatibility with this.

And I have also investigated the MoveFileEx it seems still not as good as using ADS here. By using ADS, we don't need wait for a reboot. As long as current gc mechanism is working properly, the file is gone with filesystem mechanism.

I've already tested on Windows Server 2025 for POC of ADS problem, and it's working just right.

[Cloud0310]

As I discussed with @rami3l, he thought this ADS trick also helps with his goal of concurrent safe rustup.

[rami3l]

Looking at all but the first commit, I think the changes here are quite reasonable.

As for the ADS experiment, I think it is more or less independent from the changes proposed here and there is no blocking relationship from one to another, so maybe it can be in a separate draft, given that it doesn't influence our prioritization on #5056.

@ChrisDenton as for the first commit I think I think I can entrust you with your judgement on whether "a certain amount of unsafe" is better here in terms of readability? Does that sound okay to you?

View changes since this review

[djc]

Looks like an improvement to me.

Suggest hoisting all the local imports out to the module level (likely in a separate commit).

View changes since this review