Hi Aaron,
On Wed, 2026-06-03 at 17:12 -0400, Aaron Merey wrote:
> On Tue, Jun 2, 2026 at 6:47 AM Mark Wielaard <[email protected]> wrote:
> > +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.
I would hope 9 years is old enough. But yeah. I'll add a "fallback
getrandom" for systems that don't have it. Meh.
> > + /* 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.
Ack. Removing O_TRUNC.
> > @@ -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.
O, right. And that means we also need to do the "pin directory and use
unlinkat dance in that case to make sure we don't accidentally remove
the wrong file on error.
V3 coming up.
Cheers,
Mark
