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],[],[]
+ [#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)
+ if (getentropy (r, sizeof (*r)) == 0)
+ return 0;
+#elif defined(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 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;
+ }
+
+ /* 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
