toplev: read from /dev/urandom only when needed

Hi,

Most compiler invocations don't actually need an entropy source, so
open-read-close syscall sequence on /dev/urandom that GCC performs on
each startup is useless (and can easily be avoided).

This patch makes GCC read entropy from /dev/urandom lazily on first
call to get_random_seed, and en passant cleans up some of the cruft.

Tested on x86-64, OK to apply?

Thanks.
Alexander

	* toplev.h (set_random_seed): Adjust return type.
	* toplev.c (init_local_tick): Move eager initialization of random_seed
	to get_random_seed.  Adjust comment.
	(init_random_seed): Inline to get_random_seed, delete.
	(get_random_seed): Initialize random_seed lazily.
	(set_random_seed): Do not return previous value.
	(print_switch_value): Do not call get_random_seed.

On Wed, Sep 20, 2017 at 10:39 PM, Alexander Monakov <amonakov@ispras.ru> wrote:
> Hi,
>
> Most compiler invocations don't actually need an entropy source, so
> open-read-close syscall sequence on /dev/urandom that GCC performs on
> each startup is useless (and can easily be avoided).
>
> This patch makes GCC read entropy from /dev/urandom lazily on first
> call to get_random_seed, and en passant cleans up some of the cruft.
>
> Tested on x86-64, OK to apply?

Ok.

Thanks,
Richard.

