Re: [PATCH] Add support for mkostemp, mkstemps and mkostemps

[Szabolcs Nagy <nsz@port70.net>]

* Anthony G. Basile <basile@opensource.dyc.edu> [2013-01-28 00:06:23 -0500]:
> +#include <string.h>
> +#include <unistd.h>
> +#include <errno.h>
> +#include <time.h>
> +#include <stdint.h>
> +#include "libc.h"

libc.h, errno.h, unistd.h are not used

> +
> +char *__randname(char *template)
> +{
> +	struct timespec ts;
> +	size_t i, l = strlen(template);
> +	unsigned long r;

can be unsigned char r;

> +
> +	/* This assumes that a check for the template
> +	   size has alrady been made */
> +	clock_gettime(CLOCK_REALTIME, &ts);
> +	r = ts.tv_nsec + (uintptr_t)&ts / 16 + (uintptr_t)template;
> +	for (i=1; i<=6; i++, r>>=4)
> +		template[l-i] = 'A'+(r&15);

it seems to use only 4bit entropy based on clock (and fixed addresses)

and (uintptr_t)template does not give much entropy if it's malloced

and if the clock source is bad for some reason (eg cpu clock mod 16ns
is even) then this might not even give different result for each retry
(may be it helps if retry count is included)

> +
> +	return template;
> +}
> diff --git a/src/temp/tempfile.c b/src/temp/tempfile.c

i'd use different name, eg __tempfile.c to signify that it's internal

> new file mode 100644
> index 0000000..93808a6
> --- /dev/null
> +++ b/src/temp/tempfile.c
> @@ -0,0 +1,42 @@
> +#include <string.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <limits.h>
> +#include <errno.h>
> +#include "libc.h"

stdlib.h, limits.h are not used

libc.h is not used
(in the other files it is needed for weak_alias)

> +
> +char *__randname(char *);
> +
> +int __open_tempfile (char *template, int len, int flags)
> +{
> +	if (len < 0) return EINVAL;
> +
> +	int l = strlen(template)-len;

int vs size_t problem

> +	if (l < 6 || strncmp(template+l-6, "XXXXXX",6)) {
> +		errno = EINVAL;
> +		*template = 0;
> +		return -1;
> +	}
> +
> +	/* Null terminate the template before the suffix,
> +	   and save the char for adding back the suffix */
> +	char suffix = template[l];
> +	template[l] = '\0';
> +

i'd do __randname(template, length-suffix)
so pass the template len as an argument

then no suffix saving is needed

> +	int fd, retries = 100, t0 = *template;

100 retries does not make sense if only 4bit entropy is used
(there are 16 different names for the given addresses)

> +	while (retries--) {
> +		if (!*__randname(template)) return -1;
> +		/* Add back the suffix */
> +		template[l] = suffix;
> +		if ((fd = open(template, flags | O_CREAT | O_EXCL, 0600))>=0)
> +			return fd;
> +		if (errno != EEXIST) return -1;
> +		/* this is safe because mktemp verified
> +		 * that we have a valid template string */
> +		template[0] = t0;

i dont understand why template[0] gets clobbered

> +		strcpy(template+l-6, "XXXXXX");

i'd use memcpy or memset to avoid suffix saving

> +	}
> +	return -1;
> +}
> -- 
> 1.7.12.4