[klibc] [PATCH] Add minimal mkstemp(3) implementation.

[maximilian attems]

sorry this sems to have gone unreviewed, here a first quick go.

On Sun, 27 Feb 2011, Thorsten Glaser wrote:

> This uses time, ASLR and pid for randomisation. (Closes: #516774)

why do we want mkstemp(3), please add the motiviation directly
in the patch body.
 
> Signed-off-by: Thorsten Glaser <tg at mirbsd.org>
> ---
>  usr/include/stdlib.h |    2 +
>  usr/klibc/Kbuild     |    2 +-
>  usr/klibc/mkstemp.c  |   93 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 96 insertions(+), 1 deletions(-)
>  create mode 100644 usr/klibc/mkstemp.c
> 
> diff --git a/usr/include/stdlib.h b/usr/include/stdlib.h
> index 406f446..4706f71 100644
> --- a/usr/include/stdlib.h
> +++ b/usr/include/stdlib.h
> @@ -79,6 +79,8 @@ static __inline__ void srandom(unsigned int __s)
>  	srand48(__s);
>  }
>  
> +__extern int mkstemp(char *);
> +
>  /* Basic PTY functions.  These only work if devpts is mounted! */
>  
>  __extern int unlockpt(int);
> diff --git a/usr/klibc/Kbuild b/usr/klibc/Kbuild
> index af40367..62a3268 100644
> --- a/usr/klibc/Kbuild
> +++ b/usr/klibc/Kbuild
> @@ -18,7 +18,7 @@ klib-y := vsnprintf.o snprintf.o vsprintf.o sprintf.o \
>  	  setpgrp.o getpgrp.o daemon.o \
>  	  printf.o vprintf.o fprintf.o vfprintf.o perror.o \
>  	  statfs.o fstatfs.o umount.o \
> -	  creat.o open.o openat.o open_cloexec.o \
> +	  creat.o mkstemp.o open.o openat.o open_cloexec.o \

belongs more to stdlib.h stuff after env, getenv() and friends.

>  	  fopen.o fread.o fread2.o fgetc.o fgets.o \
>  	  fwrite.o fwrite2.o fputc.o fputs.o puts.o putchar.o \
>  	  sleep.o usleep.o strtotimespec.o strtotimeval.o \
> diff --git a/usr/klibc/mkstemp.c b/usr/klibc/mkstemp.c
> new file mode 100644
> index 0000000..8430357
> --- /dev/null
> +++ b/usr/klibc/mkstemp.c
> @@ -0,0 +1,93 @@
> +/*-
> + * Compact mkstemp(3)-only implementation, for klibc
useless info once merged.
> + *
> + * Copyright (c) 2009, 2011
> + *	Thorsten Glaser <tg at mirbsd.de>
> + *
> + * This file is available under the same terms ("historic
> + * permission clause") as klibc itself or under the terms
> + * of The MirOS Licence (dual licenced).
> + */

please no new license, could it stay just under first?

> +
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/time.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +
> +int
> +mkstemp(char *template)
> +{
> +	int i;
> +	char *cp, *sp;
> +	union {
> +		unsigned short seed[3];		/* for jrand48 */
> +		uint64_t stuffing;		/* for seeding */
> +	} u;
> +	struct stat sbuf;
> +	struct timeval tv;
> +
> +	/* time ensures basic distribution */
> +	gettimeofday(&tv, NULL);
> +	u.stuffing = tv.tv_sec;
> +	u.stuffing *= 1000000;
> +	/* stack is randomised on all Linux platforms */
> +	u.stuffing ^= (uint64_t)(ptrdiff_t)&i;
> +	/* more time */
> +	u.stuffing += tv.tv_usec;
> +	/* pid ensures this differs after fork() */
> +	u.stuffing ^= (uint64_t)getpid();
> +
> +	cp = template;
> +	while (*cp)
> +		++cp;
> +
> +	sp = cp;
> +	/* generate random suffix */
> +	while (sp > template && sp[-1] == 'X') {
> +		i = (int)((unsigned int)jrand48(u.seed) % (26 + 26));
> +		*--sp = i < 26 ? 'A' + i : 'a' + i - 26;
> +	}
> +	if (sp == cp) {
> +		/* zero-length template or no X at its end */
> +		errno = EINVAL;
> +		return (-1);

useless braces, happens at several return places..

> +	}
> +
> +	/* check the target directory */
> +	cp = sp;
> +	while (cp > template && *cp != '/')
> +		--cp;
> +	if (cp > template) {
> +		*cp = '\0';
> +		i = stat(template, &sbuf);
> +		*cp = '/';
> +
> +		if (i != 0)
> +			/* stat failed, pass errno */
> +			return (-1);
> +		if (!S_ISDIR(sbuf.st_mode)) {
> +			errno = ENOTDIR;
> +			return (-1);
> +		}
> +	}
> +
> +	while (1) {
> +		if ((i = open(template, O_CREAT|O_EXCL|O_RDWR, 0600)) >= 0 ||
> +		    errno != EEXIST)

please put the '||` not at the line ending, but at the start of the next
logical block.

> +			break;
> +
> +		while (*cp == 'Z')
> +			if (!*++cp)
> +				return (-1);
this block I don't understand too, what's up with 'Z'?

> +		if (*cp == 'z')
> +			*cp = 'A';
> +		else
> +			++*cp;
this could use a comment too.
> +	}
> +	return (i);
useless braces..
> +}
> -- 
> 1.6.3.1

I didn't test it yet. could you resubmit with aboves fixed?

-- 
maks