[PATCH] toplev: use HOST_WIDE_INT for local_tick to prevent overflow

[PATCH] toplev: use HOST_WIDE_INT for local_tick to prevent overflow

[Jason A. Donenfeld]

In gcc/toplev.c, there's the comment:

  /* A local time stamp derived from the time of compilation. It will be
     zero if the system cannot provide a time.  It will be -1u, if the
     user has specified a particular random seed.  */
  unsigned local_tick;

This is affirmed by init_local_tick()'s comment:

  /* Initialize local_tick with the time of day, or -1 if
     flag_random_seed is set.  */
  static void init_local_tick (void)

And indeed we see it assigned -1 when flag_random_seed != NULL:

  else
    local_tick = -1;

So far so good. However, in the case where flag_random_seed == NULL,
local_tick is assigned like this:

  struct timeval tv;
  gettimeofday (&tv, NULL);
  local_tick = (unsigned) tv.tv_sec * 1000 + tv.tv_usec / 1000;

local_tick is currently of type "unsigned int". Somewhat often, that
expression calculates to be 0xffffffff, which means local_tick winds up
being -1 even when flag_random_seed == NULL.

Interestingly enough, Jakub already fixed one such overflow with that
assignment with 3db31fd1cc7 ("toplev.c (init_local_tick): Avoid signed
integer multiplication overflow."), but this patch missed the integer
size issue.

This is a problem for plugins that follow the documentation comments in
order to determine whether -frandom-seed is being used. To work around
this bug, these plugins must either call get_random_seed(noinit=true) in
their plugin init functions and check there whether the return value is
zero, or more laboriously reparse common_deferred_options or
save_decoded_options. If they use a local_tick==-1 check, once in a blue
moon, it'll be wrong.

Actually, one such user of this isn't a plugin and is actually in tree:
coverage.cc, which unlink()s a file that it shouldn't from time to time:

  if (!flag_branch_probabilities && flag_test_coverage
      && (!local_tick || local_tick == (unsigned)-1))
    /* Only remove the da file, if we're emitting coverage code and
       cannot uniquely stamp it.  If we can stamp it, libgcov will DTRT.  */
    unlink (da_file_name);

This patch fixes the issue by just making local_tick 64 bits, which
should also allow that timestamp to be a bit more unique as well. This
way there's no possibility of overflowing to -1. It then changes the
comparisons to use the properly typed HOST_WIDE_INT_M1U macro.

Cc: PaX Team <pageexec@freemail.hu>
Cc: Brad Spengler <spender@grsecurity.net>
Cc: Andrew Pinski <pinskia@gcc.gnu.org>
Cc: Jakub Jelinek <jakub@gcc.gnu.org>
Closes: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171
Fixes: c07e5477521 ("tree.h (default_flag_random_seed): Remove.")
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 gcc/coverage.cc |  4 ++--
 gcc/toplev.cc   | 10 +++++-----
 gcc/toplev.h    |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/gcc/coverage.cc b/gcc/coverage.cc
index 8ece5db680e..aa482152b3b 100644
--- a/gcc/coverage.cc
+++ b/gcc/coverage.cc
@@ -1310,7 +1310,7 @@ coverage_init (const char *filename)
   memcpy (da_file_name + prefix_len, filename, len);
   strcpy (da_file_name + prefix_len + len, GCOV_DATA_SUFFIX);
 
-  bbg_file_stamp = local_tick;
+  bbg_file_stamp = (unsigned) local_tick;
   if (flag_auto_profile)
     read_autofdo_file ();
   else if (flag_branch_probabilities)
@@ -1360,7 +1360,7 @@ coverage_finish (void)
     unlink (bbg_file_name);
 
   if (!flag_branch_probabilities && flag_test_coverage
-      && (!local_tick || local_tick == (unsigned)-1))
+      && (!local_tick || local_tick == HOST_WIDE_INT_M1U))
     /* Only remove the da file, if we're emitting coverage code and
        cannot uniquely stamp it.  If we can stamp it, libgcov will DTRT.  */
     unlink (da_file_name);
