Reading this quickly on my phone, so apologies if I misread - but it looks to me like this opens the new file world-writable, which would give us a different race condition.

Instead, why don't we add a new function -- p_getumask -- which uses getumask(3) where it's available (eg, Linux) and reimplements it where it's not. This reimplementation can do the create a file and check its mode mechanism that you've implemented here, but it then deletes that file. This removes the world-writable file that may become a configuration file race.

I considered something like that, but it doesn't seem obvious that there's a good place to try to create the file to probe for umask, since there might be, for example, a noexec mount that screws with your probed umask. Now that I think about it, we can just do this twice to avoid the race: once to get the umask and throw away, and once with the proper permissions.

(getumask is apparently vaporware: grepping through the kernel confirms that there isn't a getumask syscall.)

Wait, we don't need to chmod at all, we can just open with that mode directly. chmod was only needed before because mkstemp creates the files with 0600 permissions.

getumask is apparently vaporware: grepping through the kernel confirms that there isn't a getumask syscall.

Well, getumask(3) is a libc function. And a GNU extension. According to the man-page it's equivalent to:

mode_t getumask(void) {
    mode_t mask = umask(0);
    umask(mask);
    return mask;
}

So there is probably really no benefit in using it if it is available.

Pedantic nit: the term "vaporware" refers to software that was only announced but never implemented. This function was implemented, so it doesn't fit the definition. Still, it's pretty useless if you consider portability.

It's declared in the glibc headers, but there's only an implementation on Hurd. If you try to call it on Linux, you'll fail to link.

Sorry, I've been meaning to dig into the Windows failures, but I haven't managed to scrape up the time. The error seems like a leak that presumably should happen on all platforms, but after staring at the code a bit, I don't see where it is.

Maybe the test is returning early and failing to clean up the buffer because one of the assertions is failing on windows? I didn't see anything that indicated that in the logging, but I might have missed it.

Maybe the test is returning early and failing to clean up the buffer because one of the assertions is failing on windows?

That’s correct - our tests don’t tear everything down when they fail, so we’ll detect leaks there. Apologies that this isn’t more obvious; our CI grew rather organically, and leak detection was added on.

We may be able to disable printing the leaks if clar exits with a nonzero exit code. If nothing else, we could print a big banner at the bottom of the output on a failure.

I do have a concern about rand without srand. From a man page:

If no seed value is provided, the rand() function is automatically seeded with a value of 1.

Which would mean that our temporary files are deterministic. I can imagine a denial of service attack based on this - or perhaps more likely, we fail to create new files because we neglected to clean up the old ones accidentally.

Disabled the test on windows, and cleaned up all of the nits: hopefully this revision passes CI.

Closed via #6178