Opened 21 months ago

Closed 21 months ago

Last modified 21 months ago

#13660 closed bug (fixed)

mktemp() generates completely deterministic filenames

Reported by: david.given Owned by: phoudoin
Priority: normal Milestone: Unscheduled
Component: System/libroot.so Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

mktemp() uses rand() to generate filenames, but rand() doesn't get seeded on process startup. This means that mktemp() will generate precisely the same sequence of filenames on every run.

While this is technically correct, which is the best kind of correct, it's possibly the worst thing it could possibly do --- it _looks_ like it's working, but doesn't. It pretty much guarantees file collisions with parallel builds.

I have since changed my code to use mkstemp() instead, which solves my particular issue, but Haiku's mktemp() shouldn't be doing this. I know that mktemp() is broken by design, but there's so much old software using it that we should really by trying harder. I'd suggest on of:

  • use a real random number generator;
  • keep rand(), but use a private seed based on the pid and timestamp;
  • don't use random numbers at all and start with a constant string before mutating it! It's still technically correct, but now it's at least completely obvious what's happening.

Test program demonstrating the issue follows:

#include <stdlib.h>
#include <stdio.h>
int main() {
  char s[] = "/tmp/XXXXXX";
  mktemp(s);
  printf("%s\n", s);
}

$ gcc test.c
$ ./test
/tmp/temp.mq2NP5
$ ./test
/tmp/temp.mq2NP5
$ ./test
/tmp/temp.mq2NP5
$ ./test
/tmp/temp.mq2NP5

Change History (9)

comment:1 Changed 21 months ago by phoudoin

Nice catch, thanks for reporting it.

Last edited 21 months ago by phoudoin (previous) (diff)

comment:2 Changed 21 months ago by phoudoin

Owner: changed from nobody to phoudoin
Status: newin-progress

comment:3 Changed 21 months ago by phoudoin

Resolution: fixed
Status: in-progressclosed

Fixed in hrev51354.

comment:4 Changed 21 months ago by phoudoin

I take the private seed road.

comment:5 Changed 21 months ago by david.given

Sorry --- that's not going to work!

  • by calling srand() you're overwriting any seed used by the user, so disturbing any random number sequences they've set up.
  • gettimeofday() is not guaranteed to return unique values. If the user calls mktemp() twice in very short succession, with an interval smaller than the timer granularity, then you end up seeding the random number generator twice with the same value, which means the two calls to mktemp() will generate the same filename.

I'd do something like this:

static int seed = 0; // value maintained between calls
if (seed == 0) {
  // seed the PRNG once on first call
  struct timeval tv;
  gettimeofday(&tv, 0);
  seed = (getpid() << 16) ^ getuid() ^ tv.tv_sec ^ tv.tv_usec;
}
...
uint32_t value = rand_r(&seed) % (sizeof(padchar) - 1);

comment:6 Changed 21 months ago by phoudoin

Resolution: fixed
Status: closedreopened

comment:7 Changed 21 months ago by phoudoin

Indeed, PulkoMandy said the same too. I will update the fix tomorrow except if somebody do it before.

comment:8 Changed 21 months ago by phoudoin

Resolution: fixed
Status: reopenedclosed

Applied your patch in hrev51356. Thanks.

comment:9 Changed 21 months ago by david.given

Thanks!

Note: See TracTickets for help on using tickets.