diff --git a/gcc/toplev.cc b/gcc/toplev.cc
index 2d432fb2d84..7c6badeb052 100644
--- a/gcc/toplev.cc
+++ b/gcc/toplev.cc
@@ -135,9 +135,9 @@ const char * current_function_func_begin_label;
 static const char *flag_random_seed;
 
 /* A local time stamp derived from the time of compilation. It will be
-   zero if the system cannot provide a time.  It will be -1u, if the
+   zero if the system cannot provide a time.  It will be -1, if the
    user has specified a particular random seed.  */
-unsigned local_tick;
+unsigned HOST_WIDE_INT local_tick;
 
 /* Random number for this compilation */
 HOST_WIDE_INT random_seed;
@@ -248,19 +248,19 @@ init_local_tick (void)
 	struct timeval tv;
 
 	gettimeofday (&tv, NULL);
-	local_tick = (unsigned) tv.tv_sec * 1000 + tv.tv_usec / 1000;
+	local_tick = (unsigned HOST_WIDE_INT) tv.tv_sec * 1000 + tv.tv_usec / 1000;
       }
 #else
       {
 	time_t now = time (NULL);
 
 	if (now != (time_t)-1)
-	  local_tick = (unsigned) now;
+	  local_tick = (unsigned HOST_WIDE_INT) now;
       }
 #endif
     }
   else
-    local_tick = -1;
+    local_tick = HOST_WIDE_INT_M1U;
 }
 
 /* Obtain the random_seed.  Unless NOINIT, initialize it if
diff --git a/gcc/toplev.h b/gcc/toplev.h
index a82ef8b8fd3..4468396b725 100644
--- a/gcc/toplev.h
+++ b/gcc/toplev.h
@@ -74,7 +74,7 @@ extern void dump_profile_report (void);
 extern void target_reinit (void);
 
 /* A unique local time stamp, might be zero if none is available.  */
-extern unsigned local_tick;
+extern unsigned HOST_WIDE_INT local_tick;
 
 /* See toplev.cc.  */
 extern int flag_rerun_cse_after_global_opts;
-- 
2.35.1

Re: [PATCH] toplev: use HOST_WIDE_INT for local_tick to prevent overflow

[Andrew Pinski]

