Skip to content

Commit

Permalink
Fix WriteUTF8FileAtomic to preserve symlinks (#10908)
Browse files Browse the repository at this point in the history
WriteUTF8FileAtomic  overrides the content of the file "atomically"
by creating a temp file and then renaming it to the original path.
The problem arises when the original path is symbolic link,
as the link itself gets overridden by a file (rather than the link target).
This PR introduces a special handling of the symlinks:
if the path as a symlink we resolve the path and use:
1. target's directory to create a temp-file in
2. target itself to be replaced with the tempfile.

Symlink resolution is problematic when the target path does not exist,
as there is no good utility that resolves such link (canonical() fails).
In this corner case we skip the "atomic" approach of renaming the file
and write the link target directly.

Closes #10787
  • Loading branch information
Don-Vito authored and DHowett committed Aug 25, 2021
1 parent 68c54bf commit 5cc663c
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 2 deletions.
1 change: 1 addition & 0 deletions .github/actions/spelling/expect/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2241,6 +2241,7 @@ SWMR
SWP
swprintf
SYMED
symlink
SYNCPAINT
sys
syscalls
Expand Down
24 changes: 22 additions & 2 deletions src/cascadia/TerminalSettingsModel/FileUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,27 @@ namespace Microsoft::Terminal::Settings::Model

void WriteUTF8FileAtomic(const std::filesystem::path& path, const std::string_view content)
{
auto tmpPath = path;
// GH#10787: rename() will replace symbolic links themselves and not the path they point at.
// It's thus important that we first resolve them before generating temporary path.
std::error_code ec;
const auto resolvedPath = std::filesystem::is_symlink(path) ? std::filesystem::canonical(path, ec) : path;
if (ec)
{
if (ec.value() != ERROR_FILE_NOT_FOUND)
{
THROW_WIN32_MSG(ec.value(), "failed to compute canonical path");
}

// The original file is a symbolic link, but the target doesn't exist.
// Consider two fall-backs:
// * resolve the link manually, which might be less accurate and more prone to race conditions
// * write to the file directly, which lets the system resolve the symbolic link but leaves the write non-atomic
// The latter is chosen, as this is an edge case and our 'atomic' writes are only best-effort.
WriteUTF8File(path, content);
return;
}

auto tmpPath = resolvedPath;
tmpPath += L".tmp";

// Writing to a file isn't atomic, but...
Expand All @@ -132,6 +152,6 @@ namespace Microsoft::Terminal::Settings::Model
// renaming one is (supposed to be) atomic.
// Wait... "supposed to be"!? Well it's technically not always atomic,
// but it's pretty darn close to it, so... better than nothing.
std::filesystem::rename(tmpPath, path);
std::filesystem::rename(tmpPath, resolvedPath);
}
}

0 comments on commit 5cc663c

Please sign in to comment.