sprintf-related integer and memory overflow issues.
authorPaul Eggert <eggert@cs.ucla.edu>
Mon, 29 Aug 2011 15:43:34 +0000 (08:43 -0700)
committerPaul Eggert <eggert@cs.ucla.edu>
Mon, 29 Aug 2011 15:43:34 +0000 (08:43 -0700)
* doprnt.c (doprnt): Support printing ptrdiff_t and intmax_t values.
(esprintf, esnprintf, exprintf, evxprintf): New functions.
* keyboard.c (command_loop_level): Now EMACS_INT, not int.
(cmd_error): kbd macro iterations count is now EMACS_INT, not int.
(modify_event_symbol): Do not assume that the length of
name_alist_or_stem is safe to alloca and fits in int.
(Fexecute_extended_command): Likewise for function name and binding.
(Frecursion_depth): Wrap around reliably on integer overflow.
* keymap.c (push_key_description): First arg is now EMACS_INT, not int,
since some callers pass EMACS_INT values.
(Fsingle_key_description): Don't crash if symbol name contains more
than MAX_ALLOCA bytes.
* minibuf.c (minibuf_level): Now EMACS_INT, not int.
(get_minibuffer): Arg is now EMACS_INT, not int.
* lisp.h (get_minibuffer, push_key_description): Reflect API changes.
(esprintf, esnprintf, exprintf, evxprintf): New decls.
* window.h (command_loop_level, minibuf_level): Reflect API changes.

src/ChangeLog
src/doprnt.c
src/keyboard.c
src/keymap.c
src/lisp.h
src/minibuf.c
src/window.h

