bug#9412: sprintf-related integer and memory overflow issues

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

On 08/30/11 19:08, Chong Yidong wrote:

> I don't much like the idea of using custom functions like esprintf and
> esnprintf.  They make the code much less clear.

In code that is formatting buffers or strings that might be longer
than 2 GiB, we cannot use sprintf etc.  We need to fix these issues
somehow.  If there's a better way than esprintf etc. please let me know.

Anyway, I don't quite follow why using esprintf etc. makes the calling
code much less clear.  esprintf is like sprintf, except that on 64-bit
hosts esprintf doesn't have sprintf's 2 GiB limit.  If the use of
esprintf is unclear, then surely the use of sprintf is just as unclear.


> I seem to recall that the reason we don't use snprintf is that
> it's not available on all the platforms that Emacs supports.

Although that used to be true, I expect that platforms lacking
snprintf (e.g., Solaris 2.5.1, IRIX 5.3, OSF/1 4.0) are no longer of
practical importance as Emacs porting targets.

That being said, it's easy to allay this concern, by using esnprintf
instead of snprintf in all areas of the code that might run on
ancient platforms.  The following further patch does this.

====

Avoid the use of snprintf.
* font.c (APPEND_SNPRINTF): Remove.
(font_unparse_xlfd):
* xterm.c (x_io_error_quitter):
Use esnprintf, not snprintf.  That way, we don't have to worry
about porting to ancient platforms that lack snprintf.
(x_term_init): Use sprintf, not snprintf.
=== modified file 'src/font.c'
--- src/font.c	2011-08-29 20:57:42 +0000
+++ src/font.c	2011-08-31 05:50:10 +0000
@@ -1285,14 +1285,14 @@
     }
   else
     f[XLFD_AVGWIDTH_INDEX] = "*";