On Wed, Apr 6, 2022 at 3:44 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> In gcc/toplev.c, there's the comment:
>
>   /* A local time stamp derived from the time of compilation. It will be
>      zero if the system cannot provide a time.  It will be -1u, if the
>      user has specified a particular random seed.  */
>   unsigned local_tick;
>
> This is affirmed by init_local_tick()'s comment:
>
>   /* Initialize local_tick with the time of day, or -1 if
>      flag_random_seed is set.  */
>   static void init_local_tick (void)
>
> And indeed we see it assigned -1 when flag_random_seed != NULL:
>
>   else
>     local_tick = -1;
>
> So far so good. However, in the case where flag_random_seed == NULL,
> local_tick is assigned like this:
>
>   struct timeval tv;
>   gettimeofday (&tv, NULL);
>   local_tick = (unsigned) tv.tv_sec * 1000 + tv.tv_usec / 1000;
>
> local_tick is currently of type "unsigned int". Somewhat often, that
> expression calculates to be 0xffffffff, which means local_tick winds up
> being -1 even when flag_random_seed == NULL.
>
> Interestingly enough, Jakub already fixed one such overflow with that
> assignment with 3db31fd1cc7 ("toplev.c (init_local_tick): Avoid signed
> integer multiplication overflow."), but this patch missed the integer
> size issue.
>
> This is a problem for plugins that follow the documentation comments in
> order to determine whether -frandom-seed is being used. To work around
> this bug, these plugins must either call get_random_seed(noinit=true) in
> their plugin init functions and check there whether the return value is
> zero, or more laboriously reparse common_deferred_options or
> save_decoded_options. If they use a local_tick==-1 check, once in a blue
> moon, it'll be wrong.
>
> Actually, one such user of this isn't a plugin and is actually in tree:
> coverage.cc, which unlink()s a file that it shouldn't from time to time:
>
>   if (!flag_branch_probabilities && flag_test_coverage
>       && (!local_tick || local_tick == (unsigned)-1))
>     /* Only remove the da file, if we're emitting coverage code and
>        cannot uniquely stamp it.  If we can stamp it, libgcov will DTRT.  */
>     unlink (da_file_name);


You are totally reading this comment incorrectly.
When there is a timestamp, the compiler does not need to delete the da
file as it will be ignored. So if there was no valid timestamp/tick
for it will be removed so the merge in libgcov code does get mix
matches.
So removing it for 0 or -1u is fine and does nothing special really
because it would have been done anyways.

Thanks.
Andrew Pinski

>
> This patch fixes the issue by just making local_tick 64 bits, which
> should also allow that timestamp to be a bit more unique as well. This
> way there's no possibility of overflowing to -1. It then changes the
> comparisons to use the properly typed HOST_WIDE_INT_M1U macro.
>
> Cc: PaX Team <pageexec@freemail.hu>
> Cc: Brad Spengler <spender@grsecurity.net>
> Cc: Andrew Pinski <pinskia@gcc.gnu.org>
> Cc: Jakub Jelinek <jakub@gcc.gnu.org>
> Closes: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171
> Fixes: c07e5477521 ("tree.h (default_flag_random_seed): Remove.")
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  gcc/coverage.cc |  4 ++--
>  gcc/toplev.cc   | 10 +++++-----
>  gcc/toplev.h    |  2 +-
>  3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/gcc/coverage.cc b/gcc/coverage.cc
> index 8ece5db680e..aa482152b3b 100644
> --- a/gcc/coverage.cc
> +++ b/gcc/coverage.cc
> @@ -1310,7 +1310,7 @@ coverage_init (const char *filename)
>    memcpy (da_file_name + prefix_len, filename, len);
>    strcpy (da_file_name + prefix_len + len, GCOV_DATA_SUFFIX);
>
> -  bbg_file_stamp = local_tick;
> +  bbg_file_stamp = (unsigned) local_tick;
>    if (flag_auto_profile)
>      read_autofdo_file ();
>    else if (flag_branch_probabilities)
> @@ -1360,7 +1360,7 @@ coverage_finish (void)
>      unlink (bbg_file_name);
>
>    if (!flag_branch_probabilities && flag_test_coverage
> -      && (!local_tick || local_tick == (unsigned)-1))
> +      && (!local_tick || local_tick == HOST_WIDE_INT_M1U))
>      /* Only remove the da file, if we're emitting coverage code and
>         cannot uniquely stamp it.  If we can stamp it, libgcov will DTRT.  */
>      unlink (da_file_name);
> diff --git a/gcc/toplev.cc b/gcc/toplev.cc
> index 2d432fb2d84..7c6badeb052 100644
> --- a/gcc/toplev.cc
> +++ b/gcc/toplev.cc
> @@ -135,9 +135,9 @@ const char * current_function_func_begin_label;
>  static const char *flag_random_seed;
>
>  /* A local time stamp derived from the time of compilation. It will be
> -   zero if the system cannot provide a time.  It will be -1u, if the
> +   zero if the system cannot provide a time.  It will be -1, if the
>     user has specified a particular random seed.  */
> -unsigned local_tick;
> +unsigned HOST_WIDE_INT local_tick;
>
>  /* Random number for this compilation */
>  HOST_WIDE_INT random_seed;
> @@ -248,19 +248,19 @@ init_local_tick (void)
>         struct timeval tv;
>
>         gettimeofday (&tv, NULL);
> -       local_tick = (unsigned) tv.tv_sec * 1000 + tv.tv_usec / 1000;
> +       local_tick = (unsigned HOST_WIDE_INT) tv.tv_sec * 1000 + tv.tv_usec / 1000;
>        }
>  #else
>        {
>         time_t now = time (NULL);
>
>         if (now != (time_t)-1)
> -         local_tick = (unsigned) now;
> +         local_tick = (unsigned HOST_WIDE_INT) now;
>        }
>  #endif
>      }
>    else
> -    local_tick = -1;
> +    local_tick = HOST_WIDE_INT_M1U;
>  }
>
>  /* Obtain the random_seed.  Unless NOINIT, initialize it if
> diff --git a/gcc/toplev.h b/gcc/toplev.h
> index a82ef8b8fd3..4468396b725 100644
> --- a/gcc/toplev.h
> +++ b/gcc/toplev.h
> @@ -74,7 +74,7 @@ extern void dump_profile_report (void);
>  extern void target_reinit (void);
>
>  /* A unique local time stamp, might be zero if none is available.  */
> -extern unsigned local_tick;
> +extern unsigned HOST_WIDE_INT local_tick;
>
>  /* See toplev.cc.  */
>  extern int flag_rerun_cse_after_global_opts;
> --
> 2.35.1
>

