[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5108583B.4080002@opensource.dyc.edu>
Date: Tue, 29 Jan 2013 18:16:11 -0500
From: "Anthony G. Basile" <basile@...nsource.dyc.edu>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] Add support for mkostemp, mkstemps and mkostemps
Hi Szabolcs,
Thanks for the feedback. All these improvements are easy to implement,
but the random name generator definitely needs a better algorithm. I
just adopted what was already there, but its not good enough.
Here's a simple test program which demonstrates the problem:
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
int main()
{
int i, fd;
char *t = (char *)malloc(7);
for(i = 0; i < 10; i++)
{
strcpy(t, "XXXXXX");
fd = mkstemp(t);
printf("%s\n", t);
close(fd);
unlink(t);
}
return 0;
}
On a glibc system, we get something like:
4FeUYd
gZd1x7
wkq860
y2QfGU
e9rnfO
crdvOH
0P9CnB
m0cLWu
eVuTvo
cxT14h
On uclibc we get
WJMzFT
nvSCqr
fneEMi
DTWxB1
SH4n1C
TwLMQ9
LVyEHe
EihiL4
uaqxr4
xmqe7O
On musl we get
GIJNPM
GJDGKC
GJJBDH
GJOFHL
GKFHON
GKLIPB
GLCDJK
GLHONA
GMABAH
GMGPLG
Let me play around with some different algorithms and resubmit this.
I'll look at what uclibc and glibc do and try to slim them down and
speed them up.
On 01/28/2013 04:37 AM, Szabolcs Nagy wrote:
> * Anthony G. Basile <basile@...nsource.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
--
Anthony G. Basile, Ph. D.
Chair of Information Technology
D'Youville College
Buffalo, NY 14201
(716) 829-8197
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
