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 <pagee...@freemail.hu>
> Cc: Brad Spengler <spen...@grsecurity.net>
> Cc: Andrew Pinski <pins...@gcc.gnu.org>
> Cc: Jakub Jelinek <ja...@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 <ja...@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
>
