microsoft / terminal

The new Windows Terminal and the original Windows console host, all in the same place!
MIT License
95.26k stars 8.27k forks source link

Terminal overwriting hard link to settings.json #14730

Open akbyrd opened 1 year ago

akbyrd commented 1 year ago

Windows Terminal version

1.15.3465.0

Windows build number

10.0.19045.2546

Other Software

No response

Steps to reproduce

Replacing settings.json with a hard link to an existing settings file while the terminal is running causes weird behavior. The hard link is broken shortly after it is created. Then it looks like the previous settings file and newly linked file are merged and written out as a new, unlinked, combined settings file.

I ran into this accidentally because the quake window was still open and I didn't realize it.

Repro:

But if you do it a second time it works

Obviously I should be killing terminal processes before doing this, so maybe it's a bit moot. The terminal seems to respect a hard link just fine if it's already there when it starts up. Still the nuke-and-merge behavior is certainly surprising.

Expected Behavior

If a new settings.json is copied over the existing one the terminal should not merge with or overwrite it.

Actual Behavior

If a new settings.json is copied over the existing one the terminal overwrites it with a merged copy

zadjii-msft commented 1 year ago

Obviously I should be killing terminal processes before doing this, so maybe it's a bit moot. The terminal seems to respect a hard link just fine if it's already there when it starts up.

Yea, that's really the crux of the issue here. I'm not sure how possible it is for us to determine that a file was replaced by a hard link.

This is where we set up the hot-reload handler https://github.com/microsoft/terminal/blob/7d0baa79460046b64a8814b5706e92fecfdf5496/src/cascadia/TerminalApp/AppLogic.cpp#L902-L910

_reader is a wil::unique_folder_change_reader_nothrow, from https://github.com/microsoft/wil. I don't think I see anything about hard links anywhere on that repo. Hmm.

lhecker commented 1 year ago

Changing the settings file in any way causes it to be reloaded and potentially rewritten to disk. Every time the settings are written to disk the hardlink will be replaced by us. This is the code responsible for it: https://github.com/microsoft/terminal/blob/7d0baa79460046b64a8814b5706e92fecfdf5496/src/cascadia/TerminalSettingsModel/FileUtils.cpp#L290-L322

There's no way to fix this, because when we write our settings we don't want to mutate the original unless we're 100% certain that the write operation was a success. As such we can't write to the original (hardlinked) file. We also can't figure out where the hardlink "points to", because hardlinks have no "target" - they're just alternative file names pointing to the same data on disk.

But as you can see in the code above, we do support an alternative already: symlinks. Those do point to a target, allowing us to replace the targeted file atomically.

akbyrd commented 1 year ago

Oh, I see, even if I kill the process it's going to blow up the hard link file anyway because it rotates a new file into place when saving.

Any chance it's possible to save new settings the other way around:

This avoids recreating the file. I suppose the backup can succeed and the write can fail, but that seems like an edge case of an edge case and there's always a valid backup to restore from if it occurs.

Symlinks require admin rights on Windows but hard links don't, which is a bummer. Symlinks don't seem to work, unless I'm doing something wrong? I get the same result with cmd and mklink.

$terminalSettings = $env:LOCALAPPDATA + "\Packages\Microsoft.WindowsTerminal_8wekyb3d8bbwe\LocalState\settings.json"
New-Item -ItemType SymbolicLink -Force -Path $terminalSettings -Target res/windows-terminal-settings.json

image

Also, I think there's a reasonable case to be made that if you copy a new settings file over the existing one the terminal should just load that file and apply the settings without the merge-and-resave behavior. That's independent of the symlink/hard linking concern. I'm guessing it's doing something like loading the new settings, applying them to the current state, then writing out what's in memory, which is effectively the old and new settings merged together. As a user it's not particularly clear to me why that's desirable.

DHowett commented 1 year ago

Any chance it's possible to save new settings the other way around:

It's not a bad idea, but part of the reason we save settings the way we do is that it is atomic. There's no middle state where there is an empty file (after opening it with truncation) or one that is partially written during which Terminal--or the OS--could be interrupted.

237dmitry commented 1 year ago

What's wrong with this file? What settings do you want to keep intact? The only things that come to my mind are formatting, alphabetical order, and comments. Or perhaps it has to do with the long path to the settings file?

malxau commented 1 year ago

One way to fix this would be to enumerate all of the links to the file before starting the update, and after the rename for the settings file, repeat this for all other links (create a new link in a different directory, rename over the previous link.) Once the final link is replaced, the original file is gone.

Obviously this assumes all of the other directories can be modified, which is also true for the symbolic link handling code. And currently, the hardlink enumerate API is not publicly documented, although it really should be available in a supported form. The internal form has existed since Vista (see fsutil hardlink list.)

I'm not meaning to say this should be addressed, but it could be addressed. I doubt many tools can save to one hardlink without disrupting the set, for the same reason as the code here.

akbyrd commented 1 year ago

Should I open a separate issue for symlinks not working? It sounds like those are intended to be supported.

KalleOlaviNiemitalo commented 1 year ago

the hardlink enumerate API is not publicly documented

FindFirstFileNameW is documented. "Creates an enumeration of all the hard links to the specified file."

lhecker commented 1 year ago

Migrating a hardlink however is a bit more annoying. It requires a call to NtSetInformationFile of type FileLinkInformationEx. It also makes me wonder whether changing hardlinks automatically without user intervention isn't kind of a poor, unexpected behavior as well... Is there any particular reason not to use symlinks for this?

KalleOlaviNiemitalo commented 1 year ago

@lhecker is that because CreateHardLinkW does not support FILE_LINK_REPLACE_IF_EXISTS?

There is ReplaceFileW, but it is documented as not preserving the file ID of the replaced file, so I assume it doesn't preserve hard links either.

Transactional NTFS would have been a way to do the replacement atomically but it has been deprecated.