Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix WriteUTF8FileAtomic to preserve symlinks #10908

Merged
6 commits merged into from
Aug 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/spelling/expect/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2253,6 +2253,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)
Don-Vito marked this conversation as resolved.
Show resolved Hide resolved
{
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);
}
}