bug#8668: * editfns.c (Fformat): Fix several integer overflow problems

[Eli Zaretskii]

> Date: Sun, 15 May 2011 23:56:59 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> Further inspection has found several other problems with Fformat,
> having to do with large strings and integer overflow.  Here's a
> revised patch to address the problems that I've found.  This
> supersedes the earlier patch.  I'll do more testing on this one
> but thought I'd give people a heads-up.  This patch assumes other
> fixes I've sent in recently.

This uses snprintf where we previously used sprintf.  snprintf is less
portable, and not all supported platforms have a C9x compatible
implementation.  The library used on MSDOS doesn't have snprintf at
all, and those used on MS-Windows may return a negative value when the
buffer is too short (depending on the version of the library), which
your patch doesn't expect.

Is it possible to stay with sprintf?  We only use it for numerical
conversions, whose length can be predicted reliably, no?

> +  error ("Maximum format conversion size exceeded");

I wonder if we could come up with a less cryptic error message.  (What
is a "conversion size"?)  Maybe something like

  Format conversion specifies width/precision that are too large.

> +    if (bufsize <= min_bufsize / bufsize_multiplier)
> +      bufsize = min_bufsize;
> +    else if (bufsize <= max_bufsize / bufsize_multiplier)
> +      bufsize *= bufsize_multiplier;
> +    else if (bufsize <= max_bufsize)
> +      bufsize = max_bufsize;
> +    else
> +      memory_full ();

Why "memory_full"?  You didn't yet allocate such a huge buffer, so
Emacs could still have plenty of memory.  Even after the allocation,
Emacs could still have enough memory.  I think just a call to `error',
or maybe to the new string_overflow or format_overflow functions, is
TRT here, because this is simply another case of the "string too
large" problem, you just succeeded to detect it in advance.  You
actually do call string_overflow and format_overflow elsewhere in the
patch.

> +      if (bufsize - used < additional)                                       
> \
> +     {                                                               \
> +       char *newbuf;                                                 \
> +       if (INT_ADD_OVERFLOW (used, additional)                       \
> +           || max_bufsize < used + additional)                       \
> +         string_overflow ();                                         \
> +       bufsize = used + additional;                                  \
> +       bufsize = (bufsize < max_bufsize / 3 * 2                      \
> +                  ? bufsize + bufsize / 2                            \
> +                  : max_bufsize);                                    \
> +       SAFE_ALLOCA (newbuf, char *, bufsize);                        \
> +       memcpy (newbuf, buf, used);                                   \

This wastes memory, because the old buffer is not released (when
SAFE_ALLOCA uses xmalloc).  I'm not sure it is a good idea to use
SAFE_ALLOCA in a loop like that.  Perhaps xmalloc/xrealloc would be
better here, since you are keeping the previous contents of the
buffer?

> +    EMACS_INT i, extralen = 2 * formatlen + pWIDElen + 1;
> +    if (SIZE_MAX < extralen
> +     || (SIZE_MAX - extralen) / sizeof (struct info) <= nargs)
> +      memory_full ();

I think this memory_full should also be replaced with a call to
`error' or format_overflow (string_overflow doesn't seem to fit here,
IMO), for the same reasons pointed out above.

> +       char conversion;
> ...
> +       conversion = *format;

I think this is wrong (and the original code had it also wrong).
`format' could be a multibyte string, so it isn't right to take a
single byte out of it and assume you've got a full character; it could
be the 1st byte of a multibyte sequence.  It would also trip you here:

> +       else if (! (conversion == 'c' || conversion == 'd'
> +                   || conversion == 'e' || conversion == 'f'
> +                   || conversion == 'g' || conversion == 'i'
> +                   || conversion == 'o' || conversion == 'x'
> +                   || conversion == 'X'))
> +         error ("Invalid format operation %%%c", conversion);

If `conversion' is the first byte of a multibyte sequence, the error
message will show some random value that has no immediate relation to
the actual character after the `%'.

I think you need to make `conversion' an int, and assign its value
with STRING_CHAR.  Or at the very least do that where you call `error'
with the above error message, so that the error identifies the
problematic character.