From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/2695 Path: news.gmane.org!not-for-mail From: "Anthony G. Basile" Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] Add support for mkostemp, mkstemps and mkostemps Date: Tue, 29 Jan 2013 18:16:11 -0500 Message-ID: <5108583B.4080002@opensource.dyc.edu> References: <1359349583-3643-1-git-send-email-basile@opensource.dyc.edu> <20130128093755.GI10600@port70.net> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-Trace: ger.gmane.org 1359501390 26239 80.91.229.3 (29 Jan 2013 23:16:30 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Tue, 29 Jan 2013 23:16:30 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-2696-gllmg-musl=m.gmane.org@lists.openwall.com Wed Jan 30 00:16:48 2013 Return-path: Envelope-to: gllmg-musl@plane.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1U0KQ2-0004cX-6S for gllmg-musl@plane.gmane.org; Wed, 30 Jan 2013 00:16:46 +0100 Original-Received: (qmail 32656 invoked by uid 550); 29 Jan 2013 23:16:28 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 32648 invoked from network); 29 Jan 2013 23:16:27 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130121 Thunderbird/17.0.2 In-Reply-To: <20130128093755.GI10600@port70.net> Xref: news.gmane.org gmane.linux.lib.musl.general:2695 Archived-At: 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 #include #include #include 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 [2013-01-28 00:06:23 -0500]: >> +#include >> +#include >> +#include >> +#include >> +#include >> +#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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#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