Subject: Re: bug#57211: 29.0.50; generate-new-buffer-name sprintf format overflow warning

From: Matt Armstrong <[email-masked]>
Date: Sun, 14 Aug 2022 11:59:54 -0700

"Basil L. Contovounesios" via "Bug reports for GNU Emacs, the Swiss army
knife of text editors" <[email-masked]> writes:

> Severity: minor
>
> Compiling with gcc (Debian 12.1.0-7) 12.1.0 and -Og, I get the following
> -Wformat-overflow warning:
>
> In file included from buffer.c:33:
> buffer.c: In function ‘Fgenerate_new_buffer_name’:
> buffer.c:1167:46: warning: ‘sprintf’ may write a terminating nul past the end of the destination [-Wformat-overflow=]
>  1167 |       AUTO_STRING_WITH_LEN (lnumber, number, sprintf (number, "-%d", i));
>       |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~
> lisp.h:5493:36: note: in definition of macro ‘AUTO_STRING_WITH_LEN’
>  5493 |         ((&(struct Lisp_String) {{{len, -1, 0, (unsigned char *) (str)}}}), \
>       |                                    ^~~
> buffer.c:1167:46: note: ‘sprintf’ output between 3 and 9 bytes into a destination of size 8
>  1167 |       AUTO_STRING_WITH_LEN (lnumber, number, sprintf (number, "-%d", i));
>       |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~
> lisp.h:5493:36: note: in definition of macro ‘AUTO_STRING_WITH_LEN’
>  5493 |         ((&(struct Lisp_String) {{{len, -1, 0, (unsigned char *) (str)}}}), \
>       |                                    ^~~
>
> Can the upper bound 9 ever be achieved?  If so, how?  If not, is this a
> GCC bug?  Either way, is there a way to pacify the warning?
>
> I tried
>
>   snprintf (number, sizeof number, ...)
>
> but got the same warning.
>
> BTW, in the preceding
>
>   int i = r % 1000000;
>
> can the result of % ever exceed INT_MAX?  And do we care either way?

I assume that gcc is concerned about the possibility that i may be
-999999 and the resulting string "--999999" which would overflow the
buffer.

Gcc doesn't know that get_random() returns only non-negative numbers,
and the eassume() call doesn't seem to be enough to convince gcc this
fact, or gcc does not infer i is also non-negative.

Personally, I'd change this code to use a buffer

   INT_BUFSIZE_BOUND(int) + sizeof "-"

bytes large, like the just code below it.  In other words, make it large
enough for type of i, and avoid delicate inferences made about the range
of values stored in i.

The "wasted" bytes in the buffer are minor in comparison to the human
effort required to verify this code for security correctness.  :-)