Opened 7 years ago

Closed 7 years ago

Last modified 7 years 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:
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 by phoudoin, 7 years ago

Nice catch.

Version 0, edited 7 years ago by phoudoin (next)

comment:2 by phoudoin, 7 years ago

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

comment:3 by phoudoin, 7 years ago

Resolution: fixed
Status: in-progressclosed

Fixed in hrev51354.

comment:4 by phoudoin, 7 years ago

I take the private seed road.

comment:5 by david.given, 7 years ago

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 by phoudoin, 7 years ago

Resolution: fixed
Status: closedreopened

comment:7 by phoudoin, 7 years ago

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

comment:8 by phoudoin, 7 years ago

Resolution: fixed
Status: reopenedclosed

Applied your patch in hrev51356. Thanks.

comment:9 by david.given, 7 years ago

Thanks!

Note: See TracTickets for help on using tickets.