bug#50767: 28.0.50; Warnings about snprintf in image.c on armv7l

[Alan Third]

On Thu, Sep 23, 2021 at 06:04:12PM +0100, Basil L. Contovounesios via Bug 
reports for GNU Emacs, the Swiss army knife of text editors wrote:
> Severity: minor
> 
> Compiling image.c on a 32-bit armv7l host gives the following warnings:
> 
> --8<---------------cut here---------------start------------->8---
> image.c: In function ‘svg_load_image’:
> image.c:9999:64: warning: ‘%4d’ directive output may be truncated writing 
> between 4 and 11 bytes
>                  into a region of size between 7 and 8 [-Wformat-truncation=]
>  9999 |       const char *css_spec = 
> "svg{font-family:\"%s\";font-size:%4dpx}";
>       |                                                                ^~~
> image.c:10002:7: note: ‘snprintf’ output 37 or more bytes (assuming 38) into 
> a destination of size 37
> 10002 |       snprintf (css, css_len, css_spec, img->face_font_family, 
> img->face_font_size);
>       |       
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> image.c:10134:7: warning: ‘%f’ directive output may be truncated writing 
> between 3 and 317 bytes
>                  into a region of size between 167 and 187 
> [-Wformat-truncation=]
> 10134 |       "<svg xmlns:xlink=\"http://www.w3.org/1999/xlink\"; "
>       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> image.c:10138:22: note: format string is defined here
> 10138 |       "viewBox=\"0 0 %f %f\">"
>       |                      ^~
> image.c:10134:7: note: assuming directive output of 8 bytes
> 10134 |       "<svg xmlns:xlink=\"http://www.w3.org/1999/xlink\"; "
>       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> image.c:10134:7: note: assuming directive output of 8 bytes
> image.c:10134:7: note: directive argument in the range [0, 16777215]
> image.c:10134:7: note: assuming directive output of 1 byte
> image.c:10161:27: note: ‘snprintf’ output 320 or more bytes (assuming 331) 
> into a destination of size 383
> 10161 |         || buffer_size <= snprintf (wrapped_contents, buffer_size, 
> wrapper,
>       |                           
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 10162 |                                     foreground & 0xFFFFFF, width, 
> height,
>       |                                     
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 10163 |                                     viewbox_width, viewbox_height,
>       |                                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 10164 |                                     background & 0xFFFFFF,
>       |                                     ~~~~~~~~~~~~~~~~~~~~~~
> 10165 |                                     SSDATA (encoded_contents)))
>       |                                     ~~~~~~~~~~~~~~~~~~~~~~~~~~
> --8<---------------cut here---------------end--------------->8---
> 
> What's TRT to do?  Call snprintf twice, the first time to calculate the
> required buffer size?  Calculate more precisely the resulting size of
> each argument?  Inhibit the warnings?  Something else?

This is my code and I'm not sure, which is why there's a FIXME in
there somewhere about it.

> Some questions about the current code:
> 
> > const char *css_spec = "svg{font-family:\"%s\";font-size:%4dpx}";
> 
> Why specifically '%4d' for face_font_size?

I figured it unlikely that anyone would be using a font size of 10000
pixels or larger and I wanted to set an upper limit on the string size.

> > int css_len = strlen (css_spec) + strlen (img->face_font_family);
> > css = xmalloc (css_len);
> > snprintf (css, css_len, css_spec, img->face_font_family, 
> > img->face_font_size);
> > rsvg_handle_set_stylesheet (rsvg_handle, (guint8 *)css, strlen (css), NULL);
> 
> Does css_len not need to include the terminating null byte?

It does. If you add up the length of the spec string which includes
the escape codes, and the length of the font name, then if the font
size does produce it's maximum sized string of 4 characters css_len is
exactly one byte larger than the string length.

> What if xmalloc or snprintf fail?

Doesn't xmalloc causes some sort of error to occur? I'm not sure. If
snprintf fails then I believe we should end up with an invalid CSS
stylesheet and librsvg will ignore it.

Perhaps there's a worse failure mode that I'm missing.

> > css = xmalloc (SBYTES (lcss) + 1);
> > strncpy (css, SSDATA (lcss), SBYTES (lcss));
> > *(css + SBYTES (lcss) + 1) = 0;
> 
> Can this be replaced with xlispstrdup?

Probably.
-- 
Alan Third