-  len = snprintf (name, nbytes, "-%s-%s-%s-%s-%s-%s-%s-%s-%s-%s-%s",
-		  f[XLFD_FOUNDRY_INDEX], f[XLFD_FAMILY_INDEX],
-		  f[XLFD_WEIGHT_INDEX], f[XLFD_SLANT_INDEX],
-		  f[XLFD_SWIDTH_INDEX], f[XLFD_ADSTYLE_INDEX],
-		  f[XLFD_PIXEL_INDEX], f[XLFD_RESX_INDEX],
-		  f[XLFD_SPACING_INDEX], f[XLFD_AVGWIDTH_INDEX],
-		  f[XLFD_REGISTRY_INDEX]);
-  return len < nbytes ? len : -1;
+  len = esnprintf (name, nbytes, "-%s-%s-%s-%s-%s-%s-%s-%s-%s-%s-%s",
+		   f[XLFD_FOUNDRY_INDEX], f[XLFD_FAMILY_INDEX],
+		   f[XLFD_WEIGHT_INDEX], f[XLFD_SLANT_INDEX],
+		   f[XLFD_SWIDTH_INDEX], f[XLFD_ADSTYLE_INDEX],
+		   f[XLFD_PIXEL_INDEX], f[XLFD_RESX_INDEX],
+		   f[XLFD_SPACING_INDEX], f[XLFD_AVGWIDTH_INDEX],
+		   f[XLFD_REGISTRY_INDEX]);
+  return len == nbytes - 1 ? -1 : len;
 }

 /* Parse NAME (null terminated) and store information in FONT
@@ -1592,39 +1592,32 @@

   p = name;
   lim = name + nbytes;
-# define APPEND_SNPRINTF(args)			\
-    do {					\
-      int len = snprintf args;			\
-      if (! (0 <= len && len < lim - p))	\
-	return -1;				\
-      p += len;					\
-    } while (0)
   if (! NILP (family))
-    APPEND_SNPRINTF ((p, lim - p, "%s", SSDATA (family)));
+    p += esnprintf (p, lim - p, "%s", SSDATA (family));
   if (point_size > 0)
-    APPEND_SNPRINTF ((p, lim - p, "-%d" + (p == name), point_size));
+    p += esnprintf (p, lim - p, "-%d" + (p == name), point_size);
   else if (pixel_size > 0)
-    APPEND_SNPRINTF ((p, lim - p, ":pixelsize=%d", pixel_size));
+    p += esnprintf (p, lim - p, ":pixelsize=%d", pixel_size);
   if (! NILP (AREF (font, FONT_FOUNDRY_INDEX)))
-    APPEND_SNPRINTF ((p, lim - p, ":foundry=%s",
-		      SSDATA (SYMBOL_NAME (AREF (font,
-						 FONT_FOUNDRY_INDEX)))));
+    p += esnprintf (p, lim - p, ":foundry=%s",
+		    SSDATA (SYMBOL_NAME (AREF (font,
+					       FONT_FOUNDRY_INDEX))));
   for (i = 0; i < 3; i++)
     if (! NILP (styles[i]))
-      APPEND_SNPRINTF ((p, lim - p, ":%s=%s", style_names[i],
-			SSDATA (SYMBOL_NAME (styles[i]))));
+      p += esnprintf (p, lim - p, ":%s=%s", style_names[i],
+		      SSDATA (SYMBOL_NAME (styles[i])));
   if (INTEGERP (AREF (font, FONT_DPI_INDEX)))
-    APPEND_SNPRINTF ((p, lim - p, ":dpi=%"pI"d",
-		      XINT (AREF (font, FONT_DPI_INDEX))));
+    p += esnprintf (p, lim - p, ":dpi=%"pI"d",
+		    XINT (AREF (font, FONT_DPI_INDEX)));
   if (INTEGERP (AREF (font, FONT_SPACING_INDEX)))
-    APPEND_SNPRINTF ((p, lim - p, ":spacing=%"pI"d",
-		      XINT (AREF (font, FONT_SPACING_INDEX))));
+    p += esnprintf (p, lim - p, ":spacing=%"pI"d",
+		    XINT (AREF (font, FONT_SPACING_INDEX)));
   if (INTEGERP (AREF (font, FONT_AVGWIDTH_INDEX)))
-    APPEND_SNPRINTF ((p, lim - p,
-		      (XINT (AREF (font, FONT_AVGWIDTH_INDEX)) == 0
-		       ? ":scalable=true"
-		       : ":scalable=false")));
-  return (p - name);
+    p += esnprintf (p, lim - p,
+		    (XINT (AREF (font, FONT_AVGWIDTH_INDEX)) == 0
+		     ? ":scalable=true"
+		     : ":scalable=false"));
+  return lim - p == 1 ? -1 : p - name;
 }

 /* Parse NAME (null terminated) and store information in FONT

=== modified file 'src/xterm.c'
--- src/xterm.c	2011-08-29 20:03:30 +0000
+++ src/xterm.c	2011-08-31 05:24:36 +0000
@@ -7900,8 +7900,8 @@
 {
   char buf[256];

-  snprintf (buf, sizeof buf, "Connection lost to X server `%s'",
-	    DisplayString (display));
+  esnprintf (buf, sizeof buf, "Connection lost to X server `%s'",
+	     DisplayString (display));
   x_connection_closed (display, buf);
   return 0;
 }
@@ -10278,8 +10278,8 @@
       atom_names[i] = (char *) atom_refs[i].name;

     /* Build _XSETTINGS_SN atom name */
-    snprintf (xsettings_atom_name, sizeof (xsettings_atom_name),
-              "_XSETTINGS_S%d", XScreenNumberOfScreen (dpyinfo->screen));
+    sprintf (xsettings_atom_name,
+	     "_XSETTINGS_S%d", XScreenNumberOfScreen (dpyinfo->screen));
     atom_names[i] = xsettings_atom_name;

     XInternAtoms (dpyinfo->display, atom_names, total_atom_count,