bug#8545: issues with recent doprnt-related changes

[Eli Zaretskii]

> Date: Wed, 27 Apr 2011 20:11:52 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: Eli Zaretskii <eliz@gnu.org>, 8545@debbugs.gnu.org
> 
> On 04/27/11 18:32, Juanma Barranquero wrote:
> 
> > A cursory look suggests that fmt == format_end + 1 is possible
> 
> Thanks, I had missed that possibility.  (Evidently your cursory looks
> are better than mine. :-)  A possible patch is below.

I strenuously object to that patch, see below.  Please don't install
it.

> > would it be undefined behavior,
> > as long as the pointer has not been dereferenced?
> 
> Yes.  A portable C program is not allowed to create a pointer that
> doesn't point to an object, with the two exceptions of a null pointer
> and a pointer to the address immediately after an object.  On
> some architectures, attempting to point to random addresses can cause
> exceptions or other undefined behavior.

As I explain in another message, we _can_ dereference this invalid
pointer.  Which is why that test was added in the first place.

> -               size_t n = *fmt - '0';
> -               while (fmt < format_end
> -                      && '0' <= fmt[1] && fmt[1] <= '9')
> +               size_t n = *fmt++ - '0';
> +               while (fmt < format_end && '0' <= *fmt && *fmt <= '9')
>                   {
>                     if (n >= SIZE_MAX / 10
>                         || n * 10 > SIZE_MAX - (fmt[1] - '0'))
>                       error ("Format width or precision too large");
> -                   n = n * 10 + fmt[1] - '0';
> -                   *string++ = *++fmt;
> +                   n = n * 10 + *fmt - '0';
> +                   *string++ = *fmt++;
>                   }
>  
>                 if (size_bound < n)
>                   size_bound = n;
>               }
>             else if (*fmt == '-' || *fmt == ' ' || *fmt == '.' || *fmt == '+')
> -             ;
> +             fmt++;
>             else if (*fmt == 'l')
>               {
>                 long_flag = 1 + (fmt + 1 < format_end && fmt[1] == 'l');
> @@ -218,10 +217,7 @@ doprnt (char *buffer, register size_t bu
>               }
>             else
>               break;
> -           fmt++;
>           }
> -       if (fmt > format_end)
> -         fmt = format_end;

I don't see how this is a better idea.  Instead of a simple two-liner,
which could be commented if its intent isn't clear enough, and which
makes the code 100% verifiable to not dereference anything beyond
format_end, how is it better to sprinkle weird post-increments in
several places?  This totally obfuscates the intent, does NOT allow to
comment it in any reasonable way (because the increments are no longer
in a single place, and serve more than one purpose), makes the code
much harder to grasp and analyze, and makes it almost impossible to
convince ourselves that it will never get past format_end without
unduly complicated analysis.  All that for getting rid of a simple and
clearly correct line??  No, thanks!