From: Mark Wielaard Date: Thu, 11 Jun 2026 16:45:20 +0000 (+0200) Subject: elfcompress: Handle existing output files (symlinks) X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=0e8b57aa36d80fd8e366c6e22f5bc96fdfe7c062;p=thirdparty%2Felfutils.git elfcompress: Handle existing output files (symlinks) With -o elfcompress opens the output file with O_WRONLY and O_CREAT. If the output file already existed then without O_TRUNC the file is written from the start, but keeps all existing data. That means the file might contain extra data if the (compressed) ELF file is shorter than the existing file. Without -o the input file is the output file and we create a temp file to create the output which should then be atomically replaced using rename. This might fail when the input file was a symlink (to a file in a different directory.) An additional complication is that we were using full path based calls for open, rename and unlink (in case we had to remove the file again). This can go wrong if between the open and the unlink a path element is changed by a different process. To prevent renaming or unlinking the wrong file we have to first open the target directory and only then create a file inside it with openat. And use the directory fd with renameat or unlinkat when we have to rename or delete the file. So to solve all these issues we first try to pin the directory of the output file. Then try to open the output file with O_EXCL. Which makes sure we created the file so we can write directly to it. If that fails then the output file already exists. Then we first resolve the full file path (in case the output file is a symlink) and then open the (resolved) directory where the output should go. We then make sure to create a temporary file inside this directory using a new system.h helper function xmkstempat. Finally, after the output file is fully created (and flushed out to disk) we use renameat to atomically replace the output file with our temp file. Since we still have the directory open this works even if the file path is changed while creating the temp file data. On failure we use unlinkat with the directory fd to remove the output file. Add a new run-compress-test-out.sh testcase for input and/or output file being symlinks and/or pointing to the same file. * configure.ac: Check for getentropy, sys/random.h and getrandom. * lib/system.h (read_retry): New static inline function. (xrandom64): Likewise. (xmkstempat): Likewise. * src/elfcompress.c (process_file): Declare new variables to keep copies of the (ouput) directory, dirfd, base file and temporary file name. Track whether we need to unlink_fnew. Fix trailing \n in error fmt. Call open with O_EXCL for output file. Use realpath and xmkstempat if output file exists. fsync temp file before caling renameat. Use unlinkat in cleanup. close and free new temporaries. * tests/run-compress-test-out.sh: New test file. * tests/Makefile.am (TESTS): Add new test file. (EXTRA_DIST): Likewise. Signed-off-by: Mark Wielaard --- diff --git a/configure.ac b/configure.ac index 9bf50d87..0266ffd8 100644 --- a/configure.ac +++ b/configure.ac @@ -523,6 +523,13 @@ AC_CHECK_DECLS([reallocarray],[],[], [#define _GNU_SOURCE #include ]) +dnl for xmkstemp we need a random source +AC_CHECK_DECLS([getentropy],[],[], + [#include ]) +AC_CHECK_HEADERS([sys/random.h]) +AC_CHECK_DECLS([getrandom],[],[], + [#include ]) + AC_CHECK_FUNCS([process_vm_readv mremap]) AS_IF([test "x$ac_cv_func_mremap" = "xno"], diff --git a/lib/system.h b/lib/system.h index c17e2aa0..17901a3b 100644 --- a/lib/system.h +++ b/lib/system.h @@ -1,6 +1,6 @@ /* Declarations for common convenience functions. Copyright (C) 2006-2011 Red Hat, Inc. - Copyright (C) 2022 Mark J. Wielaard + Copyright (C) 2022, 2026 Mark J. Wielaard Copyright (C) 2023 Khem Raj. This file is part of elfutils. @@ -39,6 +39,7 @@ #endif #include +#include #include #include #include @@ -53,6 +54,10 @@ #include #include +#if defined(HAVE_SYS_RANDOM_H) +#include +#endif + #if defined(HAVE_ERROR_H) #include #elif defined(HAVE_ERR_H) @@ -233,6 +238,25 @@ pread_retry (int fd, void *buf, size_t len, off_t off) return recvd; } +static inline ssize_t __attribute__ ((unused)) +read_retry (int fd, void *buf, size_t len) +{ + ssize_t recvd = 0; + + do + { + ssize_t ret = TEMP_FAILURE_RETRY (read (fd, ((char *)buf) + recvd, + len - recvd)); + if (ret <= 0) + return ret < 0 ? ret : recvd; + + recvd += ret; + } + while ((size_t) recvd < len); + + return recvd; +} + /* The demangler from libstdc++. */ extern char *__cxa_demangle (const char *mangled_name, char *output_buffer, size_t *length, int *status); @@ -257,4 +281,84 @@ xbasename(const char *s) } #pragma GCC poison basename +/* Get a random uint64_t. Returns zero on success, minus one on failure. */ +static inline int +xrandom64 (uint64_t *r) +{ + /* Prefer getentropy if it is available, fallback to getrandom, if + both are missing, or if they fail try reading from /dev/urandom. */ +#if HAVE_DECL_GETENTROPY + if (getentropy (r, sizeof (*r)) == 0) + return 0; +#elif HAVE_DECL_GETRANDOM + if (TEMP_FAILURE_RETRY (getrandom (r, sizeof (*r), 0)) == sizeof (*r)) + return 0; +#endif + int fd = open("/dev/urandom", O_RDONLY | O_CLOEXEC); + if (fd < 0) + return -1; + if (read_retry (fd, r, sizeof (uint64_t)) == sizeof (uint64_t)) + { + close (fd); + return 0; + } + int save_errno = errno; + close (fd); + errno = save_errno; + /* We could try some pseudo-random thing with getpid and + clock_gettime. But if even getting something from /dev/urandom + fails it seems we tried hard enough already. */ + return -1; +} + +/* There is no mkstempat needed for creating a temp file in a specific + directory. Needed e.g. in combination with renameat to atomicly + replace a file. So define one ourselves. Like mkstemp the template + must end in "XXXXXX", which are replaced by an unique filename + suffix. The file is created with user read/write permissions only + in the given dirfd using openat. + https://sourceware.org/bugzilla/show_bug.cgi?id=19866 */ +static inline int +xmkstempat (int dirfd, char *templ) +{ + /* Only use these 64 chars. */ + const char chars[] = + "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ-_"; + + /* Must end in 6X. */ + size_t l = strlen (templ); + if (l < 6 || memcmp (templ + l - 6, "XXXXXX", 6) != 0) + { + errno = EINVAL; + return -1; + } + + int tries = 128; /* Just fail with EEXIST if 128 tries wasn't enough. */ + do + { + uint64_t r; /* We need at least 64^6 == 2^36 */ + if (xrandom64 (&r) != 0) + return -1; + + /* Random chars for the template. */ + for (int i = 0; i < 6; i++) + { + templ[l - 6 + i] = chars[r % 64]; + r /= 64; + } + + /* Must be able to open exclusively. */ + int fd = openat (dirfd, templ, + O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, + S_IRUSR | S_IWUSR); + if (fd >= 0) + return fd; + + tries--; + } + while (tries > 0 && errno == EEXIST); + + return -1; +} + #endif /* system.h */ diff --git a/src/elfcompress.c b/src/elfcompress.c index fdcff328..7818cce3 100644 --- a/src/elfcompress.c +++ b/src/elfcompress.c @@ -1,5 +1,6 @@ /* Compress or decompress an ELF file. Copyright (C) 2015, 2016, 2018 Red Hat, Inc. + Copyright (C) 2026 Mark J. Wielaard This file is part of elfutils. This file is free software; you can redistribute it and/or modify @@ -37,6 +38,9 @@ #include "libeu.h" #include "printversion.h" +/* Really should come from libgen.h, but we poisoned basename in system.h. */ +extern char *dirname(char *path); + /* Name and version of program. */ ARGP_PROGRAM_VERSION_HOOK_DEF = print_version; @@ -338,9 +342,18 @@ process_file (const char *fname) Elf *elf = NULL; /* The output ELF. */ - char *fnew = NULL; + char *fnew = NULL; /* Name used if we don't need the split up tempname. */ int fdnew = -1; Elf *elfnew = NULL; + bool unlink_fnew = false; /* Call unlinkat on failure. */ + + /* Split up dir/base names if necessary. */ + char *dirc = NULL; + char *basec = NULL; + const char *bname = NULL; + const char *dname = NULL; + int dirfd = -1; + char *tempname = NULL; /* Name used instead of fnew for output. */ /* Buffer for (one) new section name if necessary. */ char *snamebuf = NULL; @@ -370,7 +383,7 @@ process_file (const char *fname) fd = open (fname, O_RDONLY); if (fd < 0) { - error (0, errno, "Couldn't open %s\n", fname); + error (0, errno, "Couldn't open %s", fname); goto cleanup; } @@ -611,29 +624,94 @@ process_file (const char *fname) scnnames = xcalloc (shnum, sizeof (char *)); } - /* Create a new (temporary) ELF file for the result. */ - if (foutput == NULL) + /* Now deal with the output. If we can (exclusively) open the + output file directly, we can just use that. But we still need to + make sure that if there is a failure we unlink the correct file + (in case the path is manipulated between creation and + deletion). */ + fnew = xstrdup (foutput == NULL ? fname : foutput); + /* Split up the path into the dir and base parts. */ + dirc = xstrdup (fnew); + dname = dirname (dirc); + basec = xstrdup (fnew); + bname = xbasename (basec); + + /* Pin the directory. */ + dirfd = open (dname, O_RDONLY | O_DIRECTORY); + if (dirfd < 0) { - size_t fname_len = strlen (fname); - fnew = xmalloc (fname_len + sizeof (".XXXXXX")); - strcpy (mempcpy (fnew, fname, fname_len), ".XXXXXX"); - fdnew = mkstemp (fnew); + error (0, errno, "Couldn't open output dir %s", dname); + goto cleanup; } - else + fdnew = openat (dirfd, bname, O_WRONLY | O_CREAT | O_EXCL, + st.st_mode & ALLPERMS); + + /* If we cannot open the output exclusively for writing directly + (because it already exists), e.g. it might be the current input + file, then we want to write to a temporary file first and then + (atomically) replace it. This is slightly tricky. To make sure + the replacement (rename) is atomic the temp file and final file + need to be in the same directory. We use realpath to make sure + we end up in the actual directory that the output is in if it was + a symlink. To make sure the directory path doesn't change + between temp file creation and rename we need to keep a dirfd + open. */ + if (fdnew < 0 && errno == EEXIST) { - fnew = xstrdup (foutput); - fdnew = open (fnew, O_WRONLY | O_CREAT, st.st_mode & ALLPERMS); + /* OK, it already existed (or was a symlink). Try again, but now + with the resolved path. */ + free (fnew); fnew = NULL; + free (dirc); dirc = NULL; + free (basec); basec = NULL; + close (dirfd); + fnew = realpath (foutput == NULL ? fname : foutput, NULL); + if (fnew == NULL) + { + error (0, errno, "Couldn't get realpath for %s", + foutput == NULL ? fname : foutput); + goto cleanup; + } + + /* Split up the path into the dir and base parts. */ + dirc = xstrdup (fnew); + dname = dirname (dirc); + basec = xstrdup (fnew); + bname = xbasename (basec); + + /* Pin the directory. */ + dirfd = open (dname, O_RDONLY | O_DIRECTORY | O_NOFOLLOW); + if (dirfd < 0) + { + error (0, errno, "Couldn't open output dir %s", dname); + goto cleanup; + } + + /* Create a temp file inside the output dir. This could + possibly done with O_TMPFILE and then using /proc/self/fd to + rename. But it is not clear how portable that is. */ + size_t bname_len = strlen (bname); + tempname = xmalloc (bname_len + sizeof (".XXXXXX")); + sprintf (tempname, "%s.XXXXXX", bname); + fdnew = xmkstempat (dirfd, tempname); } if (fdnew < 0) { - error (0, errno, "Couldn't create output file %s", fnew); - /* Since we didn't create it we don't want to try to unlink it. */ - free (fnew); - fnew = NULL; + error (0, errno, "Couldn't create output file %s", + tempname == NULL ? fnew : tempname); + /* Since we couldn't create it we don't want to try to unlink it. */ + if (tempname != NULL) + { + free (tempname); + tempname = NULL; + } goto cleanup; } + /* Did we directly opened fnew then need to unlink on failure. */ + if (tempname == NULL) + unlink_fnew = true; + elfnew = elf_begin (fdnew, ELF_C_WRITE, NULL); if (elfnew == NULL) { @@ -1352,22 +1430,30 @@ process_file (const char *fname) or fchown may clear them. */ if (fchown (fdnew, st.st_uid, st.st_gid) != 0) if (verbose >= 0) - error (0, errno, "Couldn't fchown %s", fnew); + error (0, errno, "Couldn't fchown %s", + tempname == NULL ? fnew : tempname); if (fchmod (fdnew, st.st_mode & ALLPERMS) != 0) if (verbose >= 0) - error (0, errno, "Couldn't fchmod %s", fnew); + error (0, errno, "Couldn't fchmod %s", + tempname == NULL ? fnew : tempname); /* Finally replace the old file with the new file. */ - if (foutput == NULL) - if (rename (fnew, fname) != 0) - { - error (0, errno, "Couldn't rename %s to %s", fnew, fname); - goto cleanup; - } - - /* We are finally done with the new file, don't unlink it now. */ - free (fnew); - fnew = NULL; + if (tempname != NULL) + { + fsync (fdnew); /* Flush all data and metadata before replacing file. */ + if (renameat (dirfd, tempname, dirfd, bname) != 0) + { + error (0, errno, "Couldn't rename %s to %s", tempname, bname); + goto cleanup; + } + /* We are finally done with the temp file, don't unlink it now. */ + free (tempname); + tempname = NULL; + } + else + unlink_fnew = false; /* We created the output, it is complete now. */ + + /* Success! Now just cleanup. */ res = 0; cleanup: @@ -1377,13 +1463,23 @@ cleanup: elf_end (elfnew); close (fdnew); - if (fnew != NULL) + if (tempname != NULL) { - unlink (fnew); - free (fnew); - fnew = NULL; + unlinkat (dirfd, tempname, 0); + free (tempname); } + if (unlink_fnew) + unlinkat (dirfd, bname, 0); + + free (fnew); + + if (dirfd >= 0) + close (dirfd); + + free (dirc); + free (basec); + free (snamebuf); if (names != NULL) { diff --git a/tests/Makefile.am b/tests/Makefile.am index 8ae7d126..fb0690ed 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -206,7 +206,7 @@ TESTS = run-arextract.sh run-arsymtest.sh run-ar.sh newfile test-nlist \ elfshphehdr run-lfs-symbols.sh run-dwelfgnucompressed.sh \ run-elfgetchdr.sh \ run-elfgetzdata.sh run-elfputzdata.sh run-zstrptr.sh \ - run-compress-test.sh \ + run-compress-test.sh run-compress-test-out.sh \ run-readelf-zdebug.sh run-readelf-zdebug-rel.sh \ emptyfile vendorelf fillfile dwarf_default_lower_bound \ dwarf_language_lower_bound \ @@ -575,7 +575,7 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \ testfile-zgabi32.bz2 testfile-zgabi64.bz2 \ testfile-zgabi32be.bz2 testfile-zgabi64be.bz2 \ run-elfgetchdr.sh run-elfgetzdata.sh run-elfputzdata.sh \ - run-zstrptr.sh run-compress-test.sh \ + run-zstrptr.sh run-compress-test.sh run-compress-test-out.sh \ run-disasm-bpf.sh \ testfile-bpf-dis1.expect.bz2 testfile-bpf-dis1.o.bz2 \ run-reloc-bpf.sh \ diff --git a/tests/run-compress-test-out.sh b/tests/run-compress-test-out.sh new file mode 100755 index 00000000..ad479ba6 --- /dev/null +++ b/tests/run-compress-test-out.sh @@ -0,0 +1,74 @@ +#! /bin/sh +# Copyright (C) 2026 Mark J. Wielaard +# This file is part of elfutils. +# +# This file is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# elfutils is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +. $srcdir/test-subr.sh + +testrun_elfcompress_out() +{ + testfile="$1" + testfiles ${testfile} + + # Direct compress + testrun ${abs_top_builddir}/src/elfcompress -v -t zlib-gnu ${testfile} + testrun ${abs_top_builddir}/src/elflint --gnu-ld ${testfile} + + # Decompress with -o being the input file + testrun ${abs_top_builddir}/src/elfcompress -v -t none -o ${testfile} \ + ${testfile} + testrun ${abs_top_builddir}/src/elflint --gnu-ld ${testfile} + + # Compress with -o being an existing file + tempfiles ${testfile}.tmp + touch ${testfile}.tmp + testrun ${abs_top_builddir}/src/elfcompress -v -t zlib -o ${testfile}.tmp \ + ${testfile} + testrun ${abs_top_builddir}/src/elflint --gnu-ld ${testfile}.tmp + + # Decompress with -o being a symlink to the input + tempfiles ${testfile}.link + ln -s ${testfile}.tmp ${testfile}.link + testrun ${abs_top_builddir}/src/elfcompress -v -t none -o ${testfile}.link \ + ${testfile}.tmp + testrun ${abs_top_builddir}/src/elflint --gnu-ld ${testfile}.link + + # Compress with input being a symlink to a file in a nested directory + tempfiles ${testfile}.deep + mkdir deep + cp ${testfile} deep/ + ln -s deep/${testfile} ${testfile}.deep + testrun ${abs_top_builddir}/src/elfcompress -v -t zlib ${testfile}.deep + testrun ${abs_top_builddir}/src/elflint --gnu-ld deep/${testfile} + rm deep/${testfile} + rmdir deep +} + +# The actual test file shouldn't matter, but just use a couple of +# different ones. + +# Random ELF32 testfile +testrun_elfcompress_out testfile4 + +# Random ELF64 testfile +testrun_elfcompress_out testfile12 + +# Random ELF64BE testfile +testrun_elfcompress_out testfileppc64 + +# Random ELF32BE testfile +testrun_elfcompress_out testfileppc32 + +exit 0