Hi Mark,
On Tue, Jun 2, 2026 at 6:47 AM 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.)
>
> Solve both issues by first trying to open the output file with
> O_EXCL. Which makes sure we created the file so we can write directly
> to it. If it 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 directory where the output should go. We then make sure
> the 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.
>
> Add a new run-compress-test-out.sh testcase for input and/or output
> file being symlinks and/or pointing to the same file.
>
> * lib/system.h (xmkstempat): New helper function.
> * src/elfcompress.c (process_file): Declare new variables to
> keep copies of the (ouput) directory, dirfd, base file and
> temporary file name. 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]>
> ---
> lib/system.h | 54 ++++++++++++-
> src/elfcompress.c | 133 +++++++++++++++++++++++++--------
> tests/Makefile.am | 4 +-
> tests/run-compress-test-out.sh | 74 ++++++++++++++++++
> 4 files changed, 229 insertions(+), 36 deletions(-)
> create mode 100755 tests/run-compress-test-out.sh
>
> v2:
> - Complete comment for xbasename
> Add comments to fnew and tempname explaining what is which
> Make tempname based on bname, not fnew.
> Consistently use tempname when set instead of fnew.
> Only free tempname and set to NULL on error.
> Keep fnew around till cleanup, and always free it.
>
> diff --git a/lib/system.h b/lib/system.h
> index c17e2aa0fea1..0f97f4346558 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>
> @@ -52,6 +53,7 @@
> #include <sys/mman.h>
> #include <sys/param.h>
> #include <unistd.h>
> +#include <sys/random.h>
>
> #if defined(HAVE_ERROR_H)
> #include <error.h>
> @@ -257,4 +259,54 @@ xbasename(const char *s)
> }
> #pragma GCC poison basename
>
> +/* 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 (TEMP_FAILURE_RETRY (getrandom (&r, sizeof (r), 0)) != sizeof (r))
getrandom was introduced in glibc 2.25 (Feb 2017) so it's probably safer if
we add a configure check for getrandom or <sys/random.h>. There's one already
for reallocarray (glibc 2.26) plus fallbacks if it's missing.
> + 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,
> + 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..272d2c431281 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,10 +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;
>
> + /* 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 +382,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,26 +623,68 @@ process_file (const char *fname)
> scnnames = xcalloc (shnum, sizeof (char *));
> }
>
> - /* Create a new (temporary) ELF file for the result. */
> - if (foutput == NULL)
> - {
> - size_t fname_len = strlen (fname);
> - fnew = xmalloc (fname_len + sizeof (".XXXXXX"));
> - strcpy (mempcpy (fnew, fname, fname_len), ".XXXXXX");
> - fdnew = mkstemp (fnew);
> - }
> - else
> + /* Now deal with the output. If we can (exclusively) open the
> + output file directly, we can just use that. */
> + fnew = xstrdup (foutput == NULL ? fname : foutput);
> + fdnew = open (fnew, O_WRONLY | O_CREAT | O_EXCL | O_TRUNC,
> + st.st_mode & ALLPERMS);
I believe O_EXCL | O_CREAT makes O_TRUNC redundant. This open only
succeeds if a new file is created, in which case there is nothing to
truncate.
> +
> + /* 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 = 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);
> + 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;
> }
>
> @@ -1352,22 +1406,28 @@ 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;
> + }
> +
> + /* Success! Now just cleanup. */
> res = 0;
>
> cleanup:
> @@ -1377,13 +1437,20 @@ cleanup:
> elf_end (elfnew);
> close (fdnew);
>
> - if (fnew != NULL)
> + free (fnew);
> +
> + if (tempname != NULL)
> {
> - unlink (fnew);
> - free (fnew);
> - fnew = NULL;
> + unlinkat (dirfd, tempname, 0);
On Tue, Jun 2, 2026 at 6:30 AM Mark Wielaard <[email protected]> wrote:
> On Mon, 2026-06-01 at 20:23 -0400, Aaron Merey wrote:
> > We don't ever unlink fnew, so if we created it during open and later there
> > is an error, we will leave an empty or partial file around. We could add a
> > flag indicating whether we created fnew to determine whether or not to
> > unlink
> > it during error cleanup.
>
> The "flag" is tempname. If it is set then we need to unlink tempname.
> fnew is always set, but is the name/path of the output file in case we
> can open it directly/exclusively. If not we split fnew (realpath) and
> create tempname to write to (temporarily). There were a couple of
> places where fnew was used were we should have used tempname. I have
> added a comment to fnew because I obviously was confused myself.
In both v1 and v2 tempname tracks if we made a temp file but not whether
we created a new file by O_EXCL | O_CREAT. So for cases like `-o NEWFILE`
(where NEWFILE didn't already exist), an empty/partial file will still
be left on disk if there's an error after the file is created.
Aaron
> + free (tempname);
> }
>
> + 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..8e51e1c1ac7f
> --- /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
>
