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

From: Alan Third <[email-masked]>
Date: Mon, 4 Oct 2021 22:45:06 +0100

On Thu, Sep 23, 2021 at 11:49:37PM +0100, Alan Third wrote:
> On Thu, Sep 23, 2021 at 11:38:06PM +0100, Basil L. Contovounesios wrote:
> > ...which would mean this only holds in the common case that
> > face_font_size has fewer than 5 digits, right?
> 
> Looks that way.
> 
> I suppose in this particular case we could limit the font size to a
> maximum of 9999 or something, but surely there's a better way of
> calculating string sizes?

I've implemented a pretty basic check so we shouldn't accidentally
overrun the buffer. See attached.

-- 
Alan Third
>From 1b2ffac09c184ebc58921f2b48eacc3629596cdb Mon Sep 17 00:00:00 2001
From: Alan Third <[email-masked]>
Date: Mon, 4 Oct 2021 22:35:41 +0100
Subject: [PATCH] Fix potential buffer overflow (bug#50767)

* src/image.c (svg_load_image): Check how many bytes were actually
written to the buffer.  Don't check xmalloc return value as xmalloc
doesn't return if it fails.
---
 src/image.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/src/image.c b/src/image.c
index 206c7baa2f..49b26301e8 100644
--- a/src/image.c
+++ b/src/image.c
@@ -9996,10 +9996,16 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
   if (!STRINGP (lcss))
     {
       /* Generate the CSS for the SVG image.  */
-      const char *css_spec = "svg{font-family:\"%s\";font-size:%4dpx}";
-      int css_len = strlen (css_spec) + strlen (img->face_font_family);
+      /* FIXME: The below calculations leave enough space for a font
+	 size up to 9999, if it overflows we just throw an error but
+	 should probably increase the buffer size.  */
+      const char *css_spec = "svg{font-family:\"%s\";font-size:%dpx}";
+      int css_len = strlen (css_spec) + strlen (img->face_font_family) + 1;
       css = xmalloc (css_len);
-      snprintf (css, css_len, css_spec, img->face_font_family, img->face_font_size);
+      if (css_len <= snprintf (css, css_len, css_spec,
+			       img->face_font_family, img->face_font_size))
+	goto rsvg_error;
+
       rsvg_handle_set_stylesheet (rsvg_handle, (guint8 *)css, strlen (css), NULL);
     }
   else
@@ -10157,12 +10163,11 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
 
     wrapped_contents = xmalloc (buffer_size);
 
-    if (!wrapped_contents
-        || buffer_size <= snprintf (wrapped_contents, buffer_size, wrapper,
-                                    foreground & 0xFFFFFF, width, height,
-                                    viewbox_width, viewbox_height,
-                                    background & 0xFFFFFF,
-                                    SSDATA (encoded_contents)))
+    if (buffer_size <= snprintf (wrapped_contents, buffer_size, wrapper,
+				 foreground & 0xFFFFFF, width, height,
+				 viewbox_width, viewbox_height,
+				 background & 0xFFFFFF,
+				 SSDATA (encoded_contents)))
       goto rsvg_error;
 
     wrapped_size = strlen (wrapped_contents);
-- 
2.33.0