> Thanks.
> Alexander
>
>         * toplev.h (set_random_seed): Adjust return type.
>         * toplev.c (init_local_tick): Move eager initialization of random_seed
>         to get_random_seed.  Adjust comment.
>         (init_random_seed): Inline to get_random_seed, delete.
>         (get_random_seed): Initialize random_seed lazily.
>         (set_random_seed): Do not return previous value.
>         (print_switch_value): Do not call get_random_seed.
>
> diff --git a/gcc/toplev.c b/gcc/toplev.c
> index 7d2b8fffa0b..da2d7c566b1 100644
> --- a/gcc/toplev.c
> +++ b/gcc/toplev.c
> @@ -239,7 +239,7 @@ announce_function (tree decl)
>      }
>  }
>
> -/* Initialize local_tick with a random number or -1 if
> +/* Initialize local_tick with the time of day, or -1 if
>     flag_random_seed is set.  */
>
>  static void
> @@ -247,19 +247,6 @@ init_local_tick (void)
>  {
>    if (!flag_random_seed)
>      {
> -      /* Try urandom first. Time of day is too likely to collide.
> -        In case of any error we just use the local tick. */
> -
> -      int fd = open ("/dev/urandom", O_RDONLY);
> -      if (fd >= 0)
> -        {
> -          if (read (fd, &random_seed, sizeof (random_seed))
> -              != sizeof (random_seed))
> -            random_seed = 0;
> -          close (fd);
> -        }
> -
> -      /* Now get the tick anyways  */
>  #ifdef HAVE_GETTIMEOFDAY
>        {
>         struct timeval tv;
> @@ -280,34 +267,33 @@ init_local_tick (void)
>      local_tick = -1;
>  }
>
> -/* Set up a default flag_random_seed and local_tick, unless the user
> -   already specified one.  Must be called after init_local_tick.  */
> -
> -static void
> -init_random_seed (void)
> -{
> -  if (!random_seed)
> -    random_seed = local_tick ^ getpid ();  /* Old racey fallback method */
> -}
> -
>  /* Obtain the random_seed.  Unless NOINIT, initialize it if
>     it's not provided in the command line.  */
>
>  HOST_WIDE_INT
>  get_random_seed (bool noinit)
>  {
> -  if (!flag_random_seed && !noinit)
> -    init_random_seed ();
> +  if (!random_seed && !noinit)
> +    {
> +      int fd = open ("/dev/urandom", O_RDONLY);
> +      if (fd >= 0)
> +        {
> +          if (read (fd, &random_seed, sizeof (random_seed))
> +              != sizeof (random_seed))
> +            random_seed = 0;
> +          close (fd);
> +        }
> +      if (!random_seed)
> +       random_seed = local_tick ^ getpid ();
> +    }
>    return random_seed;
>  }
>
> -/* Modify the random_seed string to VAL.  Return its previous
> -   value.  */
> +/* Set flag_random_seed to VAL, and if non-null, reinitialize random_seed.  */
>
> -const char *
> +void
>  set_random_seed (const char *val)
>  {
> -  const char *old = flag_random_seed;
>    flag_random_seed = val;
>    if (flag_random_seed)
>      {
> @@ -318,7 +304,6 @@ set_random_seed (const char *val)
>        if (!(endp > flag_random_seed && *endp == 0))
>          random_seed = crc32_string (0, flag_random_seed);
>      }
> -  return old;
>  }
>
>  /* Handler for fatal signals, such as SIGSEGV.  These are transformed
> @@ -818,11 +803,6 @@ print_switch_values (print_switch_fn_type print_fn)
>    int pos = 0;
>    size_t j;
>
> -  /* Fill in the -frandom-seed option, if the user didn't pass it, so
> -     that it can be printed below.  This helps reproducibility.  */
> -  if (!flag_random_seed)
> -    init_random_seed ();
> -
>    /* Print the options as passed.  */
>    pos = print_single_switch (print_fn, pos,
>                              SWITCH_TYPE_DESCRIPTIVE, _("options passed: "));
> diff --git a/gcc/toplev.h b/gcc/toplev.h
> index 2f6b587d233..aed806eb28f 100644
> --- a/gcc/toplev.h
> +++ b/gcc/toplev.h
> @@ -91,7 +91,7 @@ extern bool set_src_pwd                      (const char *);
>  /* Functions used to manipulate the random seed.  */
>
>  extern HOST_WIDE_INT get_random_seed (bool);
> -extern const char *set_random_seed (const char *);
> +extern void set_random_seed (const char *);
>
>  extern void initialize_rtl (void);
>

On Wed, Sep 20, 2017 at 11:39:26PM +0300, Alexander Monakov wrote:
> --- a/gcc/toplev.c
> +++ b/gcc/toplev.c
> @@ -239,7 +239,7 @@ announce_function (tree decl)
>      }
>  }
>  
> -/* Initialize local_tick with a random number or -1 if
> +/* Initialize local_tick with the time of day, or -1 if
>     flag_random_seed is set.  */
>  
>  static void
> @@ -247,19 +247,6 @@ init_local_tick (void)
>  {
>    if (!flag_random_seed)
>      {
> -      /* Try urandom first. Time of day is too likely to collide. 
> -	 In case of any error we just use the local tick. */
> -
> -      int fd = open ("/dev/urandom", O_RDONLY);
> -      if (fd >= 0)
> -        {
> -          if (read (fd, &random_seed, sizeof (random_seed))
> -              != sizeof (random_seed))
> -            random_seed = 0;
> -          close (fd);
> -        }
> -
> -      /* Now get the tick anyways  */
>  #ifdef HAVE_GETTIMEOFDAY
>        {
>  	struct timeval tv;
> @@ -280,34 +267,33 @@ init_local_tick (void)
>      local_tick = -1;
>  }

Why isn't init_local_tick done at the get_random_seed time too?
I.e. inlined into get_random_seed by hand like you've done for
init_random_seed?

>  /* Obtain the random_seed.  Unless NOINIT, initialize it if
>     it's not provided in the command line.  */
>  
>  HOST_WIDE_INT
>  get_random_seed (bool noinit)
>  {
> -  if (!flag_random_seed && !noinit)
> -    init_random_seed ();
> +  if (!random_seed && !noinit)
> +    {
> +      int fd = open ("/dev/urandom", O_RDONLY);
> +      if (fd >= 0)
> +        {
> +          if (read (fd, &random_seed, sizeof (random_seed))
> +              != sizeof (random_seed))
> +            random_seed = 0;
> +          close (fd);
> +        }
> +      if (!random_seed)
> +	random_seed = local_tick ^ getpid ();

If one is unlucky, this can still yield 0 and multiple get_random_seed
invocations will open /dev/urandom multiple times and what is worse,
one can return 0 and the other some completely different number.
So either we should for the case the above is still 0 set random_seed
to some hardcoded constant other than 0 or return getpid () in that case,
or allow 0 to be returned, but arrange for the initialization to be done
at most once.

	Jakub

On Thu, 21 Sep 2017, Jakub Jelinek wrote:
> Why isn't init_local_tick done at the get_random_seed time too?
> I.e. inlined into get_random_seed by hand like you've done for
> init_random_seed?

init_local_tick initializes the 'local_tick' global variable that
is directly accessed from coverage.c.  Thus, changes in coverage.c
would be needed to avoid init_local_tick at each startup.

> If one is unlucky, this can still yield 0 and multiple get_random_seed
> invocations will open /dev/urandom multiple times and what is worse,
> one can return 0 and the other some completely different number.
> So either we should for the case the above is still 0 set random_seed
> to some hardcoded constant other than 0 or return getpid () in that case,
> or allow 0 to be returned, but arrange for the initialization to be done
> at most once.

Yes (there are pre-existing issues like this too).  The minimal fix could be

  random_seed += !random_seed;

after the getpid fallback, but really this internal random_seed business is
currently a mess and could benefit from some more TLC.  Would you be interested
to review further patches in this area?

Thanks.
Alexander