Hi Aaron,
On Mon, 2026-06-01 at 20:23 -0400, Aaron Merey wrote:
> > @@ -257,4 +259,52 @@ 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 with must be "XXXXXX", which are replaced by an unique
>
> Looks like the sentence ends early here.
Oops. And it says "must end with must be..." Clearly I not speak
English very goodly... Changed to:
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.
> >
> > @@ -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)
> > + /* 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);
> > +
> > + /* If we cannot open the output exclusively for writing directly
> > + (because it already exists, e.g. it might be the current input
>
> The '(' on this line doesn't have a matching ')'.
yeah, should be after "exists", before ",".
> > + /* 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 fnew_len = strlen (fnew);
> > + tempname = xmalloc (fnew_len + sizeof (".XXXXXX"));
>
> tempname is an absolute path and openat, renameat and unlinkat will ignore
> the dirfd argument when the pathname argument is absolute. This will
> almost always work as intended but if dirfd's path changes unexpectedly
> then renameat below could fail.
Doh. After carefully splitting the (fnew) path we should obviously use
bname here.
size_t bname_len = strlen (bname);
tempname = xmalloc (bname_len + sizeof (".XXXXXX"));
sprintf (tempname, "%s.XXXXXX", bname);
fdnew = xmkstempat (dirfd, tempname);
Unfortunate the testcases didn't catch this. It is somewhat hard to
scramble the paths during the test runs.
> > + sprintf (tempname, "%s.XXXXXX", fnew);
> > + fdnew = xmkstempat (dirfd, tempname);
> > }
> >
> > if (fdnew < 0)
> > {
> > - error (0, errno, "Couldn't create output file %s", fnew);
> > + error (0, errno, "Couldn't create output file %s",
> > + tempname == NULL ? fnew : tempname);
> > /* Since we didn't create it we don't want to try to unlink it. */
> > - free (fnew);
> > - fnew = NULL;
> > + if (tempname != NULL)
> > + {
> > + free (fnew);
> > + fnew = NULL;
> > + }
> > goto cleanup;
> > }
Note this is wrong. It should be:
/* Since we couldn't create it we don't want to try to unlink it. */
if (tempname != NULL)
{
free (tempname);
tempname = NULL;
}
> > @@ -1358,12 +1412,15 @@ process_file (const char *fname)
> > error (0, errno, "Couldn't fchmod %s", fnew);
> >
> > /* 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;
> > - }
> > + 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, fnew);
> > + goto cleanup;
> > + }
> > + }
There is a typo in the error message, it should say bname instead of
fnew. And here, after renameat succeeds, we should clear tempname:
/* Finally replace the old file with the new file. */
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;
}
> > /* We are finally done with the new file, don't unlink it now. */
> > free (fnew);
> > @@ -1377,13 +1434,18 @@ cleanup:
> > elf_end (elfnew);
> > close (fdnew);
> >
> > - if (fnew != NULL)
> > + if (tempname != NULL)
> > {
> > - unlink (fnew);
> > - free (fnew);
>
> fnew used to always get freed here if it was non-NULL. Now it's only freed
> on success and on one error path. The other 'goto cleanup' that aren't
> preceded by freeing fnew results in a leak.
>
> > - fnew = NULL;
> > + unlinkat (dirfd, tempname, 0);
Yeah, you are right, we need to unconditionaly free (fname) under
cleanup.
> 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.
I'll sent a V2.
Thanks,
Mark
