bug#8545: issues with recent doprnt-related changes

[Paul Eggert <eggert@cs.ucla.edu>]

On 04/27/11 12:34, Eli Zaretskii wrote:

> I added more checks, thanks for pointing this out.

Thanks, but I don't see the need for this newly-added check:

          if (fmt > format_end)                                                 
            fmt = format_end;                                                   

If fmt is actually greater than format_end, it's pointing past the end
of an object, so the C code is relying on undefined behavior and the
check therefore isn't portable.  But how can fmt ever be greater than
format_end here?  I suggest removing the check.

> ??? %-10s means to print a string LEFT-justified, and the code handles
> that in this loop...
> for %s and %c the "0" flag is not supported anyway (as stated in the
> comments) and GCC flags that with a warning.

Good points both; thanks.

>> A quick second scan found a minor bug in size parsing: the
>> expression "n >= SIZE_MAX / 10" should be "n > SIZE_MAX / 10".
> 
> When they get to messages as long as SIZE_MAX, let them sue me for
> taking away one byte.

It's not a question of saving space at run-time.  It's a question of
helping the reader.  The reader is left wondering: why is that ">="
there?  And why is there another test "n * 10 > SIZE_MAX - (fmt[1] -
'0')" that always returns 0, no matter what?  Code like that will
puzzle future maintainers, who may well mess it up later because they
don't know what's going on.  Also, most printf implementations will
mess up if given a width or precision greater than INT_MAX, so I
suggest not allowing widths or precisions greater than that.  In summary,
I suggest replacing this:

  if (n >= SIZE_MAX / 10
       || n * 10 > SIZE_MAX - (fmt[1] - '0'))
     error ("Format width or precision too large");
 
with this:

  /* Avoid int overflow, because many sprintfs seriously mess up
     with widths or precisions greater than INT_MAX.  Avoid size_t
     overflow, since our counters use size_t.  This test is slightly
     conservative, for speed and simplicity.  */
  if (n >= min (INT_MAX, SIZE_MAX) / 10)
     error ("Format width or precision too large");
 
> "MOST_POSITIVE_FIXNUM + 1" is too much, since MOST_POSITIVE_FIXNUM
> should be able to cover the terminating null character in Emacs.

Why?  Emacs size fields count the bytes in the string, and does not
count the terminating null byte (which is not part of the string).


In briefly reviewing the new version, I found that doprnt
doesn't support the "ll" modifier for "long long", and thus wouldn't
port to platforms that use long long for EMACS_INT.  This is easy to
fix, so I installed a fix for that.  I also found three more problems:

* doprnt invokes strlen to find the length of the format.  The
  vsnprintf code didn't need to do that: it traversed the format once.
  Surely it shouldn't be hard to change doprnt so that it traverses
  the format once rather than twice.

* Sometimes verror will incorrectly truncate a string, even when there
  is plenty of memory.  verror might call doprnt (buffer, SIZE, m, m +
  mlen, ap), and doprnt might discover that a multibyte character is
  chopped in half at the end of the output buffer, and might return
  (say) SIZE - 2.  verror will incorrectly conclude that the output
  was just fine, but it wasn't complete.

* verror might invoke doprnt two or more times, which means that
  doprnt will traverse ap twice.  This does not work in general; the C
  standard is quite clear that the behavior is undefined in this case.
  One way to fix this would be to modify doprnt so that it always
  walks through the format completely, and generates all the output
  that it is going to generate, and that it reallocates the output
  buffer as needed as it goes.  This would require an API change.