posix: Fix __gen_tempname iteration entropy (BZ#15813)

Message ID 20190404083142.30008-1-adhemerval.zanella@linaro.org
State Dropped
Headers

Commit Message

Adhemerval Zanella Netto April 4, 2019, 8:31 a.m. UTC
  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(-)
  

Comments

Paul Eggert April 5, 2019, 1:44 a.m. UTC | #1
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. UTC | #2
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. UTC | #3
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;