index bb2a0d2..d60c57d 100644 (file)
@@ -1,3 +1,25 @@
+2011-08-29  Paul Eggert  <eggert@cs.ucla.edu>
+
+       sprintf-related integer and memory overflow issues.
+
+       * doprnt.c (doprnt): Support printing ptrdiff_t and intmax_t values.
+       (esprintf, esnprintf, exprintf, evxprintf): New functions.
+       * keyboard.c (command_loop_level): Now EMACS_INT, not int.
+       (cmd_error): kbd macro iterations count is now EMACS_INT, not int.
+       (modify_event_symbol): Do not assume that the length of
+       name_alist_or_stem is safe to alloca and fits in int.
+       (Fexecute_extended_command): Likewise for function name and binding.
+       (Frecursion_depth): Wrap around reliably on integer overflow.
+       * keymap.c (push_key_description): First arg is now EMACS_INT, not int,
+       since some callers pass EMACS_INT values.
+       (Fsingle_key_description): Don't crash if symbol name contains more
+       than MAX_ALLOCA bytes.
+       * minibuf.c (minibuf_level): Now EMACS_INT, not int.
+       (get_minibuffer): Arg is now EMACS_INT, not int.
+       * lisp.h (get_minibuffer, push_key_description): Reflect API changes.
+       (esprintf, esnprintf, exprintf, evxprintf): New decls.
+       * window.h (command_loop_level, minibuf_level): Reflect API changes.
+
 2011-08-26  Paul Eggert  <eggert@cs.ucla.edu>
 
        Integer and memory overflow issues (Bug#9196).
index 79f9f36..dae1dab 100644 (file)
@@ -70,9 +70,9 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
      %<flags><width><precision><length>character
 
    where flags is [+ -0], width is [0-9]+, precision is .[0-9]+, and length
-   is empty or l or the value of the pI macro.  Also, %% in a format
-   stands for a single % in the output.  A % that does not introduce a
-   valid %-sequence causes undefined behavior.
+   is empty or l or the value of the pD or pI or pMd (sans "d") macros.
+   Also, %% in a format stands for a single % in the output.  A % that
+   does not introduce a valid %-sequence causes undefined behavior.
 
    The + flag character inserts a + before any positive number, while a space
    inserts a space before any positive number; these flags only affect %d, %o,
@@ -85,8 +85,10 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
    modifier: it is supported for %d, %o, and %x conversions of integral
    arguments, must immediately precede the conversion specifier, and means that
    the respective argument is to be treated as `long int' or `unsigned long
-   int'.  Similarly, the value of the pI macro means to use EMACS_INT or
-   EMACS_UINT and the empty length modifier means `int' or `unsigned int'.
+   int'.  Similarly, the value of the pD macro means to use ptrdiff_t,
+   the value of the pI macro means to use EMACS_INT or EMACS_UINT, the
+   value of the pMd etc. macros means to use intmax_t or uintmax_t,
+   and the empty length modifier means `int' or `unsigned int'.
 
    The width specifier supplies a lower limit for the length of the printed
    representation.  The padding, if any, normally goes on the left, but it goes
@@ -173,8 +175,17 @@ doprnt (char *buffer, ptrdiff_t bufsize, const char *format,
        {
          ptrdiff_t size_bound = 0;
          EMACS_INT width;  /* Columns occupied by STRING on display.  */
-         int long_flag = 0;
-         int pIlen = sizeof pI - 1;
+         enum {
+           pDlen = sizeof pD - 1,
+           pIlen = sizeof pI - 1,
+           pMlen = sizeof pMd - 2
+         };
+         enum {
+           no_modifier, long_modifier, pD_modifier, pI_modifier, pM_modifier
+         } length_modifier = no_modifier;
+         static char const modifier_len[] = { 0, 1, pDlen, pIlen, pMlen };
+         int maxmlen = max (max (1, pDlen), max (pIlen, pMlen));
+         int mlen;
 
          fmt++;
          /* Copy this one %-spec into fmtcpy.  */
@@ -213,19 +224,26 @@ doprnt (char *buffer, ptrdiff_t bufsize, const char *format,
              fmt++;
            }
 
-         if (0 < pIlen && pIlen <= format_end - fmt
-             && memcmp (fmt, pI, pIlen) == 0)
+         /* Check for the length modifiers in textual length order, so
+            that longer modifiers override shorter ones.  */
+         for (mlen = 1; mlen <= maxmlen; mlen++)
            {
-             long_flag = 2;
-             memcpy (string, fmt + 1, pIlen);
-             string += pIlen;
-             fmt += pIlen;
-           }
-         else if (fmt < format_end && *fmt == 'l')
-           {
-             long_flag = 1;
-             *string++ = *++fmt;
+             if (format_end - fmt < mlen)
+               break;
+             if (mlen == 1 && *fmt == 'l')
+               length_modifier = long_modifier;
+             if (mlen == pDlen && memcmp (fmt, pD, pDlen) == 0)
+               length_modifier = pD_modifier;
+             if (mlen == pIlen && memcmp (fmt, pI, pIlen) == 0)
+               length_modifier = pI_modifier;
+             if (mlen == pMlen && memcmp (fmt, pMd, pMlen) == 0)
+               length_modifier = pM_modifier;
            }
+
+         mlen = modifier_len[length_modifier];
+         memcpy (string, fmt + 1, mlen);
+         string += mlen;
+         fmt += mlen;
          *string = 0;
 
          /* Make the size bound large enough to handle floating point formats
@@ -252,55 +270,78 @@ doprnt (char *buffer, ptrdiff_t bufsize, const char *format,
 /*         case 'b': */
            case 'l':
            case 'd':
-             {
-               int i;
-               long l;
-
-               if (1 < long_flag)
+             switch (length_modifier)
+               {
+               case no_modifier:
                  {
-                   EMACS_INT ll = va_arg (ap, EMACS_INT);
-                   sprintf (sprintf_buffer, fmtcpy, ll);
+                   int v = va_arg (ap, int);
+                   sprintf (sprintf_buffer, fmtcpy, v);
                  }
-               else if (long_flag)
+                 break;
+               case long_modifier:
                  {
-                   l = va_arg(ap, long);
-                   sprintf (sprintf_buffer, fmtcpy, l);
+                   long v = va_arg (ap, long);
+                   sprintf (sprintf_buffer, fmtcpy, v);
                  }
-               else
+                 break;
+               case pD_modifier:
+               signed_pD_modifier:
                  {
-                   i = va_arg(ap, int);
-                   sprintf (sprintf_buffer, fmtcpy, i);
+                   ptrdiff_t v = va_arg (ap, ptrdiff_t);
+                   sprintf (sprintf_buffer, fmtcpy, v);
                  }
-               /* Now copy into final output, truncating as necessary.  */
-               string = sprintf_buffer;
-               goto doit;
-             }
+                 break;
+               case pI_modifier:
+                 {
+                   EMACS_INT v = va_arg (ap, EMACS_INT);
+                   sprintf (sprintf_buffer, fmtcpy, v);
+                 }
+                 break;
+               case pM_modifier:
+                 {
+                   intmax_t v = va_arg (ap, intmax_t);
+                   sprintf (sprintf_buffer, fmtcpy, v);
+                 }
+                 break;
+               }
+             /* Now copy into final output, truncating as necessary.  */
+             string = sprintf_buffer;
+             goto doit;
 
            case 'o':
            case 'x':
-             {
-               unsigned u;
-               unsigned long ul;
-
-               if (1 < long_flag)
+             switch (length_modifier)
+               {
+               case no_modifier:
                  {
-                   EMACS_UINT ull = va_arg (ap, EMACS_UINT);
-                   sprintf (sprintf_buffer, fmtcpy, ull);
+                   unsigned v = va_arg (ap, unsigned);
+                   sprintf (sprintf_buffer, fmtcpy, v);
                  }
-               else if (long_flag)
+                 break;
+               case long_modifier:
                  {
-                   ul = va_arg(ap, unsigned long);
-                   sprintf (sprintf_buffer, fmtcpy, ul);
+                   unsigned long v = va_arg (ap, unsigned long);
+                   sprintf (sprintf_buffer, fmtcpy, v);
                  }
-               else
+                 break;
+               case pD_modifier:
+                 goto signed_pD_modifier;
+               case pI_modifier:
                  {
-                   u = va_arg(ap, unsigned);
-                   sprintf (sprintf_buffer, fmtcpy, u);
+                   EMACS_UINT v = va_arg (ap, EMACS_UINT);
+                   sprintf (sprintf_buffer, fmtcpy, v);
                  }
-               /* Now copy into final output, truncating as necessary.  */
-               string = sprintf_buffer;
-               goto doit;
-             }
+                 break;
+               case pM_modifier:
+                 {
+                   uintmax_t v = va_arg (ap, uintmax_t);
+                   sprintf (sprintf_buffer, fmtcpy, v);
+                 }
+                 break;
+               }
+             /* Now copy into final output, truncating as necessary.  */
+             string = sprintf_buffer;
+             goto doit;
 
            case 'f':
            case 'e':
@@ -426,3 +467,82 @@ doprnt (char *buffer, ptrdiff_t bufsize, const char *format,
   SAFE_FREE ();
   return bufptr - buffer;
 }
+
+/* Format to an unbounded buffer BUF.  This is like sprintf, except it
+   is not limited to returning an 'int' so it doesn't have a silly 2
+   GiB limit on typical 64-bit hosts.  However, it is limited to the
+   Emacs-style formats that doprnt supports.
+
+   Return the number of bytes put into BUF, excluding the terminating
+   '\0'.  */
+ptrdiff_t
+esprintf (char *buf, char const *format, ...)
+{
+  ptrdiff_t nbytes;
+  va_list ap;
+  va_start (ap, format);
+  nbytes = doprnt (buf, TYPE_MAXIMUM (ptrdiff_t), format, 0, ap);
+  va_end (ap);
+  return nbytes;
+}
+
+/* Format to a buffer BUF of positive size BUFSIZE.  This is like
+   snprintf, except it is not limited to returning an 'int' so it
+   doesn't have a silly 2 GiB limit on typical 64-bit hosts.  However,
+   it is limited to the Emacs-style formats that doprnt supports, and
+   BUFSIZE must be positive.
+
+   Return the number of bytes put into BUF, excluding the terminating
+   '\0'.  Unlike snprintf, always return a nonnegative value less than
+   BUFSIZE; if the output is truncated, return BUFSIZE - 1, which is
+   the length of the truncated output.  */
+ptrdiff_t
+esnprintf (char *buf, ptrdiff_t bufsize, char const *format, ...)
+{
+  ptrdiff_t nbytes;
+  va_list ap;
+  va_start (ap, format);
+  nbytes = doprnt (buf, bufsize, format, 0, ap);
+  va_end (ap);
+  return nbytes;
+}
+
+/* Format to buffer *BUF of positive size *BUFSIZE, reallocating *BUF
+   and updating *BUFSIZE if the buffer is too small, and otherwise
+   behaving line esprintf.  When reallocating, free *BUF unless it is
+   equal to NONHEAPBUF, and if BUFSIZE_MAX is nonnegative then signal
+   memory exhaustion instead of growing the buffer size past
+   BUFSIZE_MAX.  */
+ptrdiff_t
+exprintf (char **buf, ptrdiff_t *bufsize,
+         char const *nonheapbuf, ptrdiff_t bufsize_max,
+         char const *format, ...)
+{
+  ptrdiff_t nbytes;
+  va_list ap;
+  va_start (ap, format);
+  nbytes = evxprintf (buf, bufsize, nonheapbuf, bufsize_max, format, ap);
+  va_end (ap);
+  return nbytes;
+}
+
+/* Act like exprintf, except take a va_list.  */
+ptrdiff_t
+evxprintf (char **buf, ptrdiff_t *bufsize,
+          char const *nonheapbuf, ptrdiff_t bufsize_max,
+          char const *format, va_list ap)
+{
+  for (;;)
+    {
+      ptrdiff_t nbytes;
+      va_list ap_copy;
+      va_copy (ap_copy, ap);
+      nbytes = doprnt (*buf, *bufsize, format, 0, ap_copy);
+      va_end (ap_copy);
+      if (nbytes < *bufsize - 1)
+       return nbytes;
+      if (*buf != nonheapbuf)
+       xfree (*buf);
+      *buf = xpalloc (NULL, bufsize, 1, bufsize_max, 1);
+    }
+}
index ab93e0c..51eac36 100644 (file)
@@ -196,7 +196,7 @@ int immediate_quit;
 int quit_char;
 
 /* Current depth in recursive edits.  */
-int command_loop_level;
+EMACS_INT command_loop_level;
 
 /* If not Qnil, this is a switch-frame event which we decided to put
    off until the end of a key sequence.  This should be read as the
@@ -998,7 +998,8 @@ static Lisp_Object
 cmd_error (Lisp_Object data)
 {
   Lisp_Object old_level, old_length;
-  char macroerror[50];
+  char macroerror[sizeof "After..kbd macro iterations: "
+                 + INT_STRLEN_BOUND (EMACS_INT)];
 
 #ifdef HAVE_WINDOW_SYSTEM
   if (display_hourglass_p)
@@ -1010,7 +1011,7 @@ cmd_error (Lisp_Object data)
       if (executing_kbd_macro_iterations == 1)
        sprintf (macroerror, "After 1 kbd macro iteration: ");
       else
-       sprintf (macroerror, "After %d kbd macro iterations: ",
+       sprintf (macroerror, "After %"pI"d kbd macro iterations: ",
                 executing_kbd_macro_iterations);
     }
   else
@@ -6463,11 +6464,15 @@ modify_event_symbol (EMACS_INT symbol_num, unsigned int modifiers, Lisp_Object s
        value = Fcdr_safe (Fassq (symbol_int, name_alist_or_stem));
       else if (STRINGP (name_alist_or_stem))
        {
-         int len = SBYTES (name_alist_or_stem);
-         char *buf = (char *) alloca (len + 50);
-          sprintf (buf, "%s-%"pI"d", SDATA (name_alist_or_stem),
-                   XINT (symbol_int) + 1);
+         char *buf;
+         ptrdiff_t len = (SBYTES (name_alist_or_stem)
+                          + sizeof "-" + INT_STRLEN_BOUND (EMACS_INT));
+         USE_SAFE_ALLOCA;
+         SAFE_ALLOCA (buf, char *, len);
+         esprintf (buf, "%s-%"pI"d", SDATA (name_alist_or_stem),
+                   XINT (symbol_int) + 1);
          value = intern (buf);
+         SAFE_FREE ();
        }
       else if (name_table != 0 && name_table[symbol_num])
        value = intern (name_table[symbol_num]);
@@ -6483,7 +6488,7 @@ modify_event_symbol (EMACS_INT symbol_num, unsigned int modifiers, Lisp_Object s
 
       if (NILP (value))
        {
-         char buf[20];
+         char buf[sizeof "key-" + INT_STRLEN_BOUND (EMACS_INT)];
          sprintf (buf, "key-%"pI"d", symbol_num);
          value = intern (buf);
        }
@@ -10382,19 +10387,21 @@ give to the command you invoke, if it asks for an argument.  */)
          char *newmessage;
          int message_p = push_message ();
          int count = SPECPDL_INDEX ();
+         ptrdiff_t newmessage_len, newmessage_alloc;
+         USE_SAFE_ALLOCA;
 
          record_unwind_protect (pop_message_unwind, Qnil);
          binding = Fkey_description (bindings, Qnil);
-
-         newmessage
-           = (char *) alloca (SCHARS (SYMBOL_NAME (function))
-                              + SBYTES (binding)
-                              + 100);
-         sprintf (newmessage, "You can run the command `%s' with %s",
-                  SDATA (SYMBOL_NAME (function)),
-                  SDATA (binding));
+         newmessage_alloc =
+           (sizeof "You can run the command `' with "
+            + SBYTES (SYMBOL_NAME (function)) + SBYTES (binding));
+         SAFE_ALLOCA (newmessage, char *, newmessage_alloc);
+         newmessage_len =
+           esprintf (newmessage, "You can run the command `%s' with %s",
+                     SDATA (SYMBOL_NAME (function)),
+                     SDATA (binding));
          message2 (newmessage,
-                   strlen (newmessage),
+                   newmessage_len,
                    STRING_MULTIBYTE (binding));
          if (NUMBERP (Vsuggest_key_bindings))
            waited = sit_for (Vsuggest_key_bindings, 0, 2);
@@ -10404,6 +10411,7 @@ give to the command you invoke, if it asks for an argument.  */)
          if (!NILP (waited) && message_p)
            restore_message ();
 
+         SAFE_FREE ();
          unbind_to (count, Qnil);
        }
     }
