From 62f19c197d32e8773a284616d575686d87903b7d Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 08:43:34 -0700 Subject: [PATCH] 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. --- src/ChangeLog | 22 +++++ src/doprnt.c | 224 +++++++++++++++++++++++++++++++++++++------------ src/keyboard.c | 46 ++++++---- src/keymap.c | 19 +++-- src/lisp.h | 14 +++- src/minibuf.c | 8 +- src/window.h | 4 +- 7 files changed, 252 insertions(+), 85 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index bb2a0d20c1f..d60c57dfd53 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,25 @@ +2011-08-29 Paul Eggert + + 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 Integer and memory overflow issues (Bug#9196). diff --git a/src/doprnt.c b/src/doprnt.c index 79f9f36e461..dae1dab04d7 100644 --- a/src/doprnt.c +++ b/src/doprnt.c @@ -70,9 +70,9 @@ along with GNU Emacs. If not, see . */ %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 . */ 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); + } +} diff --git a/src/keyboard.c b/src/keyboard.c index ab93e0ccd24..51eac369e7c 100644 --- a/src/keyboard.c +++ b/src/keyboard.c @@ -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; } diff --git a/src/keymap.c b/src/keymap.c index 32b531daac4..4485080db21 100644 --- a/src/keymap.c +++ b/src/keymap.c @@ -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); diff --git a/src/lisp.h b/src/lisp.h index 99555118047..2f6ec38f228 100644 --- a/src/lisp.h +++ b/src/lisp.h @@ -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 */ diff --git a/src/minibuf.c b/src/minibuf.c index eb564a10ec6..ad8f3ed8b86 100644 --- a/src/minibuf.c +++ b/src/minibuf.c @@ -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 diff --git a/src/window.h b/src/window.h index 485734e907e..c6fa5e7a338 100644 --- a/src/window.h +++ b/src/window.h @@ -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. */ -- 2.39.2