From 05bbdb27c944f6669fdd973a3b75afdb21550298 Mon Sep 17 00:00:00 2001 From: Chris Webb Date: Sun, 26 Nov 2023 17:50:32 +0000 Subject: [PATCH 1/3] Fix crash when ':write -method replace' fails to create tempfile If a user attempts to save a file without write permission for the containing directory, with writemethod set as 'replace' or an explicit ':write -method replace' command, kak crashes with "terminating due to uncaught exception of type Kakoune:runtime_error". (Note this doesn't happen with a forced write, which fails earlier when it tries to enable u+w permission.) Don't raise another exception when already bailing out with a runtime error for failing to create a temporary file or open the existing file. Instead, make a best-efforts attempt to restore the file permissions before raising the first exception, and only report the runtime chmod exception if that step fails on the non-error path. --- src/file.cc | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/file.cc b/src/file.cc index 58eb9284..8fdb4831 100644 --- a/src/file.cc +++ b/src/file.cc @@ -351,16 +351,16 @@ void write_buffer_to_file(Buffer& buffer, StringView filename, if (force and ::chmod(zfilename, st.st_mode | S_IWUSR) < 0) throw runtime_error(format("unable to change file permissions: {}", strerror(errno))); - auto restore_mode = on_scope_end([&]{ - if ((force or replace) and ::chmod(zfilename, st.st_mode) < 0) - throw runtime_error(format("unable to restore file permissions: {}", strerror(errno))); - }); - char temp_filename[PATH_MAX]; const int fd = replace ? open_temp_file(filename, temp_filename) : open(zfilename, O_CREAT | O_WRONLY | O_TRUNC, 0644); if (fd == -1) - throw file_access_error(filename, strerror(errno)); + { + auto saved_errno = errno; + if (force) + ::chmod(zfilename, st.st_mode); + throw file_access_error(filename, strerror(saved_errno)); + } { auto close_fd = on_scope_end([fd]{ close(fd); }); @@ -370,7 +370,14 @@ void write_buffer_to_file(Buffer& buffer, StringView filename, } if (replace and rename(temp_filename, zfilename) != 0) + { + if (force) + ::chmod(zfilename, st.st_mode); throw runtime_error("replacing file failed"); + } + + if ((force or replace) and ::chmod(zfilename, st.st_mode) < 0) + throw runtime_error(format("unable to restore file permissions: {}", strerror(errno))); if ((buffer.flags() & Buffer::Flags::File) and real_path(filename) == real_path(buffer.name())) From d3af9b57d46cd5a7b0a4688c161b18b4b8d40a28 Mon Sep 17 00:00:00 2001 From: Chris Webb Date: Sun, 26 Nov 2023 18:12:52 +0000 Subject: [PATCH 2/3] Restore file ownership when editing with root privilege When a privileged :write is used with -method replace, it silently resets the ownership of files to root:root. Restore the original owner and group in the same way we restore the original permissions. Ownership needs to be restored before permissions to avoid setuid and setgid bits being set while the file is still owned by root, and to avoid them being subsequently lost again on chmod(2). --- src/file.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/file.cc b/src/file.cc index 8fdb4831..a13e5c47 100644 --- a/src/file.cc +++ b/src/file.cc @@ -376,6 +376,8 @@ void write_buffer_to_file(Buffer& buffer, StringView filename, throw runtime_error("replacing file failed"); } + if (replace and geteuid() == 0 and ::chown(zfilename, st.st_uid, st.st_gid) < 0) + throw runtime_error(format("unable to restore file ownership: {}", strerror(errno))); if ((force or replace) and ::chmod(zfilename, st.st_mode) < 0) throw runtime_error(format("unable to restore file permissions: {}", strerror(errno))); From 3ba3399f943dff5d5fbe0c4b89752b0ec549ff74 Mon Sep 17 00:00:00 2001 From: Chris Webb Date: Tue, 28 Nov 2023 08:51:32 +0000 Subject: [PATCH 3/3] Set replacement file permissions before moving into place When doing :write -method replace, make sure we've set the correct mode, uid and gid on the replacement file before attempting to rename it on top of the original. This means that the original file is left in place with correct permissions if anything fails, rather than ending up with 0700 permissions from mkstemp(). --- src/file.cc | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/file.cc b/src/file.cc index a13e5c47..87112e24 100644 --- a/src/file.cc +++ b/src/file.cc @@ -369,6 +369,13 @@ void write_buffer_to_file(Buffer& buffer, StringView filename, ::fsync(fd); } + if (replace and geteuid() == 0 and ::chown(temp_filename, st.st_uid, st.st_gid) < 0) + throw runtime_error(format("unable to set replacement file ownership: {}", strerror(errno))); + if (replace and ::chmod(temp_filename, st.st_mode) < 0) + throw runtime_error(format("unable to set replacement file permissions: {}", strerror(errno))); + if (force and not replace and ::chmod(zfilename, st.st_mode) < 0) + throw runtime_error(format("unable to restore file permissions: {}", strerror(errno))); + if (replace and rename(temp_filename, zfilename) != 0) { if (force) @@ -376,11 +383,6 @@ void write_buffer_to_file(Buffer& buffer, StringView filename, throw runtime_error("replacing file failed"); } - if (replace and geteuid() == 0 and ::chown(zfilename, st.st_uid, st.st_gid) < 0) - throw runtime_error(format("unable to restore file ownership: {}", strerror(errno))); - if ((force or replace) and ::chmod(zfilename, st.st_mode) < 0) - throw runtime_error(format("unable to restore file permissions: {}", strerror(errno))); - if ((buffer.flags() & Buffer::Flags::File) and real_path(filename) == real_path(buffer.name())) buffer.notify_saved(get_fs_status(real_path(filename)));