@@ -10633,7 +10641,9 @@ DEFUN ("recursion-depth", Frecursion_depth, Srecursion_depth, 0, 0, 0,
   (void)
 {
   Lisp_Object temp;
-  XSETFASTINT (temp, command_loop_level + minibuf_level);
+  /* Wrap around reliably on integer overflow.  */
+  EMACS_INT sum = (command_loop_level & INTMASK) + (minibuf_level & INTMASK);
+  XSETINT (temp, sum);
   return temp;
 }
 
index 32b531d..4485080 100644 (file)
@@ -2143,12 +2143,12 @@ spaces are put between sequence elements, etc.  */)
 
 
 char *
-push_key_description (register unsigned int c, register char *p, int force_multibyte)
+push_key_description (EMACS_INT ch, char *p, int force_multibyte)
 {
-  unsigned c2;
+  int c, c2;
 
   /* Clear all the meaningless bits above the meta bit.  */
-  c &= meta_modifier | ~ - meta_modifier;
+  c = ch & (meta_modifier | ~ - meta_modifier);
   c2 = c & ~(alt_modifier | ctrl_modifier | hyper_modifier
             | meta_modifier | shift_modifier | super_modifier);
 
@@ -2283,10 +2283,15 @@ around function keys and event symbols.  */)
     {
       if (NILP (no_angles))
        {
-         char *buffer
-           = (char *) alloca (SBYTES (SYMBOL_NAME (key)) + 5);
-         sprintf (buffer, "<%s>", SDATA (SYMBOL_NAME (key)));
-         return build_string (buffer);
+         char *buffer;
+         Lisp_Object result;
+         USE_SAFE_ALLOCA;
+         SAFE_ALLOCA (buffer, char *,
+                      sizeof "<>" + SBYTES (SYMBOL_NAME (key)));
+         esprintf (buffer, "<%s>", SDATA (SYMBOL_NAME (key)));
+         result = build_string (buffer);
+         SAFE_FREE ();
+         return result;
        }
       else
        return Fsymbol_name (key);
index 9955511..2f6ec38 100644 (file)
@@ -2895,6 +2895,16 @@ extern void syms_of_print (void);
 /* Defined in doprnt.c */
 extern ptrdiff_t doprnt (char *, ptrdiff_t, const char *, const char *,
                         va_list);
+extern ptrdiff_t esprintf (char *, char const *, ...)
+  ATTRIBUTE_FORMAT_PRINTF (2, 3);
+extern ptrdiff_t esnprintf (char *, ptrdiff_t, char const *, ...)
+  ATTRIBUTE_FORMAT_PRINTF (3, 4);
+extern ptrdiff_t exprintf (char **, ptrdiff_t *, char const *, ptrdiff_t,
+                          char const *, ...)
+  ATTRIBUTE_FORMAT_PRINTF (5, 6);
+extern ptrdiff_t evxprintf (char **, ptrdiff_t *, char const *, ptrdiff_t,
+                           char const *, va_list)
+  ATTRIBUTE_FORMAT_PRINTF (5, 0);
 
 /* Defined in lread.c.  */
 extern Lisp_Object Qvariable_documentation, Qstandard_input;
@@ -3186,7 +3196,7 @@ EXFUN (Fread_minibuffer, 2);
 EXFUN (Feval_minibuffer, 2);
 EXFUN (Fread_string, 5);
 EXFUN (Fassoc_string, 3);
-extern Lisp_Object get_minibuffer (int);
+extern Lisp_Object get_minibuffer (EMACS_INT);
 extern void init_minibuf_once (void);
 extern void syms_of_minibuf (void);
 
@@ -3250,7 +3260,7 @@ extern void force_auto_save_soon (void);
 extern void init_keyboard (void);
 extern void syms_of_keyboard (void);
 extern void keys_of_keyboard (void);
-extern char *push_key_description (unsigned int, char *, int);
+extern char *push_key_description (EMACS_INT, char *, int);
 
 
 /* Defined in indent.c */
index eb564a1..ad8f3ed 100644 (file)
@@ -49,7 +49,7 @@ static Lisp_Object minibuf_save_list;
 
 /* Depth in minibuffer invocations.  */
 
-int minibuf_level;
+EMACS_INT minibuf_level;
 
 /* The maximum length of a minibuffer history.  */
 
@@ -772,10 +772,10 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
  used for nonrecursive minibuffer invocations.  */
 
 Lisp_Object
-get_minibuffer (int depth)
+get_minibuffer (EMACS_INT depth)
 {
   Lisp_Object tail, num, buf;
-  char name[24];
+  char name[sizeof " *Minibuf-*" + INT_STRLEN_BOUND (EMACS_INT)];
 
   XSETFASTINT (num, depth);
   tail = Fnthcdr (num, Vminibuffer_list);
@@ -787,7 +787,7 @@ get_minibuffer (int depth)
   buf = Fcar (tail);
   if (NILP (buf) || NILP (BVAR (XBUFFER (buf), name)))
     {
-      sprintf (name, " *Minibuf-%d*", depth);
+      sprintf (name, " *Minibuf-%"pI"d*", depth);
       buf = Fget_buffer_create (build_string (name));
 
       /* Although the buffer's name starts with a space, undo should be
index 485734e..c6fa5e7 100644 (file)
@@ -847,11 +847,11 @@ extern Lisp_Object echo_area_window;
 
 /* Depth in recursive edits.  */
 
-extern int command_loop_level;
+extern EMACS_INT command_loop_level;
 
 /* Depth in minibuffer invocations.  */
 
-extern int minibuf_level;
+extern EMACS_INT minibuf_level;
 
 /* true if we should redraw the mode lines on the next redisplay.  */