Re: [PATCH] toplev: use HOST_WIDE_INT for local_tick to prevent overflow

[Richard Biener]

On Thu, Apr 7, 2022 at 12:44 AM Jason A. Donenfeld via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> In gcc/toplev.c, there's the comment:
>
>   /* A local time stamp derived from the time of compilation. It will be
>      zero if the system cannot provide a time.  It will be -1u, if the
>      user has specified a particular random seed.  */
>   unsigned local_tick;
>
> This is affirmed by init_local_tick()'s comment:
>
>   /* Initialize local_tick with the time of day, or -1 if
>      flag_random_seed is set.  */
>   static void init_local_tick (void)
>
> And indeed we see it assigned -1 when flag_random_seed != NULL:
>
>   else
>     local_tick = -1;
>
> So far so good. However, in the case where flag_random_seed == NULL,
> local_tick is assigned like this:
>
>   struct timeval tv;
>   gettimeofday (&tv, NULL);
>   local_tick = (unsigned) tv.tv_sec * 1000 + tv.tv_usec / 1000;
>
> local_tick is currently of type "unsigned int". Somewhat often, that
> expression calculates to be 0xffffffff, which means local_tick winds up
> being -1 even when flag_random_seed == NULL.
>
> Interestingly enough, Jakub already fixed one such overflow with that
> assignment with 3db31fd1cc7 ("toplev.c (init_local_tick): Avoid signed
> integer multiplication overflow."), but this patch missed the integer
> size issue.
>
> This is a problem for plugins that follow the documentation comments in
> order to determine whether -frandom-seed is being used. To work around
> this bug, these plugins must either call get_random_seed(noinit=true) in
> their plugin init functions and check there whether the return value is
> zero, or more laboriously reparse common_deferred_options or
> save_decoded_options. If they use a local_tick==-1 check, once in a blue
> moon, it'll be wrong.

While I support using a 64bit type for local_tick please use uint64_t and
not unsigned HOST_WIDE_INT.  For the issue of overloading -1 I'd
instead suggest to do

>   struct timeval tv;
>   gettimeofday (&tv, NULL);
>   local_tick = (unsigned) tv.tv_sec * 1000 + tv.tv_usec / 1000;
     /* Avoid aliasing with the special-meaning -1.  */
     if (local_tick == -1)
       local_tick = 1;

because even with uint64_t the result could be -1, no?

> Actually, one such user of this isn't a plugin and is actually in tree:
> coverage.cc, which unlink()s a file that it shouldn't from time to time:
>
>   if (!flag_branch_probabilities && flag_test_coverage
>       && (!local_tick || local_tick == (unsigned)-1))
>     /* Only remove the da file, if we're emitting coverage code and
>        cannot uniquely stamp it.  If we can stamp it, libgcov will DTRT.  */
>     unlink (da_file_name);
>
> This patch fixes the issue by just making local_tick 64 bits, which
> should also allow that timestamp to be a bit more unique as well. This
> way there's no possibility of overflowing to -1. It then changes the
> comparisons to use the properly typed HOST_WIDE_INT_M1U macro.
>
> Cc: PaX Team <pageexec@freemail.hu>
> Cc: Brad Spengler <spender@grsecurity.net>
> Cc: Andrew Pinski <pinskia@gcc.gnu.org>
> Cc: Jakub Jelinek <jakub@gcc.gnu.org>
> Closes: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171
> Fixes: c07e5477521 ("tree.h (default_flag_random_seed): Remove.")
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  gcc/coverage.cc |  4 ++--
>  gcc/toplev.cc   | 10 +++++-----
>  gcc/toplev.h    |  2 +-
>  3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/gcc/coverage.cc b/gcc/coverage.cc
> index 8ece5db680e..aa482152b3b 100644
> --- a/gcc/coverage.cc
> +++ b/gcc/coverage.cc
> @@ -1310,7 +1310,7 @@ coverage_init (const char *filename)
>    memcpy (da_file_name + prefix_len, filename, len);
>    strcpy (da_file_name + prefix_len + len, GCOV_DATA_SUFFIX);
>
> -  bbg_file_stamp = local_tick;
> +  bbg_file_stamp = (unsigned) local_tick;
>    if (flag_auto_profile)
>      read_autofdo_file ();
>    else if (flag_branch_probabilities)
> @@ -1360,7 +1360,7 @@ coverage_finish (void)
>      unlink (bbg_file_name);
>
>    if (!flag_branch_probabilities && flag_test_coverage
> -      && (!local_tick || local_tick == (unsigned)-1))
> +      && (!local_tick || local_tick == HOST_WIDE_INT_M1U))
>      /* Only remove the da file, if we're emitting coverage code and
>         cannot uniquely stamp it.  If we can stamp it, libgcov will DTRT.  */
>      unlink (da_file_name);
> diff --git a/gcc/toplev.cc b/gcc/toplev.cc
> index 2d432fb2d84..7c6badeb052 100644
> --- a/gcc/toplev.cc
> +++ b/gcc/toplev.cc
> @@ -135,9 +135,9 @@ const char * current_function_func_begin_label;
>  static const char *flag_random_seed;
>
>  /* A local time stamp derived from the time of compilation. It will be
> -   zero if the system cannot provide a time.  It will be -1u, if the
> +   zero if the system cannot provide a time.  It will be -1, if the
>     user has specified a particular random seed.  */
> -unsigned local_tick;
> +unsigned HOST_WIDE_INT local_tick;
>
>  /* Random number for this compilation */
>  HOST_WIDE_INT random_seed;
> @@ -248,19 +248,19 @@ init_local_tick (void)
>         struct timeval tv;
>
>         gettimeofday (&tv, NULL);
> -       local_tick = (unsigned) tv.tv_sec * 1000 + tv.tv_usec / 1000;
> +       local_tick = (unsigned HOST_WIDE_INT) tv.tv_sec * 1000 + tv.tv_usec / 1000;
>        }
>  #else
>        {
>         time_t now = time (NULL);
>
>         if (now != (time_t)-1)
> -         local_tick = (unsigned) now;
> +         local_tick = (unsigned HOST_WIDE_INT) now;
>        }
>  #endif
>      }
>    else
> -    local_tick = -1;
> +    local_tick = HOST_WIDE_INT_M1U;
>  }
>
>  /* Obtain the random_seed.  Unless NOINIT, initialize it if
> diff --git a/gcc/toplev.h b/gcc/toplev.h
> index a82ef8b8fd3..4468396b725 100644
> --- a/gcc/toplev.h
> +++ b/gcc/toplev.h
> @@ -74,7 +74,7 @@ extern void dump_profile_report (void);
>  extern void target_reinit (void);
>
>  /* A unique local time stamp, might be zero if none is available.  */
> -extern unsigned local_tick;
> +extern unsigned HOST_WIDE_INT local_tick;
>
>  /* See toplev.cc.  */
>  extern int flag_rerun_cse_after_global_opts;
> --
> 2.35.1
>

