bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] inttostr.h: add compile-time buffer overrun checks


From: Jim Meyering
Subject: Re: [PATCH] inttostr.h: add compile-time buffer overrun checks
Date: Mon, 25 Oct 2010 10:22:46 +0200

Paul Eggert wrote:
> On 10/18/2010 11:41 PM, Jim Meyering wrote:
>
>> I have to agree.  The whole point of inttostr functions is to
>> provide a minimal and robust mechanism for converting integral
>> values to strings.
>
> I looked into this some more and came up with a different idea: add a
> primitive umax2str that automatically allocates a big-enough buffer in
> the caller's stack frame.  So, instead of this:
>
>   if (repetition)
>     {
>       char buf[INT_BUFSIZE_BOUND (uintmax_t)];
>       fprintf (stderr, _(" on repetition %s\n"), umaxtostr (repetition, buf));
>     }
>
> the caller can write this:
>
>   if (repetition)
>     fprintf (stderr, _(" on repetition %s\n"), umax2str (repetition));
>
> Sometimes code still needs umaxtostr, so we'll continue to support that,
> and do compile-time checking on the buffer if it's easy, but the idea
> is to prefer umax2str when possible, since it makes code nicer to read
> and easier to maintain.
>
> Some downsides of umax2str, imax2str, etc. are:
>
>  * They return a pointer to storage whose lifetime is that of the
>    enclosing block.  It's possible that someone who doesn't understand
>    how they work, will write code that ends up with dangling pointers.
>
>  * The calling code (though not gnulib) must assume support for
>    C99 compound literals.  Coreutils already assumes a C99 compiler,
>    so I expect this is not a fatal objection for coreutils.
>
>  * GCC generates slightly worse code for umax2str than for umaxtostr,
>    since it unnecessarily stores zeros into the literals.  The number
>    of extra instructions and the pressure on the cache is fairly
>    small, but still, this is annoying.  Perhaps someday we can
>    convince GCC to generate better code.
>
>  * C99 compound literals are not that widely used in this way, and
>    it's possible that we'll tickle compiler bugs.
>
> Anyway, if you like this approach despite the downsides, here is a
> proposed gnulib patch to add support for umax2str etc., while also
> adding compile-time bounds checking for umaxtostr etc. when the
> compile-time checking is easy.  This implementation does not support
> VLA buffers but the need for that is practically zero.

Nice.  You've defined away the problem.
IMHO, the benefits of this approach easily outweigh the downsides.
Those of us concerned about portability (all?) avoid VLAs, so that is
only a theoretical loss.
Those that do not require C99 support need not use the new macros.
It's not often that we can totally remove a source of subtle
size/type-mismatch bugs, like this permits, without sacrificing more.

I confirmed that this works with coreutils, though please defer
pushing the coreutils change set until after the imminent release.

> diff --git a/src/du.c b/src/du.c
> index 4951826..f41eccd 100644
> --- a/src/du.c
> +++ b/src/du.c
> @@ -350,8 +350,7 @@ show_date (const char *format, struct timespec when)
>    struct tm *tm = localtime (&when.tv_sec);
>    if (! tm)
>      {
> -      char buf[INT_BUFSIZE_BOUND (intmax_t)];
> -      char *when_str = timetostr (when.tv_sec, buf);
> +      char *when_str = time2str (when.tv_sec);
>        error (0, 0, _("time %s is out of range"), when_str);
>        fputs (when_str, stdout);
>        return;
> diff --git a/src/expr.c b/src/expr.c
> index dbeaeba..4c412b5 100644
> --- a/src/expr.c
> +++ b/src/expr.c
> @@ -117,8 +117,7 @@ static char *
>  mpz_get_str (char const *str, int base, mpz_t z)
>  {
>    (void) str; (void) base;
> -  char buf[INT_BUFSIZE_BOUND (intmax_t)];
> -  return xstrdup (imaxtostr (z[0], buf));
> +  return xstrdup (imax2str (z[0]));
>  }
>  static int
>  mpz_sgn (mpz_t z)
> @@ -139,8 +138,7 @@ static int
>  mpz_out_str (FILE *stream, int base, mpz_t z)
>  {
>    (void) base;
> -  char buf[INT_BUFSIZE_BOUND (intmax_t)];
> -  return fputs (imaxtostr (z[0], buf), stream) != EOF;
> +  return fputs (imax2str (z[0]), stream) != EOF;
>  }
>  #endif
>
> diff --git a/src/factor.c b/src/factor.c
> index 7291d28..04bd60e 100644
> --- a/src/factor.c
> +++ b/src/factor.c
> @@ -338,11 +338,10 @@ print_factors_single (uintmax_t n)
>    uintmax_t factors[MAX_N_FACTORS];
>    size_t n_factors = factor_wheel (n, MAX_N_FACTORS, factors);
>    size_t i;
> -  char buf[INT_BUFSIZE_BOUND (uintmax_t)];
>
> -  printf ("%s:", umaxtostr (n, buf));
> +  printf ("%s:", umax2str (n));
>    for (i = 0; i < n_factors; i++)
> -    printf (" %s", umaxtostr (factors[i], buf));
> +    printf (" %s", umax2str (factors[i]));
>    putchar ('\n');
>  }
...



reply via email to

[Prev in Thread] Current Thread [Next in Thread]