Patchwork posix: Fix __gen_tempname iteration entropy (BZ#15813)

login
register
mail settings
Submitter Adhemerval Zanella Netto
Date April 4, 2019, 8:31 a.m.
Message ID <20190404083142.30008-1-adhemerval.zanella@linaro.org>
Download mbox | patch
Permalink /patch/32160/
State New
Headers show

Comments

Adhemerval Zanella Netto - April 4, 2019, 8:31 a.m.
From: Adhemerval Zanella <adhemerval.zanella@linaro.org>

Patch "Do not use HP_TIMING_NOW for random bits (359653aaacad4)" fixed
mostly of the __gen_tempname issues described by BZ#15813. This patch
fixes the remaining one by adding a extra call to random_bits for
eac iteration while trying to create the random name.

The patch also cleanups the tempname implementation since now it
deviates from gnulib counterpart.

Checked on powerpc64le-linux-gnu.

	[BZ #15813]
	* sysdeps/posix/tempname.c: Remove ununsed includes, redundant
	definitions, and defines used only for gnulib.
	(__gen_tempname): Set number of attemps to TMP_MAX and use
	random_bits on eachh iteration.
---
 ChangeLog                |  8 ++++
 sysdeps/posix/tempname.c | 97 ++++------------------------------------
 2 files changed, 17 insertions(+), 88 deletions(-)
Paul Eggert - April 5, 2019, 1:44 a.m.
On 4/4/19 1:31 AM, adhemerval.zanella@linaro.org wrote:
> The patch also cleanups the tempname implementation since now it
> deviates from gnulib counterpart.

Shouldn't we strive to keep them in sync? Does the Gnulib version need
the fix you're proposing for the glibc version?
Adhemerval Zanella Netto - April 9, 2019, 12:07 p.m.
On 04/04/2019 22:44, Paul Eggert wrote:
> On 4/4/19 1:31 AM, adhemerval.zanella@linaro.org wrote:
>> The patch also cleanups the tempname implementation since now it
>> deviates from gnulib counterpart.
> 
> Shouldn't we strive to keep them in sync? Does the Gnulib version need
> the fix you're proposing for the glibc version?
> 

I am not sure if gnulib is willing to add the random_bits implementation
based on clock_gettime, this patch is following Wilco idea to remove the
fallback gettimeofday and related code. Checking on gnulib code it seems
it already have a timespec module, so one can assume clock_gettime is
always supported (even if it calls gettimeofday in the end). I don't mind
sending a second version if gnulib adds the fix.
Yann Droneaud - April 29, 2019, 10:16 a.m.
Le jeudi 04 avril 2019 à 15:31 +0700, adhemerval.zanella@linaro.org a
écrit :
> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> 
> Patch "Do not use HP_TIMING_NOW for random bits (359653aaacad4)"
> fixed
> mostly of the __gen_tempname issues described by BZ#15813. This patch
> fixes the remaining one by adding a extra call to random_bits for
> eac iteration while trying to create the random name.
> 
> The patch also cleanups the tempname implementation since now it
> deviates from gnulib counterpart.
> 
> Checked on powerpc64le-linux-gnu.
> 
> 	[BZ #15813]
> 	* sysdeps/posix/tempname.c: Remove ununsed includes, redundant
> 	definitions, and defines used only for gnulib.
> 	(__gen_tempname): Set number of attemps to TMP_MAX and use
> 	random_bits on eachh iteration.
> ---
>  ChangeLog                |  8 ++++
>  sysdeps/posix/tempname.c | 97 ++++--------------------------------
> ----
>  2 files changed, 17 insertions(+), 88 deletions(-)
> 

For easier review (and maintenance) I would like this patch to be split
in 2 or more patches. At least: one for removing gnulib support, and
one to improve randomness for each retry.

Regards.

Patch

diff --git a/sysdeps/posix/tempname.c b/sysdeps/posix/tempname.c
index de346949b2..ccbab46029 100644
--- a/sysdeps/posix/tempname.c
+++ b/sysdeps/posix/tempname.c
@@ -15,88 +15,18 @@ 
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#if !_LIBC
-# include <config.h>
-# include "tempname.h"
-#endif
-
-#include <sys/types.h>
-#include <assert.h>
-
-#include <errno.h>
-#ifndef __set_errno
-# define __set_errno(Val) errno = (Val)
-#endif
-
 #include <stdio.h>
-#ifndef P_tmpdir
-# define P_tmpdir "/tmp"
-#endif
-#ifndef TMP_MAX
-# define TMP_MAX 238328
-#endif
-#ifndef __GT_FILE
-# define __GT_FILE	0
-# define __GT_DIR	1
-# define __GT_NOCREATE	2
-#endif
-#if !_LIBC && (GT_FILE != __GT_FILE || GT_DIR != __GT_DIR	\
-	       || GT_NOCREATE != __GT_NOCREATE)
-# error report this to bug-gnulib@gnu.org
-#endif
-
-#include <stddef.h>
-#include <stdlib.h>
-#include <string.h>
-
-#include <fcntl.h>
-#include <sys/time.h>
-#include <stdint.h>
 #include <unistd.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <assert.h>
+#include <random-bits.h>
 
-#include <sys/stat.h>
-
-#if _LIBC
-# define struct_stat64 struct stat64
-# define __secure_getenv __libc_secure_getenv
-#else
-# define struct_stat64 struct stat
-# define __gen_tempname gen_tempname
-# define __getpid getpid
-# define __gettimeofday gettimeofday
-# define __mkdir mkdir
-# define __open open
-# define __lxstat64(version, file, buf) lstat (file, buf)
-# define __secure_getenv secure_getenv
-#endif
-
-#ifdef _LIBC
-# include <random-bits.h>
-# define RANDOM_BITS(Var) ((Var) = random_bits ())
-# else
-# define RANDOM_BITS(Var) \
-    {                                                                         \
-      struct timeval tv;                                                      \
-      __gettimeofday (&tv, NULL);                                             \
-      (Var) = ((uint64_t) tv.tv_usec << 16) ^ tv.tv_sec;                      \
-    }
-#endif
-
-/* Use the widest available unsigned type if uint64_t is not
-   available.  The algorithm below extracts a number less than 62**6
-   (approximately 2**35.725) from uint64_t, so ancient hosts where
-   uintmax_t is only 32 bits lose about 3.725 bits of randomness,
-   which is better than not having mkstemp at all.  */
-#if !defined UINT64_MAX && !defined uint64_t
-# define uint64_t uintmax_t
-#endif
-
-#if _LIBC
 /* Return nonzero if DIR is an existent directory.  */
 static int
 direxists (const char *dir)
 {
-  struct_stat64 buf;
+  struct stat64 buf;
   return __xstat64 (_STAT_VER, dir, &buf) == 0 && S_ISDIR (buf.st_mode);
 }
 
@@ -127,7 +57,7 @@  __path_search (char *tmpl, size_t tmpl_len, const char *dir, const char *pfx,
 
   if (try_tmpdir)
     {
-      d = __secure_getenv ("TMPDIR");
+      d = __libc_secure_getenv ("TMPDIR");
       if (d != NULL && direxists (d))
 	dir = d;
       else if (dir != NULL && direxists (dir))
@@ -162,7 +92,6 @@  __path_search (char *tmpl, size_t tmpl_len, const char *dir, const char *pfx,
   sprintf (tmpl, "%.*s/%.*sXXXXXX", (int) dlen, dir, (int) plen, pfx);
   return 0;
 }
-#endif /* _LIBC */
 
 /* These are the characters used in temporary file names.  */
 static const char letters[] =
@@ -190,7 +119,7 @@  __gen_tempname (char *tmpl, int suffixlen, int flags, int kind)
   unsigned int count;
   int fd = -1;
   int save_errno = errno;
-  struct_stat64 st;
+  struct stat64 st;
 
   /* A lower bound on the number of temporary files to attempt to
      generate.  The maximum total number of temporary file names that
@@ -198,15 +127,7 @@  __gen_tempname (char *tmpl, int suffixlen, int flags, int kind)
      necessary to try all of these combinations.  Instead if a reasonable
      number of names is tried (we define reasonable as 62**3) fail to
      give the system administrator the chance to remove the problems.  */
-#define ATTEMPTS_MIN (62 * 62 * 62)
-
-  /* The number of times to attempt to generate a temporary file.  To
-     conform to POSIX, this must be no smaller than TMP_MAX.  */
-#if ATTEMPTS_MIN < TMP_MAX
   unsigned int attempts = TMP_MAX;
-#else
-  unsigned int attempts = ATTEMPTS_MIN;
-#endif
 
   len = strlen (tmpl);
   if (len < 6 + suffixlen || memcmp (&tmpl[len - 6 - suffixlen], "XXXXXX", 6))
@@ -219,10 +140,10 @@  __gen_tempname (char *tmpl, int suffixlen, int flags, int kind)
   XXXXXX = &tmpl[len - 6 - suffixlen];
 
   /* Get some more or less random data.  */
-  RANDOM_BITS (value);
+  value = random_bits ();
   value ^= (uint64_t)__getpid () << 32;
 
-  for (count = 0; count < attempts; value += 7777, ++count)
+  for (count = 0; count < attempts; value += random_bits (), ++count)
     {
       uint64_t v = value;