From dean-clamav@arctic.org Sun Mar  7 16:39:05 2004
Date: Sun, 7 Mar 2004 14:16:39 -0800 (PST)
From: dean gaudet <dean-clamav@arctic.org>
To: Nigel Horne <njh@bandsman.co.uk>
Cc: bugs@clamav.net
Subject: Re: 3.3x speed improvement
X-comment: visit http://arctic.org/~dean/legal for information regarding
    copyright and disclaimer.

On Sun, 7 Mar 2004, Nigel Horne wrote:

> On Sunday 07 Mar 2004 3:19 am, dean gaudet wrote:
> > while investigating whether i wanted to enable clamav on all my email i
> > discovered that it was using /dev/urandom... "using" is a light term -- it
> > is literally abusing /dev/urandom.
>
> This has already been addressed.
>
> From the ChangeLog:
>
> Mon Feb 23 00:43:44 CET 2004 (tk)
> ---------------------------------
>   * libclamav: cl_rndnum: do not use buffered fread() (thanks to Nigel)

yeah but using urandom for this is complete overkill.  there are
guaranteed race-free library functions mkstemp/mkdtemp which do not need
the cpu intensive urandom.

also -- the cvs code is still throwing away 3 of every 4 bytes read from
urandom.  and there's no need to run the output of /dev/urandom through
md5 because urandom is generally already the output of sha1 or md5.  the
patch below cuts system time in half in my measurements.

it would still be better to fix the api.

-dean

Index: libclamav/others.c
===================================================================
RCS file: /cvsroot/clamav/clamav-devel/libclamav/others.c,v
retrieving revision 1.13
diff -u -r1.13 others.c
--- libclamav/others.c	22 Feb 2004 23:46:03 -0000	1.13
+++ libclamav/others.c	7 Mar 2004 21:50:04 -0000
@@ -259,43 +259,75 @@
   return rand() % max;
 }

+/* it uses MD5 to avoid potential races in tmp */
+char *cl_gentemp(const char *dir)
+{
+	char *name, *mdir, *tmp;
+	unsigned char salt[32];
+	int cnt=0, i;
+	struct stat foo;
+
+    if(!dir)
+	mdir = "/tmp";
+    else
+	mdir = (char *) dir;
+
+    name = (char*) cli_calloc(strlen(mdir) + 1 + 16 + 1, sizeof(char));
+    if(name == NULL) {
+	cli_dbgmsg("cl_gentemp('%s'): out of memory\n", dir);
+	return NULL;
+    }
+    cnt += sprintf(name, "%s/", mdir);
+
+    do {
+	for(i = 0; i < 32; i++)
+	    salt[i] = cl_rndnum(255);
+
+	tmp = cl_md5buff(salt, 32);
+	strncat(name, tmp, 16);
+	free(tmp);
+    } while(stat(name, &foo) != -1);
+
+    return(name);
+}
+
 #else

 unsigned int cl_rndnum(unsigned int max)
 {
 	int fd;
 	unsigned int generated;
-	char *byte;
-	int size;
-
+	int ret;

     if((fd = open("/dev/urandom", O_RDONLY)) < 0) {
 	cli_errmsg("!Can't open /dev/urandom.\n");
 	return -1;
     }

-    byte = (char *) &generated;
-    size = sizeof(generated);
-    do {
-	int bread;
-	bread = read(fd, byte, 1);
-	size -= bread;
-	byte += bread;
-    } while(size > 0);
+    ret = read(fd, &generated, sizeof(generated));
+    if (ret <= 0) {
+	cli_errmsg("!Error reading /dev/urandom.\n");
+	close(fd);
+	return -1;
+    }

     close(fd);
     return generated % max;
 }
-#endif

-/* it uses MD5 to avoid potential races in tmp */
 char *cl_gentemp(const char *dir)
 {
-	char *name, *mdir, *tmp;
-	unsigned char salt[32];
+	int fd, ret;
+	char *name, *mdir;
+	unsigned char rnd[8];
 	int cnt=0, i;
 	struct stat foo;

+    if((fd = open("/dev/urandom", O_RDONLY)) < 0) {
+	cli_errmsg("!Can't open /dev/urandom.\n");
+	return NULL;
+    }
+
     if(!dir)
 	mdir = "/tmp";
     else
@@ -304,21 +336,27 @@
     name = (char*) cli_calloc(strlen(mdir) + 1 + 16 + 1, sizeof(char));
     if(name == NULL) {
 	cli_dbgmsg("cl_gentemp('%s'): out of memory\n", dir);
+	close(fd);
 	return NULL;
     }
     cnt += sprintf(name, "%s/", mdir);

     do {
-	for(i = 0; i < 32; i++)
-	    salt[i] = cl_rndnum(255);
+	ret = read(fd, rnd, sizeof(rnd));
+	if (ret <= 0) {
+	    cli_errmsg("!Error reading /dev/urandom.\n");
+	    close(fd);
+	    return NULL;
+	}

-	tmp = cl_md5buff(salt, 32);
-	strncat(name, tmp, 16);
-	free(tmp);
+	sprintf(name, "%s/%02x%02x%02x%02x%02x%02x%02x%02x", mdir,
+	    rnd[0], rnd[1], rnd[2], rnd[3], rnd[4], rnd[5], rnd[6], rnd[7]);
     } while(stat(name, &foo) != -1);

+    close(fd);
     return(name);
 }
+#endif

 int cli_rmdirs(const char *dirname)
 {