Hi Mark, On Thu, Jun 11, 2026 at 12:52 PM Mark Wielaard <[email protected]> wrote: > > 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 <[email protected]> > --- > configure.ac | 7 ++ > lib/system.h | 106 +++++++++++++++++++++- > src/elfcompress.c | 158 ++++++++++++++++++++++++++------- > tests/Makefile.am | 4 +- > tests/run-compress-test-out.sh | 74 +++++++++++++++ > 5 files changed, 315 insertions(+), 34 deletions(-) > create mode 100755 tests/run-compress-test-out.sh > > v3 > - Add configure checks for getentropy, sys/random.h and getrandom. > - Add read_retry helper for reading /dev/urandom > - Add xrandom64 helper that tries getentropy if it is available, > fallback to getrandom, and if both are missing, or if they fail try > reading from /dev/urandom. > - Use it in xmkstempat, also use O_CLOEXEC. > - Add bool unlink_fnew to track if we have to delete fnew on failure. > - Remove O_TRUNC on O_CREAT | O_EXCL open call. > - Also split up the path into the dir and base parts for O_EXCL openat > call. So we have a pinned directory dirfd to use with unlinkat. > - Use O_DIRECTORY | O_NOFOLLOW after realpath call. > > > diff --git a/configure.ac b/configure.ac > index fbe039d5b43a..5779050b944b 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -518,6 +518,13 @@ AC_CHECK_DECLS([reallocarray],[],[], > [#define _GNU_SOURCE > #include <stdlib.h>]) > > +dnl for xmkstemp we need a random source > +AC_CHECK_DECLS([getentropy],[],[]
Missing comma after the 2nd `[]`. I believe this still works as intended as-is since getentropy is declared in unistd.h which is included by default via AC_INCLUDES_DEFAULT when AC_CHECK_DECLS 4th arg is missing. > + [#include <unistd.h>]) > +AC_CHECK_HEADERS([sys/random.h]) > +AC_CHECK_DECLS([getrandom],[],[], > + [#include <sys/random.h>]) > + > 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 c17e2aa0fea1..965d994065eb 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 <[email protected]> > + Copyright (C) 2022, 2026 Mark J. Wielaard <[email protected]> > Copyright (C) 2023 Khem Raj. > This file is part of elfutils. > > @@ -39,6 +39,7 @@ > #endif > > #include <errno.h> > +#include <fcntl.h> > #include <stdbool.h> > #include <stddef.h> > #include <stdint.h> > @@ -53,6 +54,10 @@ > #include <sys/param.h> > #include <unistd.h> > > +#if defined(HAVE_SYS_RANDOM_H) > +#include <sys/random.h> > +#endif > + > #if defined(HAVE_ERROR_H) > #include <error.h> > #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 defined(HAVE_DECL_GETENTROPY) This will evaluate to true unconditionally since AC_CHECK_DECLS always sets this macro to either 1 or 0. > + if (getentropy (r, sizeof (*r)) == 0) > + return 0; > +#elif defined(HAVE_DECL_GETRANDOM) Currently this is dead code, but defined(HAVE_DECL_GETRANDOM) will similarly always evaluate to true if it were reached. > + 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 fdcff32847ce..680bd4a4977a 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 <[email protected]> > 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); > + free (dirc); > + free (basec); > + 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; This goto will cause double free/close of dirc, basec and dirfd. They are freed/closed right before the goto but cleanup assumes they're set. Apart from these small fixes the patch LGTM. Aaron > + } > + > + /* 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 8ae7d126636e..fb0690edaf57 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 000000000000..ad479ba65c1a > --- /dev/null > +++ b/tests/run-compress-test-out.sh > @@ -0,0 +1,74 @@ > +#! /bin/sh > +# Copyright (C) 2026 Mark J. Wielaard <[email protected]> > +# 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 <http://www.gnu.org/licenses/>. > + > +. $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 > -- > 2.54.0 >