Re: [PATCH] toplev: use HOST_WIDE_INT for local_tick to prevent overflow

[Jason A. Donenfeld]

Hi Richard,

The gcc devs have apparently declined to do anything about this bug
(it's marked as "RESOLVED WONTFIX") and don't care much about plugins
I guess: <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105171>, so I
won't be sending a v2. However,

On Thu, Apr 7, 2022 at 8:30 AM Richard Biener
<richard.guenther@gmail.com> wrote:
> For the issue of overloading -1 I'd
> instead suggest to do
>
> >   struct timeval tv;
> >   gettimeofday (&tv, NULL);
> >   local_tick = (unsigned) tv.tv_sec * 1000 + tv.tv_usec / 1000;
>      /* Avoid aliasing with the special-meaning -1.  */
>      if (local_tick == -1)
>        local_tick = 1;
>
> because even with uint64_t the result could be -1, no?

>>> (2**64 - 1 - 1000 - 1649334804) / 1000 / (365 * 24 * 60 * 60)
584942417.3027719

If we're still using this code in 584 million years, God help us all...

But, anyway, if the devs in that bug report do want to fix this
ultimately but don't want to change the type, your `if (local_tick ==
-1) local_tick = 1` thing there and in the next stanza seems like it'd
fix the problem, and also might make things easier on plugins
straddling versions.

Jason