From 17107bb698f82bb5b9c8a732cd6b3faaad4d7af6 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Sun, 28 Aug 2011 16:52:34 -0700 Subject: [PATCH 01/33] Integer and memory overflow issues. * emacsclient.c (xmalloc): Accept size_t, not unsigned int, to avoid potential buffer overflow issues on typical 64-bit hosts. Return void *, not long *. (get_current_dir_name): Report a failure, instead of looping forever, if buffer size calculation overflows. Treat malloc failures like realloc failures, as that has better behavior and is more consistent. Do not check whether xmalloc returns NULL, as that's not possible. (message): Do not arbitrarily truncate message to 2048 bytes when sending it to stderr; use vfprintf instead. (get_server_config, set_local_socket) (start_daemon_and_retry_set_socket): Do not alloca arbitrarily-large buffers; that's not safe. (get_server_config, set_local_socket): Do not use sprintf when its result might not fit in 'int'. (set_local_socket): Do not assume uid fits in 'int'. --- lib-src/ChangeLog | 21 ++++++++++ lib-src/emacsclient.c | 95 ++++++++++++++++++++++++++----------------- 2 files changed, 79 insertions(+), 37 deletions(-) diff --git a/lib-src/ChangeLog b/lib-src/ChangeLog index c878d313b70..d056b1a4b81 100644 --- a/lib-src/ChangeLog +++ b/lib-src/ChangeLog @@ -1,3 +1,24 @@ +2011-08-28 Paul Eggert + + Integer and memory overflow issues. + + * emacsclient.c (xmalloc): Accept size_t, not unsigned int, to + avoid potential buffer overflow issues on typical 64-bit hosts. + Return void *, not long *. + (get_current_dir_name): Report a failure, instead of looping + forever, if buffer size calculation overflows. Treat malloc + failures like realloc failures, as that has better behavior and is + more consistent. Do not check whether xmalloc returns NULL, as + that's not possible. + (message): Do not arbitrarily truncate message to 2048 bytes when + sending it to stderr; use vfprintf instead. + (get_server_config, set_local_socket) + (start_daemon_and_retry_set_socket): Do not alloca + arbitrarily-large buffers; that's not safe. + (get_server_config, set_local_socket): Do not use sprintf when its + result might not fit in 'int'. + (set_local_socket): Do not assume uid fits in 'int'. + 2011-07-28 Paul Eggert Assume freestanding C89 headers, string.h, stdlib.h. diff --git a/lib-src/emacsclient.c b/lib-src/emacsclient.c index 2af139aee6d..ece9dc65c49 100644 --- a/lib-src/emacsclient.c +++ b/lib-src/emacsclient.c @@ -194,10 +194,10 @@ struct option longopts[] = /* Like malloc but get fatal error if memory is exhausted. */ -static long * -xmalloc (unsigned int size) +static void * +xmalloc (size_t size) { - long *result = (long *) malloc (size); + void *result = malloc (size); if (result == NULL) { perror ("malloc"); @@ -250,32 +250,33 @@ get_current_dir_name (void) ) { buf = (char *) xmalloc (strlen (pwd) + 1); - if (!buf) - return NULL; strcpy (buf, pwd); } #ifdef HAVE_GETCWD else { size_t buf_size = 1024; - buf = (char *) xmalloc (buf_size); - if (!buf) - return NULL; for (;;) { + int tmp_errno; + buf = malloc (buf_size); + if (! buf) + break; if (getcwd (buf, buf_size) == buf) break; - if (errno != ERANGE) + tmp_errno = errno; + free (buf); + if (tmp_errno != ERANGE) { - int tmp_errno = errno; - free (buf); errno = tmp_errno; return NULL; } buf_size *= 2; - buf = (char *) realloc (buf, buf_size); - if (!buf) - return NULL; + if (! buf_size) + { + errno = ENOMEM; + return NULL; + } } } #else @@ -283,8 +284,6 @@ get_current_dir_name (void) { /* We need MAXPATHLEN here. */ buf = (char *) xmalloc (MAXPATHLEN + 1); - if (!buf) - return NULL; if (getwd (buf) == NULL) { int tmp_errno = errno; @@ -494,16 +493,17 @@ static void message (int, const char *, ...) ATTRIBUTE_FORMAT_PRINTF (2, 3); static void message (int is_error, const char *format, ...) { - char msg[2048]; va_list args; va_start (args, format); - vsprintf (msg, format, args); - va_end (args); #ifdef WINDOWSNT if (w32_window_app ()) { + char msg[2048]; + vsnprintf (msg, sizeof msg, format, args); + msg[sizeof msg - 1] = '\0'; + if (is_error) MessageBox (NULL, msg, "Emacsclient ERROR", MB_ICONERROR); else @@ -514,9 +514,11 @@ message (int is_error, const char *format, ...) { FILE *f = is_error ? stderr : stdout; - fputs (msg, f); + vfprintf (f, format, args); fflush (f); } + + va_end (args); } /* Decode the options from argv and argc. @@ -959,18 +961,24 @@ get_server_config (struct sockaddr_in *server, char *authentication) if (home) { - char *path = alloca (strlen (home) + strlen (server_file) - + EXTRA_SPACE); - sprintf (path, "%s/.emacs.d/server/%s", home, server_file); + char *path = xmalloc (strlen (home) + strlen (server_file) + + EXTRA_SPACE); + strcpy (path, home); + strcat (path, "/.emacs.d/server/"); + strcat (path, server_file); config = fopen (path, "rb"); + free (path); } #ifdef WINDOWSNT if (!config && (home = egetenv ("APPDATA"))) { - char *path = alloca (strlen (home) + strlen (server_file) - + EXTRA_SPACE); - sprintf (path, "%s/.emacs.d/server/%s", home, server_file); + char *path = xmalloc (strlen (home) + strlen (server_file) + + EXTRA_SPACE); + strcpy (path, home); + strcat (path, "/.emacs.d/server/"); + strcat (path, server_file); config = fopen (path, "rb"); + free (path); } #endif } @@ -1243,6 +1251,8 @@ set_local_socket (void) int saved_errno = 0; const char *server_name = "server"; const char *tmpdir IF_LINT ( = NULL); + char *tmpdir_storage = NULL; + char *socket_name_storage = NULL; if (socket_name && !strchr (socket_name, '/') && !strchr (socket_name, '\\')) @@ -1255,6 +1265,8 @@ set_local_socket (void) if (default_sock) { + long uid = geteuid (); + ptrdiff_t tmpdirlen; tmpdir = egetenv ("TMPDIR"); if (!tmpdir) { @@ -1265,17 +1277,19 @@ set_local_socket (void) size_t n = confstr (_CS_DARWIN_USER_TEMP_DIR, NULL, (size_t) 0); if (n > 0) { - tmpdir = alloca (n); + tmpdir = tmpdir_storage = xmalloc (n); confstr (_CS_DARWIN_USER_TEMP_DIR, tmpdir, n); } else #endif tmpdir = "/tmp"; } - socket_name = alloca (strlen (tmpdir) + strlen (server_name) - + EXTRA_SPACE); - sprintf (socket_name, "%s/emacs%d/%s", - tmpdir, (int) geteuid (), server_name); + tmpdirlen = strlen (tmpdir); + socket_name = socket_name_storage = + xmalloc (tmpdirlen + strlen (server_name) + EXTRA_SPACE); + strcpy (socket_name, tmpdir); + sprintf (socket_name + tmpdirlen, "/emacs%ld/", uid); + strcat (socket_name + tmpdirlen, server_name); } if (strlen (socket_name) < sizeof (server.sun_path)) @@ -1309,10 +1323,13 @@ set_local_socket (void) if (pw && (pw->pw_uid != geteuid ())) { /* We're running under su, apparently. */ - socket_name = alloca (strlen (tmpdir) + strlen (server_name) - + EXTRA_SPACE); - sprintf (socket_name, "%s/emacs%d/%s", - tmpdir, (int) pw->pw_uid, server_name); + long uid = pw->pw_uid; + ptrdiff_t tmpdirlen = strlen (tmpdir); + socket_name = xmalloc (tmpdirlen + strlen (server_name) + + EXTRA_SPACE); + strcpy (socket_name, tmpdir); + sprintf (socket_name + tmpdirlen, "/emacs%ld/", uid); + strcat (socket_name + tmpdirlen, server_name); if (strlen (socket_name) < sizeof (server.sun_path)) strcpy (server.sun_path, socket_name); @@ -1322,6 +1339,7 @@ set_local_socket (void) progname, socket_name); exit (EXIT_FAILURE); } + free (socket_name); sock_status = socket_status (server.sun_path); saved_errno = errno; @@ -1331,6 +1349,9 @@ set_local_socket (void) } } + free (socket_name_storage); + free (tmpdir_storage); + switch (sock_status) { case 1: @@ -1526,8 +1547,8 @@ start_daemon_and_retry_set_socket (void) { /* Pass --daemon=socket_name as argument. */ const char *deq = "--daemon="; - char *daemon_arg = alloca (strlen (deq) - + strlen (socket_name) + 1); + char *daemon_arg = xmalloc (strlen (deq) + + strlen (socket_name) + 1); strcpy (daemon_arg, deq); strcat (daemon_arg, socket_name); d_argv[1] = daemon_arg; From 9250f758254937f43a621a2371e3433ce7daa573 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Sun, 28 Aug 2011 16:55:41 -0700 Subject: [PATCH 02/33] * etags.c (xmalloc, xrealloc): Accept size_t, not unsigned int, to avoid potential buffer overflow issues on typical 64-bit hosts. (whatlen_max): New static var. (main): Avoid buffer overflow if subsidiary command length is greater than BUFSIZ or 2*BUFSIZ + 20. Do not use sprintf when its result might not fit in 'int'. --- lib-src/ChangeLog | 7 +++++++ lib-src/etags.c | 44 ++++++++++++++++++++++++++++++++++---------- 2 files changed, 41 insertions(+), 10 deletions(-) diff --git a/lib-src/ChangeLog b/lib-src/ChangeLog index d056b1a4b81..8d46a37ce51 100644 --- a/lib-src/ChangeLog +++ b/lib-src/ChangeLog @@ -19,6 +19,13 @@ result might not fit in 'int'. (set_local_socket): Do not assume uid fits in 'int'. + * etags.c (xmalloc, xrealloc): Accept size_t, not unsigned int, + to avoid potential buffer overflow issues on typical 64-bit hosts. + (whatlen_max): New static var. + (main): Avoid buffer overflow if subsidiary command length is + greater than BUFSIZ or 2*BUFSIZ + 20. Do not use sprintf when its + result might not fit in 'int'. + 2011-07-28 Paul Eggert Assume freestanding C89 headers, string.h, stdlib.h. diff --git a/lib-src/etags.c b/lib-src/etags.c index 522c54ee4a5..9d920565804 100644 --- a/lib-src/etags.c +++ b/lib-src/etags.c @@ -414,8 +414,8 @@ static bool filename_is_absolute (char *f); static void canonicalize_filename (char *); static void linebuffer_init (linebuffer *); static void linebuffer_setlen (linebuffer *, int); -static PTR xmalloc (unsigned int); -static PTR xrealloc (char *, unsigned int); +static PTR xmalloc (size_t); +static PTR xrealloc (char *, size_t); static char searchar = '/'; /* use /.../ searches */ @@ -425,6 +425,7 @@ static char *progname; /* name this program was invoked with */ static char *cwd; /* current working directory */ static char *tagfiledir; /* directory of tagfile */ static FILE *tagf; /* ioptr for tags file */ +static ptrdiff_t whatlen_max; /* maximum length of any 'what' member */ static fdesc *fdhead; /* head of file description list */ static fdesc *curfdp; /* current file description */ @@ -1066,6 +1067,7 @@ main (int argc, char **argv) int current_arg, file_count; linebuffer filename_lb; bool help_asked = FALSE; + ptrdiff_t len; char *optstring; int opt; @@ -1110,6 +1112,9 @@ main (int argc, char **argv) /* This means that a file name has been seen. Record it. */ argbuffer[current_arg].arg_type = at_filename; argbuffer[current_arg].what = optarg; + len = strlen (optarg); + if (whatlen_max < len) + whatlen_max = len; ++current_arg; ++file_count; break; @@ -1118,6 +1123,9 @@ main (int argc, char **argv) /* Parse standard input. Idea by Vivek . */ argbuffer[current_arg].arg_type = at_stdin; argbuffer[current_arg].what = optarg; + len = strlen (optarg); + if (whatlen_max < len) + whatlen_max = len; ++current_arg; ++file_count; if (parsing_stdin) @@ -1160,6 +1168,9 @@ main (int argc, char **argv) case 'r': argbuffer[current_arg].arg_type = at_regexp; argbuffer[current_arg].what = optarg; + len = strlen (optarg); + if (whatlen_max < len) + whatlen_max = len; ++current_arg; break; case 'R': @@ -1198,6 +1209,9 @@ main (int argc, char **argv) { argbuffer[current_arg].arg_type = at_filename; argbuffer[current_arg].what = argv[optind]; + len = strlen (argv[optind]); + if (whatlen_max < len) + whatlen_max = len; ++current_arg; ++file_count; } @@ -1331,7 +1345,9 @@ main (int argc, char **argv) /* From here on, we are in (CTAGS && !cxref_style) */ if (update) { - char cmd[BUFSIZ]; + char *cmd = + xmalloc (strlen (tagfile) + whatlen_max + + sizeof "mv..OTAGS;fgrep -v '\t\t' OTAGS >;rm OTAGS"); for (i = 0; i < current_arg; ++i) { switch (argbuffer[i].arg_type) @@ -1342,12 +1358,17 @@ main (int argc, char **argv) default: continue; /* the for loop */ } - sprintf (cmd, - "mv %s OTAGS;fgrep -v '\t%s\t' OTAGS >%s;rm OTAGS", - tagfile, argbuffer[i].what, tagfile); + strcpy (cmd, "mv "); + strcat (cmd, tagfile); + strcat (cmd, " OTAGS;fgrep -v '\t"); + strcat (cmd, argbuffer[i].what); + strcat (cmd, "\t' OTAGS >"); + strcat (cmd, tagfile); + strcat (cmd, ";rm OTAGS"); if (system (cmd) != EXIT_SUCCESS) fatal ("failed to execute shell command", (char *)NULL); } + free (cmd); append_to_tagfile = TRUE; } @@ -1363,11 +1384,14 @@ main (int argc, char **argv) if (CTAGS) if (append_to_tagfile || update) { - char cmd[2*BUFSIZ+20]; + char *cmd = xmalloc (2 * strlen (tagfile) + sizeof "sort -u -o.."); /* Maybe these should be used: setenv ("LC_COLLATE", "C", 1); setenv ("LC_ALL", "C", 1); */ - sprintf (cmd, "sort -u -o %.*s %.*s", BUFSIZ, tagfile, BUFSIZ, tagfile); + strcpy (cmd, "sort -u -o "); + strcat (cmd, tagfile); + strcat (cmd, " "); + strcat (cmd, tagfile); exit (system (cmd)); } return EXIT_SUCCESS; @@ -6656,7 +6680,7 @@ linebuffer_setlen (linebuffer *lbp, int toksize) /* Like malloc but get fatal error if memory is exhausted. */ static PTR -xmalloc (unsigned int size) +xmalloc (size_t size) { PTR result = (PTR) malloc (size); if (result == NULL) @@ -6665,7 +6689,7 @@ xmalloc (unsigned int size) } static PTR -xrealloc (char *ptr, unsigned int size) +xrealloc (char *ptr, size_t size) { PTR result = (PTR) realloc (ptr, size); if (result == NULL) From 644a0faa36ad8c1e251d198c2bc69f17c8bdb83a Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Sun, 28 Aug 2011 16:57:19 -0700 Subject: [PATCH 03/33] * movemail.c (main): Do not use sprintf when its result might not fit in 'int'. Instead, put the possibly-long file name into the output of pfatal_with_name. --- lib-src/ChangeLog | 4 ++++ lib-src/movemail.c | 7 +++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/lib-src/ChangeLog b/lib-src/ChangeLog index 8d46a37ce51..ca08ece7c39 100644 --- a/lib-src/ChangeLog +++ b/lib-src/ChangeLog @@ -26,6 +26,10 @@ greater than BUFSIZ or 2*BUFSIZ + 20. Do not use sprintf when its result might not fit in 'int'. + * movemail.c (main): Do not use sprintf when its result might not fit + in 'int'. Instead, put the possibly-long file name into the + output of pfatal_with_name. + 2011-07-28 Paul Eggert Assume freestanding C89 headers, string.h, stdlib.h. diff --git a/lib-src/movemail.c b/lib-src/movemail.c index d70c655adec..097bf23c202 100644 --- a/lib-src/movemail.c +++ b/lib-src/movemail.c @@ -325,11 +325,10 @@ main (int argc, char **argv) if (desc < 0) { int mkstemp_errno = errno; - char *message = (char *) xmalloc (strlen (tempname) + 50); - sprintf (message, "creating %s, which would become the lock file", - tempname); + error ("error while creating what would become the lock file", + 0, 0); errno = mkstemp_errno; - pfatal_with_name (message); + pfatal_with_name (tempname); } close (desc); From 0c6d656d269f84c83c483057eac6081eddfd33a0 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Sun, 28 Aug 2011 16:59:14 -0700 Subject: [PATCH 04/33] * update-game-score.c: Include (get_user_id): Do not assume uid fits in 'int'. Simplify. --- lib-src/ChangeLog | 3 +++ lib-src/update-game-score.c | 15 +++++---------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/lib-src/ChangeLog b/lib-src/ChangeLog index ca08ece7c39..a888a8cdd1c 100644 --- a/lib-src/ChangeLog +++ b/lib-src/ChangeLog @@ -30,6 +30,9 @@ in 'int'. Instead, put the possibly-long file name into the output of pfatal_with_name. + * update-game-score.c: Include + (get_user_id): Do not assume uid fits in 'int'. Simplify. + 2011-07-28 Paul Eggert Assume freestanding C89 headers, string.h, stdlib.h. diff --git a/lib-src/update-game-score.c b/lib-src/update-game-score.c index 2a89379aefe..9fba51a33de 100644 --- a/lib-src/update-game-score.c +++ b/lib-src/update-game-score.c @@ -35,6 +35,7 @@ along with GNU Emacs. If not, see . */ #include #include +#include #include #include #include @@ -128,19 +129,13 @@ lose_syserr (const char *msg) static char * get_user_id (void) { - char *name; struct passwd *buf = getpwuid (getuid ()); if (!buf) { - int count = 1; - int uid = (int) getuid (); - int tuid = uid; - while (tuid /= 10) - count++; - name = malloc (count+1); - if (!name) - return NULL; - sprintf (name, "%d", uid); + long uid = getuid (); + char *name = malloc (sizeof uid * CHAR_BIT / 3 + 1); + if (name) + sprintf (name, "%ld", uid); return name; } return buf->pw_name; From 005d87bd2306e943a16b86c36d1482651d9932d8 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Sun, 28 Aug 2011 17:27:35 -0700 Subject: [PATCH 05/33] Add Bug#. --- lib-src/ChangeLog | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib-src/ChangeLog b/lib-src/ChangeLog index a888a8cdd1c..e8a0b13f419 100644 --- a/lib-src/ChangeLog +++ b/lib-src/ChangeLog @@ -1,6 +1,6 @@ 2011-08-28 Paul Eggert - Integer and memory overflow issues. + Integer and memory overflow issues (Bug#9397). * emacsclient.c (xmalloc): Accept size_t, not unsigned int, to avoid potential buffer overflow issues on typical 64-bit hosts. From 62f19c197d32e8773a284616d575686d87903b7d Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 08:43:34 -0700 Subject: [PATCH 06/33] 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. */ From 2ea16b8969850fd2952ca80239cf37e77d0e7edd Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 08:49:19 -0700 Subject: [PATCH 07/33] * dbusbind.c (xd_signature, Fdbus_register_signal): Do not overrun buffer; instead, report string overflow. --- src/ChangeLog | 3 +++ src/dbusbind.c | 41 ++++++++++++++++++++++------------------- 2 files changed, 25 insertions(+), 19 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index d60c57dfd53..307174268bc 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -20,6 +20,9 @@ (esprintf, esnprintf, exprintf, evxprintf): New decls. * window.h (command_loop_level, minibuf_level): Reflect API changes. + * dbusbind.c (xd_signature, Fdbus_register_signal): + Do not overrun buffer; instead, report string overflow. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/dbusbind.c b/src/dbusbind.c index 4828f4e968d..005d521c1db 100644 --- a/src/dbusbind.c +++ b/src/dbusbind.c @@ -271,6 +271,7 @@ xd_signature (char *signature, unsigned int dtype, unsigned int parent_type, Lis { unsigned int subtype; Lisp_Object elt; + char const *subsig; char x[DBUS_MAXIMUM_SIGNATURE_LENGTH]; elt = object; @@ -328,12 +329,13 @@ xd_signature (char *signature, unsigned int dtype, unsigned int parent_type, Lis if (NILP (elt)) { subtype = DBUS_TYPE_STRING; - strcpy (x, DBUS_TYPE_STRING_AS_STRING); + subsig = DBUS_TYPE_STRING_AS_STRING; } else { subtype = XD_OBJECT_TO_DBUS_TYPE (CAR_SAFE (elt)); xd_signature (x, subtype, dtype, CAR_SAFE (XD_NEXT_VALUE (elt))); + subsig = x; } /* If the element type is DBUS_TYPE_SIGNATURE, and this is the @@ -342,7 +344,7 @@ xd_signature (char *signature, unsigned int dtype, unsigned int parent_type, Lis if ((subtype == DBUS_TYPE_SIGNATURE) && STRINGP (CAR_SAFE (XD_NEXT_VALUE (elt))) && NILP (CDR_SAFE (XD_NEXT_VALUE (elt)))) - strcpy (x, SSDATA (CAR_SAFE (XD_NEXT_VALUE (elt)))); + subsig = SSDATA (CAR_SAFE (XD_NEXT_VALUE (elt))); while (!NILP (elt)) { @@ -351,7 +353,10 @@ xd_signature (char *signature, unsigned int dtype, unsigned int parent_type, Lis elt = CDR_SAFE (XD_NEXT_VALUE (elt)); } - sprintf (signature, "%c%s", dtype, x); + if (esnprintf (signature, DBUS_MAXIMUM_SIGNATURE_LENGTH, + "%c%s", dtype, subsig) + == DBUS_MAXIMUM_SIGNATURE_LENGTH - 1) + string_overflow (); break; case DBUS_TYPE_VARIANT: @@ -2026,7 +2031,7 @@ usage: (dbus-register-signal BUS SERVICE PATH INTERFACE SIGNAL HANDLER &rest ARG DBusConnection *connection; ptrdiff_t i; char rule[DBUS_MAXIMUM_MATCH_RULE_LENGTH]; - char x[DBUS_MAXIMUM_MATCH_RULE_LENGTH]; + int rulelen; DBusError derror; /* Check parameters. */ @@ -2071,34 +2076,32 @@ usage: (dbus-register-signal BUS SERVICE PATH INTERFACE SIGNAL HANDLER &rest ARG connection = xd_initialize (bus, TRUE); /* Create a rule to receive related signals. */ - sprintf (rule, - "type='signal',interface='%s',member='%s'", - SDATA (interface), - SDATA (signal)); + rulelen = esnprintf (rule, sizeof rule, + "type='signal',interface='%s',member='%s'", + SDATA (interface), + SDATA (signal)); /* Add unique name and path to the rule if they are non-nil. */ if (!NILP (uname)) - { - sprintf (x, ",sender='%s'", SDATA (uname)); - strcat (rule, x); - } + rulelen += esnprintf (rule + rulelen, sizeof rule - rulelen, + ",sender='%s'", SDATA (uname)); if (!NILP (path)) - { - sprintf (x, ",path='%s'", SDATA (path)); - strcat (rule, x); - } + rulelen += esnprintf (rule + rulelen, sizeof rule - rulelen, + ",path='%s'", SDATA (path)); /* Add arguments to the rule if they are non-nil. */ for (i = 6; i < nargs; ++i) if (!NILP (args[i])) { CHECK_STRING (args[i]); - sprintf (x, ",arg%"pD"d='%s'", i - 6, - SDATA (args[i])); - strcat (rule, x); + rulelen += esnprintf (rule + rulelen, sizeof rule - rulelen, + ",arg%"pD"d='%s'", i - 6, SDATA (args[i])); } + if (rulelen == sizeof rule - 1) + string_overflow (); + /* Add the rule to the bus. */ dbus_error_init (&derror); dbus_bus_add_match (connection, rule, &derror); From 9d1df220c5484374901f8edff05e41bb575c0c77 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 08:51:23 -0700 Subject: [PATCH 08/33] * dispnew.c (add_window_display_history): Don't overrun buffer. Truncate instead; this is OK since it's just a log. --- src/ChangeLog | 3 +++ src/dispnew.c | 19 ++++++++++--------- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 307174268bc..4336d6a6b83 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -23,6 +23,9 @@ * dbusbind.c (xd_signature, Fdbus_register_signal): Do not overrun buffer; instead, report string overflow. + * dispnew.c (add_window_display_history): Don't overrun buffer. + Truncate instead; this is OK since it's just a log. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/dispnew.c b/src/dispnew.c index e96583e0025..0cc888b4b7a 100644 --- a/src/dispnew.c +++ b/src/dispnew.c @@ -272,15 +272,16 @@ add_window_display_history (struct window *w, const char *msg, int paused_p) buf = redisplay_history[history_idx].trace; ++history_idx; - sprintf (buf, "%"pMu": window %p (`%s')%s\n", - history_tick++, - w, - ((BUFFERP (w->buffer) - && STRINGP (BVAR (XBUFFER (w->buffer), name))) - ? SSDATA (BVAR (XBUFFER (w->buffer), name)) - : "???"), - paused_p ? " ***paused***" : ""); - strcat (buf, msg); + esnprintf (buf, sizeof redisplay_history[0].trace, + "%"pMu": window %p (`%s')%s\n%s", + history_tick++, + w, + ((BUFFERP (w->buffer) + && STRINGP (BVAR (XBUFFER (w->buffer), name))) + ? SSDATA (BVAR (XBUFFER (w->buffer), name)) + : "???"), + paused_p ? " ***paused***" : "", + msg); } From 33ef5c64373c744c6ce4329fe021c3eb729aeee4 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 08:53:21 -0700 Subject: [PATCH 09/33] * editfns.c (Fcurrent_time_zone): Don't overrun buffer even if the time zone offset is outlandishly large. Don't mishandle offset == INT_MIN. --- src/ChangeLog | 4 ++++ src/editfns.c | 5 +++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 4336d6a6b83..afd78a46c6e 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -26,6 +26,10 @@ * dispnew.c (add_window_display_history): Don't overrun buffer. Truncate instead; this is OK since it's just a log. + * editfns.c (Fcurrent_time_zone): Don't overrun buffer + even if the time zone offset is outlandishly large. + Don't mishandle offset == INT_MIN. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/editfns.c b/src/editfns.c index 6759016766f..580298c6e7d 100644 --- a/src/editfns.c +++ b/src/editfns.c @@ -2014,7 +2014,7 @@ the data it can't find. */) { int offset = tm_diff (t, &gmt); char *s = 0; - char buf[6]; + char buf[sizeof "+00" + INT_STRLEN_BOUND (int)]; #ifdef HAVE_TM_ZONE if (t->tm_zone) @@ -2029,7 +2029,8 @@ the data it can't find. */) if (!s) { /* No local time zone name is available; use "+-NNNN" instead. */ - int am = (offset < 0 ? -offset : offset) / 60; + int m = offset / 60; + int am = offset < 0 ? - m : m; sprintf (buf, "%c%02d%02d", (offset < 0 ? '-' : '+'), am/60, am%60); s = buf; } From 66c6fdd52e4e5d4b9a1133bf2c57444b8a6b0048 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 08:56:20 -0700 Subject: [PATCH 10/33] * emacs.c (main) [NS_IMPL_COCOA]: Don't overrun buffer when creating daemon; the previous buffer-overflow check was incorrect. --- src/ChangeLog | 3 +++ src/emacs.c | 8 +++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index afd78a46c6e..e918fa46a2b 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -30,6 +30,9 @@ even if the time zone offset is outlandishly large. Don't mishandle offset == INT_MIN. + * emacs.c (main) [NS_IMPL_COCOA]: Don't overrun buffer + when creating daemon; the previous buffer-overflow check was incorrect. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/emacs.c b/src/emacs.c index 7039f063dc2..2c6af6b5431 100644 --- a/src/emacs.c +++ b/src/emacs.c @@ -1068,15 +1068,17 @@ Using an Emacs configured with --with-x-toolkit=lucid does not have this problem if (!dname_arg || !strchr (dname_arg, '\n')) { /* In orig, child: now exec w/special daemon name. */ char fdStr[80]; + int fdStrlen = + snprintf (fdStr, sizeof fdStr, + "--daemon=\n%d,%d\n%s", daemon_pipe[0], + daemon_pipe[1], dname_arg ? dname_arg : ""); - if (dname_arg && strlen (dname_arg) > 70) + if (! (0 <= fdStrlen && fdStrlen < sizeof fdStr)) { fprintf (stderr, "daemon: child name too long\n"); exit (1); } - sprintf (fdStr, "--daemon=\n%d,%d\n%s", daemon_pipe[0], - daemon_pipe[1], dname_arg ? dname_arg : ""); argv[skip_args] = fdStr; execv (argv[0], argv); From d749b01b0c7daff6427373b787e56d06e6f4d223 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 09:01:33 -0700 Subject: [PATCH 11/33] * eval.c (verror): Simplify by rewriting in terms of evxprintf, which has the guts of the old verror function. --- src/ChangeLog | 3 +++ src/eval.c | 26 +------------------------- 2 files changed, 4 insertions(+), 25 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index e918fa46a2b..bf7d8b9f36f 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -33,6 +33,9 @@ * emacs.c (main) [NS_IMPL_COCOA]: Don't overrun buffer when creating daemon; the previous buffer-overflow check was incorrect. + * eval.c (verror): Simplify by rewriting in terms of evxprintf, + which has the guts of the old verror function. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/eval.c b/src/eval.c index e722b53fb72..f2407cede31 100644 --- a/src/eval.c +++ b/src/eval.c @@ -1951,35 +1951,11 @@ verror (const char *m, va_list ap) char buf[4000]; ptrdiff_t size = sizeof buf; ptrdiff_t size_max = STRING_BYTES_BOUND + 1; - char const *m_end = m + strlen (m); char *buffer = buf; ptrdiff_t used; Lisp_Object string; - while (1) - { - va_list ap_copy; - va_copy (ap_copy, ap); - used = doprnt (buffer, size, m, m_end, ap_copy); - va_end (ap_copy); - - /* Note: the -1 below is because `doprnt' returns the number of bytes - excluding the terminating null byte, and it always terminates with a - null byte, even when producing a truncated message. */ - if (used < size - 1) - break; - if (size <= size_max / 2) - size *= 2; - else if (size < size_max) - size = size_max; - else - break; /* and leave the message truncated */ - - if (buffer != buf) - xfree (buffer); - buffer = (char *) xmalloc (size); - } - + used = evxprintf (&buffer, &size, buf, size_max, m, ap); string = make_string (buffer, used); if (buffer != buf) xfree (buffer); From b5cd19054673bfa46a4f0d1ac3905deeafcf94ff Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 09:48:19 -0700 Subject: [PATCH 12/33] * filelock.c (lock_file_1, lock_file): Don't blindly alloca long name; use SAFE_ALLOCA instead. Use esprintf to avoid int-overflow issues. --- src/ChangeLog | 3 +++ src/filelock.c | 35 +++++++++++++++++++++-------------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index bf7d8b9f36f..4624e5fc30e 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -36,6 +36,9 @@ * eval.c (verror): Simplify by rewriting in terms of evxprintf, which has the guts of the old verror function. + * filelock.c (lock_file_1, lock_file): Don't blindly alloca long name; + use SAFE_ALLOCA instead. Use esprintf to avoid int-overflow issues. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/filelock.c b/src/filelock.c index c28ee7837fa..7235c862ef0 100644 --- a/src/filelock.c +++ b/src/filelock.c @@ -341,6 +341,9 @@ lock_file_1 (char *lfname, int force) const char *user_name; const char *host_name; char *lock_info_str; + ptrdiff_t lock_info_size; + int symlink_errno; + USE_SAFE_ALLOCA; /* Call this first because it can GC. */ boot = get_boot_time (); @@ -353,17 +356,14 @@ lock_file_1 (char *lfname, int force) host_name = SSDATA (Fsystem_name ()); else host_name = ""; - lock_info_str = (char *)alloca (strlen (user_name) + strlen (host_name) - + 2 * INT_STRLEN_BOUND (printmax_t) - + sizeof "@.:"); + lock_info_size = (strlen (user_name) + strlen (host_name) + + 2 * INT_STRLEN_BOUND (printmax_t) + + sizeof "@.:"); + SAFE_ALLOCA (lock_info_str, char *, lock_info_size); pid = getpid (); - if (boot) - sprintf (lock_info_str, "%s@%s.%"pMd":%"pMd, - user_name, host_name, pid, boot); - else - sprintf (lock_info_str, "%s@%s.%"pMd, - user_name, host_name, pid); + esprintf (lock_info_str, boot ? "%s@%s.%"pMd":%"pMd : "%s@%s.%"pMd, + user_name, host_name, pid, boot); err = symlink (lock_info_str, lfname); if (errno == EEXIST && force) @@ -372,6 +372,9 @@ lock_file_1 (char *lfname, int force) err = symlink (lock_info_str, lfname); } + symlink_errno = errno; + SAFE_FREE (); + errno = symlink_errno; return err == 0; } @@ -541,9 +544,11 @@ lock_file (Lisp_Object fn) { register Lisp_Object attack, orig_fn, encoded_fn; register char *lfname, *locker; + ptrdiff_t locker_size; lock_info_type lock_info; printmax_t pid; struct gcpro gcpro1; + USE_SAFE_ALLOCA; /* Don't do locking while dumping Emacs. Uncompressing wtmp files uses call-process, which does not work @@ -580,15 +585,17 @@ lock_file (Lisp_Object fn) return; /* Else consider breaking the lock */ - locker = (char *) alloca (strlen (lock_info.user) + strlen (lock_info.host) - + INT_STRLEN_BOUND (printmax_t) - + sizeof "@ (pid )"); + locker_size = (strlen (lock_info.user) + strlen (lock_info.host) + + INT_STRLEN_BOUND (printmax_t) + + sizeof "@ (pid )"); + SAFE_ALLOCA (locker, char *, locker_size); pid = lock_info.pid; - sprintf (locker, "%s@%s (pid %"pMd")", - lock_info.user, lock_info.host, pid); + esprintf (locker, "%s@%s (pid %"pMd")", + lock_info.user, lock_info.host, pid); FREE_LOCK_INFO (lock_info); attack = call2 (intern ("ask-user-about-lock"), fn, build_string (locker)); + SAFE_FREE (); if (!NILP (attack)) /* User says take the lock */ { From c21721cc3953732047ffdfe268764898f089f74b Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 11:46:42 -0700 Subject: [PATCH 13/33] * font.c (font_unparse_xlfd): Don't blindly alloca long strings. Don't assume XINT result fits in int, or that XFLOAT_DATA * 10 fits in int, when using sprintf. Use single snprintf to count length of string rather than counting it via multiple sprintfs; that's simpler and more reliable. (APPEND_SPRINTF): New macro. (font_unparse_fcname): Use it to avoid sprintf buffer overrun. (generate_otf_features) [0 && HAVE_LIBOTF]: Use esprintf, not sprintf, in case result does not fit in int. --- src/ChangeLog | 10 +++ src/font.c | 173 +++++++++++++++++++------------------------------- 2 files changed, 77 insertions(+), 106 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 4624e5fc30e..f94f9c4632f 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -39,6 +39,16 @@ * filelock.c (lock_file_1, lock_file): Don't blindly alloca long name; use SAFE_ALLOCA instead. Use esprintf to avoid int-overflow issues. + * font.c (font_unparse_xlfd): Don't blindly alloca long strings. + Don't assume XINT result fits in int, or that XFLOAT_DATA * 10 + fits in int, when using sprintf. Use single snprintf to count + length of string rather than counting it via multiple sprintfs; + that's simpler and more reliable. + (APPEND_SPRINTF): New macro. + (font_unparse_fcname): Use it to avoid sprintf buffer overrun. + (generate_otf_features) [0 && HAVE_LIBOTF]: Use esprintf, not + sprintf, in case result does not fit in int. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/font.c b/src/font.c index 5f8d22157d6..cc5939982d3 100644 --- a/src/font.c +++ b/src/font.c @@ -1180,7 +1180,7 @@ font_unparse_xlfd (Lisp_Object font, int pixel_size, char *name, int nbytes) char *p; const char *f[XLFD_REGISTRY_INDEX + 1]; Lisp_Object val; - int i, j, len = 0; + int i, j, len; font_assert (FONTP (font)); @@ -1195,9 +1195,9 @@ font_unparse_xlfd (Lisp_Object font, int pixel_size, char *name, int nbytes) if (NILP (val)) { if (j == XLFD_REGISTRY_INDEX) - f[j] = "*-*", len += 4; + f[j] = "*-*"; else - f[j] = "*", len += 2; + f[j] = "*"; } else { @@ -1207,21 +1207,15 @@ font_unparse_xlfd (Lisp_Object font, int pixel_size, char *name, int nbytes) && ! strchr (SSDATA (val), '-')) { /* Change "jisx0208*" and "jisx0208" to "jisx0208*-*". */ - if (SDATA (val)[SBYTES (val) - 1] == '*') - { - f[j] = p = alloca (SBYTES (val) + 3); - sprintf (p, "%s-*", SDATA (val)); - len += SBYTES (val) + 3; - } - else - { - f[j] = p = alloca (SBYTES (val) + 4); - sprintf (p, "%s*-*", SDATA (val)); - len += SBYTES (val) + 4; - } + ptrdiff_t alloc = SBYTES (val) + 4; + if (nbytes <= alloc) + return -1; + f[j] = p = alloca (alloc); + sprintf (p, "%s%s-*", SDATA (val), + "*" + (SDATA (val)[SBYTES (val) - 1] == '*')); } else - f[j] = SSDATA (val), len += SBYTES (val) + 1; + f[j] = SSDATA (val); } } @@ -1230,11 +1224,11 @@ font_unparse_xlfd (Lisp_Object font, int pixel_size, char *name, int nbytes) { val = font_style_symbolic (font, i, 0); if (NILP (val)) - f[j] = "*", len += 2; + f[j] = "*"; else { val = SYMBOL_NAME (val); - f[j] = SSDATA (val), len += SBYTES (val) + 1; + f[j] = SSDATA (val); } } @@ -1242,64 +1236,62 @@ font_unparse_xlfd (Lisp_Object font, int pixel_size, char *name, int nbytes) font_assert (NUMBERP (val) || NILP (val)); if (INTEGERP (val)) { - i = XINT (val); - if (i <= 0) - i = pixel_size; - if (i > 0) + EMACS_INT v = XINT (val); + if (v <= 0) + v = pixel_size; + if (v > 0) { - f[XLFD_PIXEL_INDEX] = p = alloca (22); - len += sprintf (p, "%d-*", i) + 1; + f[XLFD_PIXEL_INDEX] = p = + alloca (sizeof "-*" + INT_STRLEN_BOUND (EMACS_INT)); + sprintf (p, "%"pI"d-*", v); } else - f[XLFD_PIXEL_INDEX] = "*-*", len += 4; + f[XLFD_PIXEL_INDEX] = "*-*"; } else if (FLOATP (val)) { - i = XFLOAT_DATA (val) * 10; - f[XLFD_PIXEL_INDEX] = p = alloca (12); - len += sprintf (p, "*-%d", i) + 1; + double v = XFLOAT_DATA (val) * 10; + f[XLFD_PIXEL_INDEX] = p = alloca (sizeof "*-" + 1 + DBL_MAX_10_EXP + 1); + sprintf (p, "*-%.0f", v); } else - f[XLFD_PIXEL_INDEX] = "*-*", len += 4; + f[XLFD_PIXEL_INDEX] = "*-*"; if (INTEGERP (AREF (font, FONT_DPI_INDEX))) { - i = XINT (AREF (font, FONT_DPI_INDEX)); - f[XLFD_RESX_INDEX] = p = alloca (22); - len += sprintf (p, "%d-%d", i, i) + 1; + EMACS_INT v = XINT (AREF (font, FONT_DPI_INDEX)); + f[XLFD_RESX_INDEX] = p = + alloca (sizeof "-" + 2 * INT_STRLEN_BOUND (EMACS_INT)); + sprintf (p, "%"pI"d-%"pI"d", v, v); } else - f[XLFD_RESX_INDEX] = "*-*", len += 4; + f[XLFD_RESX_INDEX] = "*-*"; if (INTEGERP (AREF (font, FONT_SPACING_INDEX))) { - int spacing = XINT (AREF (font, FONT_SPACING_INDEX)); + EMACS_INT spacing = XINT (AREF (font, FONT_SPACING_INDEX)); f[XLFD_SPACING_INDEX] = (spacing <= FONT_SPACING_PROPORTIONAL ? "p" : spacing <= FONT_SPACING_DUAL ? "d" : spacing <= FONT_SPACING_MONO ? "m" : "c"); - len += 2; } else - f[XLFD_SPACING_INDEX] = "*", len += 2; + f[XLFD_SPACING_INDEX] = "*"; if (INTEGERP (AREF (font, FONT_AVGWIDTH_INDEX))) { - f[XLFD_AVGWIDTH_INDEX] = p = alloca (22); - len += sprintf (p, "%"pI"d", - XINT (AREF (font, FONT_AVGWIDTH_INDEX))) + 1; + f[XLFD_AVGWIDTH_INDEX] = p = alloca (INT_BUFSIZE_BOUND (EMACS_INT)); + sprintf (p, "%"pI"d", XINT (AREF (font, FONT_AVGWIDTH_INDEX))); } else - f[XLFD_AVGWIDTH_INDEX] = "*", len += 2; - len++; /* for terminating '\0'. */ - if (len >= nbytes) - return -1; - return sprintf (name, "-%s-%s-%s-%s-%s-%s-%s-%s-%s-%s-%s", + 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; } /* Parse NAME (null terminated) and store information in FONT @@ -1553,23 +1545,19 @@ int font_unparse_fcname (Lisp_Object font, int pixel_size, char *name, int nbytes) { Lisp_Object family, foundry; - Lisp_Object tail, val; + Lisp_Object val; int point_size; int i; - ptrdiff_t len = 1; char *p; + char *lim; Lisp_Object styles[3]; const char *style_names[3] = { "weight", "slant", "width" }; - char work[256]; family = AREF (font, FONT_FAMILY_INDEX); if (! NILP (family)) { if (SYMBOLP (family)) - { - family = SYMBOL_NAME (family); - len += SBYTES (family); - } + family = SYMBOL_NAME (family); else family = Qnil; } @@ -1580,7 +1568,6 @@ font_unparse_fcname (Lisp_Object font, int pixel_size, char *name, int nbytes) if (XINT (val) != 0) pixel_size = XINT (val); point_size = -1; - len += 21; /* for ":pixelsize=NUM" */ } else { @@ -1588,80 +1575,54 @@ font_unparse_fcname (Lisp_Object font, int pixel_size, char *name, int nbytes) abort (); pixel_size = -1; point_size = (int) XFLOAT_DATA (val); - len += 11; /* for "-NUM" */ } foundry = AREF (font, FONT_FOUNDRY_INDEX); if (! NILP (foundry)) { if (SYMBOLP (foundry)) - { - foundry = SYMBOL_NAME (foundry); - len += 9 + SBYTES (foundry); /* ":foundry=NAME" */ - } + foundry = SYMBOL_NAME (foundry); else foundry = Qnil; } for (i = 0; i < 3; i++) - { - styles[i] = font_style_symbolic (font, FONT_WEIGHT_INDEX + i, 0); - if (! NILP (styles[i])) - len += sprintf (work, ":%s=%s", style_names[i], - SDATA (SYMBOL_NAME (styles[i]))); - } + styles[i] = font_style_symbolic (font, FONT_WEIGHT_INDEX + i, 0); - if (INTEGERP (AREF (font, FONT_DPI_INDEX))) - len += sprintf (work, ":dpi=%"pI"d", XINT (AREF (font, FONT_DPI_INDEX))); - if (INTEGERP (AREF (font, FONT_SPACING_INDEX))) - len += strlen (":spacing=100"); - if (INTEGERP (AREF (font, FONT_AVGWIDTH_INDEX))) - len += strlen (":scalable=false"); /* or ":scalable=true" */ - for (tail = AREF (font, FONT_EXTRA_INDEX); CONSP (tail); tail = XCDR (tail)) - { - Lisp_Object key = XCAR (XCAR (tail)), value = XCDR (XCAR (tail)); - - len += SBYTES (SYMBOL_NAME (key)) + 1; /* for :KEY= */ - if (STRINGP (value)) - len += SBYTES (value); - else if (INTEGERP (value)) - len += sprintf (work, "%"pI"d", XINT (value)); - else if (SYMBOLP (value)) - len += (NILP (value) ? 5 : 4); /* for "false" or "true" */ - } - - if (len > nbytes) - return -1; 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)) - p += sprintf (p, "%s", SDATA (family)); + APPEND_SNPRINTF ((p, lim - p, "%s", SSDATA (family))); if (point_size > 0) - { - if (p == name) - p += sprintf (p, "%d", point_size); - else - p += sprintf (p, "-%d", point_size); - } + APPEND_SNPRINTF ((p, lim - p, "-%d" + (p == name), point_size)); else if (pixel_size > 0) - p += sprintf (p, ":pixelsize=%d", pixel_size); + APPEND_SNPRINTF ((p, lim - p, ":pixelsize=%d", pixel_size)); if (! NILP (AREF (font, FONT_FOUNDRY_INDEX))) - p += sprintf (p, ":foundry=%s", - SDATA (SYMBOL_NAME (AREF (font, FONT_FOUNDRY_INDEX)))); + APPEND_SNPRINTF ((p, lim - p, ":foundry=%s", + SSDATA (SYMBOL_NAME (AREF (font, + FONT_FOUNDRY_INDEX))))); for (i = 0; i < 3; i++) if (! NILP (styles[i])) - p += sprintf (p, ":%s=%s", style_names[i], - SDATA (SYMBOL_NAME (styles[i]))); + APPEND_SNPRINTF ((p, lim - p, ":%s=%s", style_names[i], + SSDATA (SYMBOL_NAME (styles[i])))); if (INTEGERP (AREF (font, FONT_DPI_INDEX))) - p += sprintf (p, ":dpi=%"pI"d", XINT (AREF (font, FONT_DPI_INDEX))); + APPEND_SNPRINTF ((p, lim - p, ":dpi=%"pI"d", + XINT (AREF (font, FONT_DPI_INDEX)))); if (INTEGERP (AREF (font, FONT_SPACING_INDEX))) - p += sprintf (p, ":spacing=%"pI"d", XINT (AREF (font, FONT_SPACING_INDEX))); + APPEND_SNPRINTF ((p, lim - p, ":spacing=%"pI"d", + XINT (AREF (font, FONT_SPACING_INDEX)))); if (INTEGERP (AREF (font, FONT_AVGWIDTH_INDEX))) - { - if (XINT (AREF (font, FONT_AVGWIDTH_INDEX)) == 0) - p += sprintf (p, ":scalable=true"); - else - p += sprintf (p, ":scalable=false"); - } + APPEND_SNPRINTF ((p, lim - p, + (XINT (AREF (font, FONT_AVGWIDTH_INDEX)) == 0 + ? ":scalable=true" + : ":scalable=false"))); return (p - name); } @@ -1952,12 +1913,12 @@ generate_otf_features (Lisp_Object spec, char *features) else if (! asterisk) { val = SYMBOL_NAME (val); - p += sprintf (p, "%s", SDATA (val)); + p += esprintf (p, "%s", SDATA (val)); } else { val = SYMBOL_NAME (val); - p += sprintf (p, "~%s", SDATA (val)); + p += esprintf (p, "~%s", SDATA (val)); } } if (CONSP (spec)) From c57b67fcf07e10378fbb11cf8c6aecded43d1736 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 11:48:24 -0700 Subject: [PATCH 14/33] * fontset.c (num_auto_fontsets): Now printmax_t, not int. (fontset_from_font): Print it. --- src/ChangeLog | 3 +++ src/fontset.c | 6 +++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index f94f9c4632f..a1af6127635 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -49,6 +49,9 @@ (generate_otf_features) [0 && HAVE_LIBOTF]: Use esprintf, not sprintf, in case result does not fit in int. + * fontset.c (num_auto_fontsets): Now printmax_t, not int. + (fontset_from_font): Print it. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/fontset.c b/src/fontset.c index c8ae1e74848..74a25a1ca04 100644 --- a/src/fontset.c +++ b/src/fontset.c @@ -1700,7 +1700,7 @@ FONT-SPEC is a vector, a cons, or a string. See the documentation of static Lisp_Object auto_fontset_alist; /* Number of automatically created fontsets. */ -static int num_auto_fontsets; +static printmax_t num_auto_fontsets; /* Retun a fontset synthesized from FONT-OBJECT. This is called from x_new_font when FONT-OBJECT is used for the default ASCII font of a @@ -1727,9 +1727,9 @@ fontset_from_font (Lisp_Object font_object) alias = intern ("fontset-startup"); else { - char temp[32]; + char temp[sizeof "fontset-auto" + INT_STRLEN_BOUND (printmax_t)]; - sprintf (temp, "fontset-auto%d", num_auto_fontsets - 1); + sprintf (temp, "fontset-auto%"pMd, num_auto_fontsets - 1); alias = intern (temp); } fontset_spec = copy_font_spec (font_spec); From 8a4014344e961833b940a36887bdeee4935f88bd Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 11:52:26 -0700 Subject: [PATCH 15/33] * frame.c (tty_frame_count): Now printmax_t, not int. (make_terminal_frame, set_term_frame_name): Print it. (x_report_frame_params): In X, window IDs are unsigned long, not signed long, so print them as unsigned. (validate_x_resource_name): Check for implausibly long names, and don't assume name length fits in 'int'. (x_get_resource_string): Don't blindly alloca invocation name; use SAFE_ALLOCA. Use esprintf, not sprintf, in case result does not fit in int. --- src/ChangeLog | 10 ++++++++++ src/frame.c | 46 ++++++++++++++++++++++++++++------------------ 2 files changed, 38 insertions(+), 18 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index a1af6127635..91bcaebb7bb 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -52,6 +52,16 @@ * fontset.c (num_auto_fontsets): Now printmax_t, not int. (fontset_from_font): Print it. + * frame.c (tty_frame_count): Now printmax_t, not int. + (make_terminal_frame, set_term_frame_name): Print it. + (x_report_frame_params): In X, window IDs are unsigned long, + not signed long, so print them as unsigned. + (validate_x_resource_name): Check for implausibly long names, + and don't assume name length fits in 'int'. + (x_get_resource_string): Don't blindly alloca invocation name; + use SAFE_ALLOCA. Use esprintf, not sprintf, in case result does + not fit in int. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/frame.c b/src/frame.c index 711109a70c6..66b857a73e9 100644 --- a/src/frame.c +++ b/src/frame.c @@ -497,7 +497,7 @@ make_minibuffer_frame (void) /* Construct a frame that refers to a terminal. */ -static int tty_frame_count; +static printmax_t tty_frame_count; struct frame * make_initial_frame (void) @@ -551,7 +551,7 @@ make_terminal_frame (struct terminal *terminal) { register struct frame *f; Lisp_Object frame; - char name[20]; + char name[sizeof "F" + INT_STRLEN_BOUND (printmax_t)]; if (!terminal->name) error ("Terminal is not live, can't create new frames on it"); @@ -562,7 +562,7 @@ make_terminal_frame (struct terminal *terminal) Vframe_list = Fcons (frame, Vframe_list); tty_frame_count++; - sprintf (name, "F%d", tty_frame_count); + sprintf (name, "F%"pMd, tty_frame_count); f->name = build_string (name); f->visible = 1; /* FRAME_SET_VISIBLE wd set frame_garbaged. */ @@ -2074,7 +2074,7 @@ set_term_frame_name (struct frame *f, Lisp_Object name) /* If NAME is nil, set the name to F. */ if (NILP (name)) { - char namebuf[20]; + char namebuf[sizeof "F" + INT_STRLEN_BOUND (printmax_t)]; /* Check for no change needed in this very common case before we do any consing. */ @@ -2083,7 +2083,7 @@ set_term_frame_name (struct frame *f, Lisp_Object name) return; tty_frame_count++; - sprintf (namebuf, "F%d", tty_frame_count); + sprintf (namebuf, "F%"pMd, tty_frame_count); name = build_string (namebuf); } else @@ -3065,6 +3065,7 @@ x_report_frame_params (struct frame *f, Lisp_Object *alistptr) { char buf[16]; Lisp_Object tem; + unsigned long w; /* Represent negative positions (off the top or left screen edge) in a way that Fmodify_frame_parameters will understand correctly. */ @@ -3097,7 +3098,8 @@ x_report_frame_params (struct frame *f, Lisp_Object *alistptr) for non-toolkit scroll bar. ruler-mode.el depends on this. */ : Qnil)); - sprintf (buf, "%ld", (long) FRAME_X_WINDOW (f)); + w = FRAME_X_WINDOW (f); + sprintf (buf, "%lu", w); store_in_alist (alistptr, Qwindow_id, build_string (buf)); #ifdef HAVE_X_WINDOWS @@ -3105,7 +3107,10 @@ x_report_frame_params (struct frame *f, Lisp_Object *alistptr) /* Tooltip frame may not have this widget. */ if (FRAME_X_OUTPUT (f)->widget) #endif - sprintf (buf, "%ld", (long) FRAME_OUTER_WINDOW (f)); + { + w = FRAME_OUTER_WINDOW (f); + sprintf (buf, "%lu", w); + } store_in_alist (alistptr, Qouter_window_id, build_string (buf)); #endif @@ -3576,13 +3581,13 @@ x_set_alpha (struct frame *f, Lisp_Object arg, Lisp_Object oldval) void validate_x_resource_name (void) { - int len = 0; + ptrdiff_t len = 0; /* Number of valid characters in the resource name. */ - int good_count = 0; + ptrdiff_t good_count = 0; /* Number of invalid characters in the resource name. */ - int bad_count = 0; + ptrdiff_t bad_count = 0; Lisp_Object new; - int i; + ptrdiff_t i; if (!STRINGP (Vx_resource_class)) Vx_resource_class = build_string (EMACS_CLASS); @@ -3615,8 +3620,9 @@ validate_x_resource_name (void) if (bad_count == 0) return; - /* If name is entirely invalid, or nearly so, use `emacs'. */ - if (good_count < 2) + /* If name is entirely invalid, or nearly so, or is so implausibly + large that alloca might not work, use `emacs'. */ + if (good_count < 2 || MAX_ALLOCA - sizeof ".customization" < len) { Vx_resource_name = build_string ("emacs"); return; @@ -3745,20 +3751,24 @@ x_get_resource_string (const char *attribute, const char *class) { char *name_key; char *class_key; + char *result; struct frame *sf = SELECTED_FRAME (); + ptrdiff_t invocation_namelen = SBYTES (Vinvocation_name); + USE_SAFE_ALLOCA; /* Allocate space for the components, the dots which separate them, and the final '\0'. */ - name_key = (char *) alloca (SBYTES (Vinvocation_name) - + strlen (attribute) + 2); + SAFE_ALLOCA (name_key, char *, invocation_namelen + strlen (attribute) + 2); class_key = (char *) alloca ((sizeof (EMACS_CLASS) - 1) + strlen (class) + 2); - sprintf (name_key, "%s.%s", SSDATA (Vinvocation_name), attribute); + esprintf (name_key, "%s.%s", SSDATA (Vinvocation_name), attribute); sprintf (class_key, "%s.%s", EMACS_CLASS, class); - return x_get_string_resource (FRAME_X_DISPLAY_INFO (sf)->xrdb, - name_key, class_key); + result = x_get_string_resource (FRAME_X_DISPLAY_INFO (sf)->xrdb, + name_key, class_key); + SAFE_FREE (); + return result; } #endif From 84722b3d573a4ad663f84ed44e212743970a0daf Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 11:54:11 -0700 Subject: [PATCH 16/33] * gtkutil.c (xg_check_special_colors, xg_set_geometry): Make sprintf buffers a bit bigger, to avoid potential buffer overrun. --- src/ChangeLog | 3 +++ src/gtkutil.c | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 91bcaebb7bb..adf9bb244b8 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -62,6 +62,9 @@ use SAFE_ALLOCA. Use esprintf, not sprintf, in case result does not fit in int. + * gtkutil.c (xg_check_special_colors, xg_set_geometry): + Make sprintf buffers a bit bigger, to avoid potential buffer overrun. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/gtkutil.c b/src/gtkutil.c index c39119c8151..5f23d597329 100644 --- a/src/gtkutil.c +++ b/src/gtkutil.c @@ -567,7 +567,7 @@ xg_check_special_colors (struct frame *f, GtkStyleContext *gsty = gtk_widget_get_style_context (FRAME_GTK_OUTER_WIDGET (f)); GdkRGBA col; - char buf[64]; + char buf[sizeof "rgbi://" + 3 * (DBL_MAX_10_EXP + sizeof "-1.000000" - 1)]; int state = GTK_STATE_FLAG_SELECTED|GTK_STATE_FLAG_FOCUSED; if (get_fg) gtk_style_context_get_color (gsty, state, &col); @@ -797,7 +797,7 @@ xg_set_geometry (FRAME_PTR f) int xneg = f->size_hint_flags & XNegative; int top = f->top_pos; int yneg = f->size_hint_flags & YNegative; - char geom_str[32]; + char geom_str[sizeof "=x--" + 4 * INT_STRLEN_BOUND (int)]; if (xneg) left = -left; From 0df02bf3e941de4c20a7174e8233357eeca738d5 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 11:55:58 -0700 Subject: [PATCH 17/33] * lread.c (dir_warning): Don't blindly alloca buffer; use SAFE_ALLOCA. Use esprintf, not sprintf, in case result does not fit in int. --- src/ChangeLog | 3 +++ src/lread.c | 16 +++++++++++----- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index adf9bb244b8..ac83d07cba5 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -65,6 +65,9 @@ * gtkutil.c (xg_check_special_colors, xg_set_geometry): Make sprintf buffers a bit bigger, to avoid potential buffer overrun. + * lread.c (dir_warning): Don't blindly alloca buffer; use SAFE_ALLOCA. + Use esprintf, not sprintf, in case result does not fit in int. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/lread.c b/src/lread.c index d24da729df6..ec65e881b0e 100644 --- a/src/lread.c +++ b/src/lread.c @@ -4295,14 +4295,20 @@ init_lread (void) void dir_warning (const char *format, Lisp_Object dirname) { - char *buffer - = (char *) alloca (SCHARS (dirname) + strlen (format) + 5); - fprintf (stderr, format, SDATA (dirname)); - sprintf (buffer, format, SDATA (dirname)); + /* Don't log the warning before we've initialized!! */ if (initialized) - message_dolog (buffer, strlen (buffer), 0, STRING_MULTIBYTE (dirname)); + { + char *buffer; + ptrdiff_t message_len; + USE_SAFE_ALLOCA; + SAFE_ALLOCA (buffer, char *, + SBYTES (dirname) + strlen (format) - (sizeof "%s" - 1) + 1); + message_len = esprintf (buffer, format, SDATA (dirname)); + message_dolog (buffer, message_len, 0, STRING_MULTIBYTE (dirname)); + SAFE_FREE (); + } } void From 48e3079369b75be22bcc429bc77bc5e61843562d Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 12:07:18 -0700 Subject: [PATCH 18/33] * macros.c (executing_kbd_macro_iterations): Now EMACS_INT, not int. (Fend_kbd_macro): Don't mishandle MOST_NEGATIVE_FIXNUM by treating it as a large positive number. (Fexecute_kbd_macro): Don't assume repeat count fits in int. * macros.h (executing_kbd_macro_iterations): Now EMACS_INT, not int. --- src/ChangeLog | 6 ++++++ src/macros.c | 12 ++++++------ src/macros.h | 3 +-- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index ac83d07cba5..b69830b23a0 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -68,6 +68,12 @@ * lread.c (dir_warning): Don't blindly alloca buffer; use SAFE_ALLOCA. Use esprintf, not sprintf, in case result does not fit in int. + * macros.c (executing_kbd_macro_iterations): Now EMACS_INT, not int. + (Fend_kbd_macro): Don't mishandle MOST_NEGATIVE_FIXNUM by treating + it as a large positive number. + (Fexecute_kbd_macro): Don't assume repeat count fits in int. + * macros.h (executing_kbd_macro_iterations): Now EMACS_INT, not int. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/macros.c b/src/macros.c index f6cd3a3ccad..4ecf49834a1 100644 --- a/src/macros.c +++ b/src/macros.c @@ -35,7 +35,7 @@ static Lisp_Object Qkbd_macro_termination_hook; This is not bound at each level, so after an error, it describes the innermost interrupted macro. */ -int executing_kbd_macro_iterations; +EMACS_INT executing_kbd_macro_iterations; /* This is the macro that was executing. This is not bound at each level, @@ -175,11 +175,11 @@ each iteration of the macro. Iteration stops if LOOPFUNC returns nil. */) if (XFASTINT (repeat) == 0) Fexecute_kbd_macro (KVAR (current_kboard, Vlast_kbd_macro), repeat, loopfunc); - else + else if (XINT (repeat) > 1) { XSETINT (repeat, XINT (repeat)-1); - if (XINT (repeat) > 0) - Fexecute_kbd_macro (KVAR (current_kboard, Vlast_kbd_macro), repeat, loopfunc); + Fexecute_kbd_macro (KVAR (current_kboard, Vlast_kbd_macro), + repeat, loopfunc); } return Qnil; } @@ -302,9 +302,9 @@ each iteration of the macro. Iteration stops if LOOPFUNC returns nil. */) Lisp_Object final; Lisp_Object tem; int pdlcount = SPECPDL_INDEX (); - int repeat = 1; + EMACS_INT repeat = 1; struct gcpro gcpro1, gcpro2; - int success_count = 0; + EMACS_INT success_count = 0; executing_kbd_macro_iterations = 0; diff --git a/src/macros.h b/src/macros.h index 32a97e457e8..7a5d532fbb7 100644 --- a/src/macros.h +++ b/src/macros.h @@ -22,7 +22,7 @@ along with GNU Emacs. If not, see . */ This is not bound at each level, so after an error, it describes the innermost interrupted macro. */ -extern int executing_kbd_macro_iterations; +extern EMACS_INT executing_kbd_macro_iterations; /* This is the macro that was executing. This is not bound at each level, @@ -42,4 +42,3 @@ extern void finalize_kbd_macro_chars (void); /* Store a character into kbd macro being defined */ extern void store_kbd_macro_char (Lisp_Object); - From a66ff6d8b7f1ba4a8ef4d52f7d66b7804ba97091 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 12:09:16 -0700 Subject: [PATCH 19/33] * nsterm.m ((NSSize)windowWillResize): Use esprintf, not sprintf, in case result does not fit in int. --- src/ChangeLog | 3 +++ src/nsterm.m | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/ChangeLog b/src/ChangeLog index b69830b23a0..362109acbe6 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -74,6 +74,9 @@ (Fexecute_kbd_macro): Don't assume repeat count fits in int. * macros.h (executing_kbd_macro_iterations): Now EMACS_INT, not int. + * nsterm.m ((NSSize)windowWillResize): Use esprintf, not sprintf, + in case result does not fit in int. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/nsterm.m b/src/nsterm.m index 4c9574c35ba..827404a2974 100644 --- a/src/nsterm.m +++ b/src/nsterm.m @@ -5316,7 +5316,7 @@ - (NSSize)windowWillResize: (NSWindow *)sender toSize: (NSSize)frameSize strcpy (old_title, t); } size_title = xmalloc (strlen (old_title) + 40); - sprintf (size_title, "%s — (%d x %d)", old_title, cols, rows); + esprintf (size_title, "%s — (%d x %d)", old_title, cols, rows); [window setTitle: [NSString stringWithUTF8String: size_title]]; [window display]; xfree (size_title); From aca216ff01bab0741e718036a3c6d887994904c6 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 12:14:47 -0700 Subject: [PATCH 20/33] * print.c (float_to_string): Detect width overflow more reliably. (print_object): Make sprintf buffer a bit bigger, to avoid potential buffer overrun. Don't assume list length fits in 'int'. Treat print length of 0 as 0, not as infinity; to be consistent with other uses of print length in this function. Don't overflow print length index. Don't assume hash table size fits in 'long', or that vectorlike size fits in 'unsigned long'. --- src/ChangeLog | 8 ++++++++ src/print.c | 35 +++++++++++++++++++---------------- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 362109acbe6..d1d11df1900 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -77,6 +77,14 @@ * nsterm.m ((NSSize)windowWillResize): Use esprintf, not sprintf, in case result does not fit in int. + * print.c (float_to_string): Detect width overflow more reliably. + (print_object): Make sprintf buffer a bit bigger, to avoid potential + buffer overrun. Don't assume list length fits in 'int'. Treat + print length of 0 as 0, not as infinity; to be consistent with other + uses of print length in this function. Don't overflow print length + index. Don't assume hash table size fits in 'long', or that + vectorlike size fits in 'unsigned long'. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/print.c b/src/print.c index 35f89860843..f47dc985e96 100644 --- a/src/print.c +++ b/src/print.c @@ -1016,12 +1016,15 @@ float_to_string (char *buf, double data) { width = 0; do - width = (width * 10) + (*cp++ - '0'); + { + width = (width * 10) + (*cp++ - '0'); + if (DBL_DIG < width) + goto lose; + } while (*cp >= '0' && *cp <= '9'); /* A precision of zero is valid only for %f. */ - if (width > DBL_DIG - || (width == 0 && *cp != 'f')) + if (width == 0 && *cp != 'f') goto lose; } @@ -1314,7 +1317,9 @@ print_prune_string_charset (Lisp_Object string) static void print_object (Lisp_Object obj, register Lisp_Object printcharfun, int escapeflag) { - char buf[40]; + char buf[max (sizeof "from..to..in " + 2 * INT_STRLEN_BOUND (EMACS_INT), + max (sizeof " . #" + INT_STRLEN_BOUND (printmax_t), + 40))]; QUIT; @@ -1614,8 +1619,7 @@ print_object (Lisp_Object obj, register Lisp_Object printcharfun, int escapeflag PRINTCHAR ('('); { - EMACS_INT print_length; - int i; + printmax_t i, print_length; Lisp_Object halftail = obj; /* Negative values of print-length are invalid in CL. @@ -1623,7 +1627,7 @@ print_object (Lisp_Object obj, register Lisp_Object printcharfun, int escapeflag if (NATNUMP (Vprint_length)) print_length = XFASTINT (Vprint_length); else - print_length = 0; + print_length = TYPE_MAXIMUM (printmax_t); i = 0; while (CONSP (obj)) @@ -1634,7 +1638,7 @@ print_object (Lisp_Object obj, register Lisp_Object printcharfun, int escapeflag /* Simple but imcomplete way. */ if (i != 0 && EQ (obj, halftail)) { - sprintf (buf, " . #%d", i / 2); + sprintf (buf, " . #%"pMd, i / 2); strout (buf, -1, -1, printcharfun); goto end_of_list; } @@ -1654,15 +1658,16 @@ print_object (Lisp_Object obj, register Lisp_Object printcharfun, int escapeflag } } - if (i++) + if (i) PRINTCHAR (' '); - if (print_length && i > print_length) + if (print_length <= i) { strout ("...", 3, 3, printcharfun); goto end_of_list; } + i++; print_object (XCAR (obj), printcharfun, escapeflag); obj = XCDR (obj); @@ -1798,19 +1803,17 @@ print_object (Lisp_Object obj, register Lisp_Object printcharfun, int escapeflag PRINTCHAR (' '); strout (SDATA (SYMBOL_NAME (h->weak)), -1, -1, printcharfun); PRINTCHAR (' '); - sprintf (buf, "%ld/%ld", (long) h->count, - (long) ASIZE (h->next)); + sprintf (buf, "%"pI"d/%"pI"d", h->count, ASIZE (h->next)); strout (buf, -1, -1, printcharfun); } - sprintf (buf, " 0x%lx", (unsigned long) h); + sprintf (buf, " %p", h); strout (buf, -1, -1, printcharfun); PRINTCHAR ('>'); #endif /* Implement a readable output, e.g.: #s(hash-table size 2 test equal data (k1 v1 k2 v2)) */ /* Always print the size. */ - sprintf (buf, "#s(hash-table size %ld", - (long) ASIZE (h->next)); + sprintf (buf, "#s(hash-table size %"pI"d", ASIZE (h->next)); strout (buf, -1, -1, printcharfun); if (!NILP (h->test)) @@ -2038,7 +2041,7 @@ print_object (Lisp_Object obj, register Lisp_Object printcharfun, int escapeflag if (MISCP (obj)) sprintf (buf, "(MISC 0x%04x)", (int) XMISCTYPE (obj)); else if (VECTORLIKEP (obj)) - sprintf (buf, "(PVEC 0x%08lx)", (unsigned long) ASIZE (obj)); + sprintf (buf, "(PVEC 0x%08"pI"x)", ASIZE (obj)); else sprintf (buf, "(0x%02x)", (int) XTYPE (obj)); strout (buf, -1, -1, printcharfun); From 31c286f79d43a002f441b90dc0176014ba0fa8e7 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 12:46:15 -0700 Subject: [PATCH 21/33] * process.c (make_process): Use printmax_t, not int, to format process-name gensyms. --- src/ChangeLog | 3 +++ src/process.c | 6 +++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index d1d11df1900..b1c29363da0 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -85,6 +85,9 @@ index. Don't assume hash table size fits in 'long', or that vectorlike size fits in 'unsigned long'. + * process.c (make_process): Use printmax_t, not int, to format + process-name gensyms. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/process.c b/src/process.c index a8088322147..058ad5f871f 100644 --- a/src/process.c +++ b/src/process.c @@ -616,8 +616,8 @@ make_process (Lisp_Object name) { register Lisp_Object val, tem, name1; register struct Lisp_Process *p; - char suffix[10]; - register int i; + char suffix[sizeof "<>" + INT_STRLEN_BOUND (printmax_t)]; + printmax_t i; p = allocate_process (); @@ -651,7 +651,7 @@ make_process (Lisp_Object name) { tem = Fget_process (name1); if (NILP (tem)) break; - sprintf (suffix, "<%d>", i); + sprintf (suffix, "<%"pMd">", i); name1 = concat2 (name, build_string (suffix)); } name = name1; From 80f2e268a31679c363e0fa2c660d8dca53871aed Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 12:47:44 -0700 Subject: [PATCH 22/33] * term.c (produce_glyphless_glyph): Make sprintf buffer a bit bigger to avoid potential buffer overrun. --- src/ChangeLog | 3 +++ src/term.c | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/ChangeLog b/src/ChangeLog index b1c29363da0..97d1ea08db1 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -88,6 +88,9 @@ * process.c (make_process): Use printmax_t, not int, to format process-name gensyms. + * term.c (produce_glyphless_glyph): Make sprintf buffer a bit bigger + to avoid potential buffer overrun. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/term.c b/src/term.c index f3bf3a947cb..48d4069e20e 100644 --- a/src/term.c +++ b/src/term.c @@ -1887,7 +1887,7 @@ produce_glyphless_glyph (struct it *it, int for_no_font, Lisp_Object acronym) { int face_id; int len; - char buf[9]; + char buf[sizeof "\\x" + max (6, (sizeof it->c * CHAR_BIT + 3) / 4)]; char const *str = " "; /* Get a face ID for the glyph by utilizing a cache (the same way as From 670741ab04da51fa86058b6a88f0923adfcea1b2 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 12:58:56 -0700 Subject: [PATCH 23/33] * xfaces.c (x_update_menu_appearance): Don't overrun buffer if X resource line is longer than 512 bytes. --- src/ChangeLog | 3 +++ src/xfaces.c | 35 +++++++++++++++++++++-------------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 97d1ea08db1..aeb984ee8df 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -91,6 +91,9 @@ * term.c (produce_glyphless_glyph): Make sprintf buffer a bit bigger to avoid potential buffer overrun. + * xfaces.c (x_update_menu_appearance): Don't overrun buffer + if X resource line is longer than 512 bytes. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/xfaces.c b/src/xfaces.c index 431ca07b8df..47d55f4da4b 100644 --- a/src/xfaces.c +++ b/src/xfaces.c @@ -3549,6 +3549,8 @@ x_update_menu_appearance (struct frame *f) rdb != NULL)) { char line[512]; + char *buf = line; + ptrdiff_t bufsize = sizeof line; Lisp_Object lface = lface_from_face_name (f, Qmenu, 1); struct face *face = FACE_FROM_ID (f, MENU_FACE_ID); const char *myname = SSDATA (Vx_resource_name); @@ -3561,24 +3563,25 @@ x_update_menu_appearance (struct frame *f) if (STRINGP (LFACE_FOREGROUND (lface))) { - sprintf (line, "%s.%s*foreground: %s", - myname, popup_path, - SDATA (LFACE_FOREGROUND (lface))); + exprintf (&buf, &bufsize, line, -1, "%s.%s*foreground: %s", + myname, popup_path, + SDATA (LFACE_FOREGROUND (lface))); XrmPutLineResource (&rdb, line); - sprintf (line, "%s.pane.menubar*foreground: %s", - myname, SDATA (LFACE_FOREGROUND (lface))); + exprintf (&buf, &bufsize, line, -1, "%s.pane.menubar*foreground: %s", + myname, SDATA (LFACE_FOREGROUND (lface))); XrmPutLineResource (&rdb, line); changed_p = 1; } if (STRINGP (LFACE_BACKGROUND (lface))) { - sprintf (line, "%s.%s*background: %s", - myname, popup_path, - SDATA (LFACE_BACKGROUND (lface))); + exprintf (&buf, &bufsize, line, -1, "%s.%s*background: %s", + myname, popup_path, + SDATA (LFACE_BACKGROUND (lface))); XrmPutLineResource (&rdb, line); - sprintf (line, "%s.pane.menubar*background: %s", - myname, SDATA (LFACE_BACKGROUND (lface))); + + exprintf (&buf, &bufsize, line, -1, "%s.pane.menubar*background: %s", + myname, SDATA (LFACE_BACKGROUND (lface))); XrmPutLineResource (&rdb, line); changed_p = 1; } @@ -3616,11 +3619,12 @@ x_update_menu_appearance (struct frame *f) #else char *fontsetname = SSDATA (xlfd); #endif - sprintf (line, "%s.pane.menubar*font%s: %s", - myname, suffix, fontsetname); + exprintf (&buf, &bufsize, line, -1, "%s.pane.menubar*font%s: %s", + myname, suffix, fontsetname); XrmPutLineResource (&rdb, line); - sprintf (line, "%s.%s*font%s: %s", - myname, popup_path, suffix, fontsetname); + + exprintf (&buf, &bufsize, line, -1, "%s.%s*font%s: %s", + myname, popup_path, suffix, fontsetname); XrmPutLineResource (&rdb, line); changed_p = 1; if (fontsetname != SSDATA (xlfd)) @@ -3630,6 +3634,9 @@ x_update_menu_appearance (struct frame *f) if (changed_p && f->output_data.x->menubar_widget) free_frame_menubar (f); + + if (buf != line) + xfree (buf); } } From b7163a504a0b26e0068b6bc37a2b192e5e0cdac8 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 12:59:51 -0700 Subject: [PATCH 24/33] * xfns.c (x_window): Make sprintf buffer a bit bigger to avoid potential buffer overrun. --- src/ChangeLog | 3 +++ src/xfns.c | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/ChangeLog b/src/ChangeLog index aeb984ee8df..53344aa7dd9 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -94,6 +94,9 @@ * xfaces.c (x_update_menu_appearance): Don't overrun buffer if X resource line is longer than 512 bytes. + * xfns.c (x_window): Make sprintf buffer a bit bigger + to avoid potential buffer overrun. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/xfns.c b/src/xfns.c index 9a3d5fcda83..194a8f063b7 100644 --- a/src/xfns.c +++ b/src/xfns.c @@ -2440,7 +2440,7 @@ x_window (struct frame *f, long window_prompting, int minibuffer_only) /* Do some needed geometry management. */ { ptrdiff_t len; - char *tem, shell_position[32]; + char *tem, shell_position[sizeof "=x++" + 4 * INT_STRLEN_BOUND (int)]; Arg gal[10]; int gac = 0; int extra_borders = 0; From ae58ff1fbc37d8ace14dae8cc15859add5edac7f Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 13:03:30 -0700 Subject: [PATCH 25/33] * xterm.c (x_io_error_quitter): Don't overrun sprintf buffer. --- src/ChangeLog | 2 ++ src/xterm.c | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/ChangeLog b/src/ChangeLog index 53344aa7dd9..38779ef8598 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -97,6 +97,8 @@ * xfns.c (x_window): Make sprintf buffer a bit bigger to avoid potential buffer overrun. + * xterm.c (x_io_error_quitter): Don't overrun sprintf buffer. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/xterm.c b/src/xterm.c index c07caec6c78..86393cf411f 100644 --- a/src/xterm.c +++ b/src/xterm.c @@ -7900,7 +7900,8 @@ x_io_error_quitter (Display *display) { char buf[256]; - sprintf (buf, "Connection lost to X server `%s'", DisplayString (display)); + snprintf (buf, sizeof buf, "Connection lost to X server `%s'", + DisplayString (display)); x_connection_closed (display, buf); return 0; } From c43c8a6af9f6877b97c1f9a88e361456775cb88b Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 13:04:46 -0700 Subject: [PATCH 26/33] * xterm.h (x_check_errors): Add ATTRIBUTE_FORMAT_PRINTF. --- src/ChangeLog | 2 ++ src/xterm.h | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/ChangeLog b/src/ChangeLog index 38779ef8598..d38d2df6b8c 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -99,6 +99,8 @@ * xterm.c (x_io_error_quitter): Don't overrun sprintf buffer. + * xterm.h (x_check_errors): Add ATTRIBUTE_FORMAT_PRINTF. + 2011-08-26 Paul Eggert Integer and memory overflow issues (Bug#9196). diff --git a/src/xterm.h b/src/xterm.h index 30867656710..fe86a32d09f 100644 --- a/src/xterm.h +++ b/src/xterm.h @@ -955,7 +955,8 @@ XrmDatabase x_load_resources (Display *, const char *, const char *, extern int x_text_icon (struct frame *, const char *); extern int x_bitmap_icon (struct frame *, Lisp_Object); extern void x_catch_errors (Display *); -extern void x_check_errors (Display *, const char *); +extern void x_check_errors (Display *, const char *) + ATTRIBUTE_FORMAT_PRINTF (2, 0); extern int x_had_errors_p (Display *); extern int x_catching_errors (void); extern void x_uncatch_errors (void); From 6e1a67fbe915e6fdc1d63a8e6c434aa79e4e7fb4 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 29 Aug 2011 13:57:42 -0700 Subject: [PATCH 27/33] * font.c, gtkutil.c: Include . --- src/ChangeLog | 6 ++++-- src/font.c | 1 + src/gtkutil.c | 1 + 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 9f86c42d43b..1de15f4796e 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -39,7 +39,8 @@ * filelock.c (lock_file_1, lock_file): Don't blindly alloca long name; use SAFE_ALLOCA instead. Use esprintf to avoid int-overflow issues. - * font.c (font_unparse_xlfd): Don't blindly alloca long strings. + * font.c: Include , for DBL_MAX_10_EXP. + (font_unparse_xlfd): Don't blindly alloca long strings. Don't assume XINT result fits in int, or that XFLOAT_DATA * 10 fits in int, when using sprintf. Use single snprintf to count length of string rather than counting it via multiple sprintfs; @@ -62,7 +63,8 @@ use SAFE_ALLOCA. Use esprintf, not sprintf, in case result does not fit in int. - * gtkutil.c (xg_check_special_colors, xg_set_geometry): + * gtkutil.c: Include , for DBL_MAX_10_EXP. + (xg_check_special_colors, xg_set_geometry): Make sprintf buffers a bit bigger, to avoid potential buffer overrun. * lread.c (dir_warning): Don't blindly alloca buffer; use SAFE_ALLOCA. diff --git a/src/font.c b/src/font.c index cc5939982d3..1609a2cc9ff 100644 --- a/src/font.c +++ b/src/font.c @@ -21,6 +21,7 @@ You should have received a copy of the GNU General Public License along with GNU Emacs. If not, see . */ #include +#include #include #include #include diff --git a/src/gtkutil.c b/src/gtkutil.c index 5f23d597329..c154797735e 100644 --- a/src/gtkutil.c +++ b/src/gtkutil.c @@ -20,6 +20,7 @@ along with GNU Emacs. If not, see . */ #include #ifdef USE_GTK +#include #include #include #include From 2be7d7020619ebbdfb3df2bc2c3fcc3123bcedc0 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Tue, 30 Aug 2011 09:27:26 -0700 Subject: [PATCH 28/33] * dbusbind.c (signature_cat): New function. --- src/ChangeLog | 5 +++-- src/dbusbind.c | 22 +++++++++++++++++----- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 1de15f4796e..e6c58903f03 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,4 +1,4 @@ -2011-08-29 Paul Eggert +2011-08-30 Paul Eggert sprintf-related integer and memory overflow issues. @@ -20,7 +20,8 @@ (esprintf, esnprintf, exprintf, evxprintf): New decls. * window.h (command_loop_level, minibuf_level): Reflect API changes. - * dbusbind.c (xd_signature, Fdbus_register_signal): + * dbusbind.c (signature_cat): New function. + (xd_signature, Fdbus_register_signal): Do not overrun buffer; instead, report string overflow. * dispnew.c (add_window_display_history): Don't overrun buffer. diff --git a/src/dbusbind.c b/src/dbusbind.c index 005d521c1db..f7422ca2ddc 100644 --- a/src/dbusbind.c +++ b/src/dbusbind.c @@ -259,6 +259,18 @@ xd_symbol_to_dbus_type (Lisp_Object object) } \ while (0) +/* Append to SIGNATURE the a copy of X, making sure SIGNATURE does + not become too long. */ +static void +signature_cat (char *signature, char const *x) +{ + ptrdiff_t siglen = strlen (signature); + ptrdiff_t xlen = strlen (x); + if (DBUS_MAXIMUM_SIGNATURE_LENGTH - xlen <= siglen) + string_overflow (); + strcat (signature, x); +} + /* Compute SIGNATURE of OBJECT. It must have a form that it can be used in dbus_message_iter_open_container. DTYPE is the DBusType the object is related to. It is passed as argument, because it @@ -388,10 +400,10 @@ xd_signature (char *signature, unsigned int dtype, unsigned int parent_type, Lis { subtype = XD_OBJECT_TO_DBUS_TYPE (CAR_SAFE (elt)); xd_signature (x, subtype, dtype, CAR_SAFE (XD_NEXT_VALUE (elt))); - strcat (signature, x); + signature_cat (signature, x); elt = CDR_SAFE (XD_NEXT_VALUE (elt)); } - strcat (signature, DBUS_STRUCT_END_CHAR_AS_STRING); + signature_cat (signature, DBUS_STRUCT_END_CHAR_AS_STRING); break; case DBUS_TYPE_DICT_ENTRY: @@ -412,7 +424,7 @@ xd_signature (char *signature, unsigned int dtype, unsigned int parent_type, Lis elt = XD_NEXT_VALUE (elt); subtype = XD_OBJECT_TO_DBUS_TYPE (CAR_SAFE (elt)); xd_signature (x, subtype, dtype, CAR_SAFE (XD_NEXT_VALUE (elt))); - strcat (signature, x); + signature_cat (signature, x); if (!XD_BASIC_DBUS_TYPE (subtype)) wrong_type_argument (intern ("D-Bus"), CAR_SAFE (XD_NEXT_VALUE (elt))); @@ -421,14 +433,14 @@ xd_signature (char *signature, unsigned int dtype, unsigned int parent_type, Lis elt = CDR_SAFE (XD_NEXT_VALUE (elt)); subtype = XD_OBJECT_TO_DBUS_TYPE (CAR_SAFE (elt)); xd_signature (x, subtype, dtype, CAR_SAFE (XD_NEXT_VALUE (elt))); - strcat (signature, x); + signature_cat (signature, x); if (!NILP (CDR_SAFE (XD_NEXT_VALUE (elt)))) wrong_type_argument (intern ("D-Bus"), CAR_SAFE (CDR_SAFE (XD_NEXT_VALUE (elt)))); /* Closing signature. */ - strcat (signature, DBUS_DICT_ENTRY_END_CHAR_AS_STRING); + signature_cat (signature, DBUS_DICT_ENTRY_END_CHAR_AS_STRING); break; default: From e936e76efd66ce5f078d80daea414a36ee8696e9 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Tue, 30 Aug 2011 15:02:56 -0700 Subject: [PATCH 29/33] Fix misworded comment. --- src/dbusbind.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dbusbind.c b/src/dbusbind.c index f7422ca2ddc..fd9a43aaf86 100644 --- a/src/dbusbind.c +++ b/src/dbusbind.c @@ -259,7 +259,7 @@ xd_symbol_to_dbus_type (Lisp_Object object) } \ while (0) -/* Append to SIGNATURE the a copy of X, making sure SIGNATURE does +/* Append to SIGNATURE a copy of X, making sure SIGNATURE does not become too long. */ static void signature_cat (char *signature, char const *x) From 0999621ac510fb0e8e949966ec6ab737b7443ab0 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Tue, 30 Aug 2011 15:49:45 -0700 Subject: [PATCH 30/33] Add Bug#. --- src/ChangeLog | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ChangeLog b/src/ChangeLog index b38c11ace93..67ec3140cd4 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,6 +1,6 @@ 2011-08-30 Paul Eggert - sprintf-related integer and memory overflow issues. + sprintf-related integer and memory overflow issues (Bug#9412). * doprnt.c (doprnt): Support printing ptrdiff_t and intmax_t values. (esprintf, esnprintf, exprintf, evxprintf): New functions. From 61bfeeb79dee6b321cb9f2257aeec9d0c1fa5a2f Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Tue, 30 Aug 2011 22:50:49 -0700 Subject: [PATCH 31/33] 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. --- src/ChangeLog | 10 +++++++++ src/font.c | 57 ++++++++++++++++++++++----------------------------- src/xterm.c | 8 ++++---- 3 files changed, 39 insertions(+), 36 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 67ec3140cd4..d0f57593220 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,13 @@ +2011-08-31 Paul Eggert + + 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. + 2011-08-30 Paul Eggert sprintf-related integer and memory overflow issues (Bug#9412). diff --git a/src/font.c b/src/font.c index 1609a2cc9ff..a5b873aea51 100644 --- a/src/font.c +++ b/src/font.c @@ -1285,14 +1285,14 @@ font_unparse_xlfd (Lisp_Object font, int pixel_size, char *name, int nbytes) } 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 @@ font_unparse_fcname (Lisp_Object font, int pixel_size, char *name, int nbytes) 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 diff --git a/src/xterm.c b/src/xterm.c index 86393cf411f..72e9f2b2236 100644 --- a/src/xterm.c +++ b/src/xterm.c @@ -7900,8 +7900,8 @@ x_io_error_quitter (Display *display) { 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 @@ x_term_init (Lisp_Object display_name, char *xrm_option, char *resource_name) 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, From 55e5faa18952a5608cf653f8fd268a7645a2f876 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Wed, 31 Aug 2011 15:18:16 -0700 Subject: [PATCH 32/33] Add a stub for snprintf, for ancient hosts lacking it. * configure.in (snprintf): New check. * nt/config.nt (HAVE_SNPRINTF): New macro. * src/sysdep.c (snprintf) [! HAVE_SNPRINTF]: New function. --- ChangeLog | 4 ++++ configure.in | 2 ++ nt/ChangeLog | 4 ++++ nt/config.nt | 1 + src/ChangeLog | 2 ++ src/sysdep.c | 39 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 52 insertions(+) diff --git a/ChangeLog b/ChangeLog index 1f38dbf71ca..c973a82e8a4 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,7 @@ +2011-08-31 Paul Eggert + + * configure.in (snprintf): New check. + 2011-08-30 Paul Eggert * configure.in (opsys): Change pattern to *-*-linux* diff --git a/configure.in b/configure.in index 715e8278df2..dcc2bdb99d1 100644 --- a/configure.in +++ b/configure.in @@ -3071,6 +3071,8 @@ fi AC_FUNC_FORK +AC_CHECK_FUNCS(snprintf) + dnl Adapted from Haible's version. AC_CACHE_CHECK([for nl_langinfo and CODESET], emacs_cv_langinfo_codeset, [AC_LINK_IFELSE([AC_LANG_PROGRAM([[#include ]], diff --git a/nt/ChangeLog b/nt/ChangeLog index edbd1a1c1d4..f3c57c7e0d0 100644 --- a/nt/ChangeLog +++ b/nt/ChangeLog @@ -1,3 +1,7 @@ +2011-08-31 Paul Eggert + + * config.nt (HAVE_SNPRINTF): New macro. + 2011-07-28 Paul Eggert Assume freestanding C89 headers, string.h, stdlib.h. diff --git a/nt/config.nt b/nt/config.nt index 3436bc989e0..ae3807538c0 100644 --- a/nt/config.nt +++ b/nt/config.nt @@ -240,6 +240,7 @@ along with GNU Emacs. If not, see . */ #define HAVE_SETSOCKOPT 1 #define HAVE_GETSOCKNAME 1 #define HAVE_GETPEERNAME 1 +#define HAVE_SNPRINTF 1 #define HAVE_LANGINFO_CODESET 1 /* Local (unix) sockets are not supported. */ #undef HAVE_SYS_UN_H diff --git a/src/ChangeLog b/src/ChangeLog index 463f2965baa..0ba2df42186 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -90,6 +90,8 @@ * process.c (make_process): Use printmax_t, not int, to format process-name gensyms. + * sysdep.c (snprintf) [! HAVE_SNPRINTF]: New function. + * term.c (produce_glyphless_glyph): Make sprintf buffer a bit bigger to avoid potential buffer overrun. diff --git a/src/sysdep.c b/src/sysdep.c index 57fff94f552..e20bd591da1 100644 --- a/src/sysdep.c +++ b/src/sysdep.c @@ -1811,6 +1811,45 @@ strerror (int errnum) } #endif /* not WINDOWSNT */ #endif /* ! HAVE_STRERROR */ + +#ifndef HAVE_SNPRINTF +/* Approximate snprintf as best we can on ancient hosts that lack it. */ +int +snprintf (char *buf, size_t bufsize, char const *format, ...) +{ + ptrdiff_t size = min (bufsize, PTRDIFF_MAX); + ptrdiff_t nbytes = size - 1; + va_list ap; + + if (size) + { + va_start (ap, format); + nbytes = doprnt (buf, size, format, 0, ap); + va_end (ap); + } + + if (nbytes == size - 1) + { + /* Calculate the length of the string that would have been created + had the buffer been large enough. */ + char stackbuf[4000]; + char *b = stackbuf; + ptrdiff_t bsize = sizeof stackbuf; + va_start (ap, format); + nbytes = evxprintf (&b, &bsize, stackbuf, -1, format, ap); + va_end (ap); + if (b != stackbuf) + xfree (b); + } + + if (INT_MAX < nbytes) + { + errno = EOVERFLOW; + return -1; + } + return nbytes; +} +#endif int emacs_open (const char *path, int oflag, int mode) From 8666506ecd6b1a90c7c66fb4b6051e90fba20c30 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Thu, 1 Sep 2011 07:44:49 -0700 Subject: [PATCH 33/33] * src/doprnt.c (esnprintf): Remove. All uses removed. Suggested by Chong Yidong in . --- src/ChangeLog | 11 +++--- src/dbusbind.c | 42 ++++++++++++++-------- src/dispnew.c | 20 +++++------ src/doprnt.c | 21 ----------- src/font.c | 94 ++++++++++++++++++++++++++++++++++++-------------- src/lisp.h | 2 -- src/xterm.c | 8 ++--- 7 files changed, 115 insertions(+), 83 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 0ba2df42186..94ba3a040ca 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,9 +1,9 @@ -2011-08-31 Paul Eggert +2011-09-01 Paul Eggert sprintf-related integer and memory overflow issues (Bug#9412). * doprnt.c (doprnt): Support printing ptrdiff_t and intmax_t values. - (esprintf, esnprintf, exprintf, evxprintf): New functions. + (esprintf, 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 @@ -17,7 +17,7 @@ * 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. + (esprintf, exprintf, evxprintf): New decls. * window.h (command_loop_level, minibuf_level): Reflect API changes. * dbusbind.c (signature_cat): New function. @@ -43,7 +43,7 @@ * font.c: Include , for DBL_MAX_10_EXP. (font_unparse_xlfd): Don't blindly alloca long strings. Don't assume XINT result fits in int, or that XFLOAT_DATA * 10 - fits in int, when using sprintf. Use single esnprintf to count + fits in int, when using sprintf. Use single snprintf to count length of string rather than counting it via multiple sprintfs; that's simpler and more reliable. (font_unparse_fcname): Use it to avoid sprintf buffer overrun. @@ -102,9 +102,6 @@ to avoid potential buffer overrun. * xterm.c (x_io_error_quitter): Don't overrun sprintf buffer. - (x_term_init): Use sprintf, not snprintf, so that we need not - worry about ancient hosts that lack snprintf. The buffer cannot - possibly be overrun, so this is safe. * xterm.h (x_check_errors): Add ATTRIBUTE_FORMAT_PRINTF. diff --git a/src/dbusbind.c b/src/dbusbind.c index fd9a43aaf86..8dac2a6249f 100644 --- a/src/dbusbind.c +++ b/src/dbusbind.c @@ -284,6 +284,7 @@ xd_signature (char *signature, unsigned int dtype, unsigned int parent_type, Lis unsigned int subtype; Lisp_Object elt; char const *subsig; + int subsiglen; char x[DBUS_MAXIMUM_SIGNATURE_LENGTH]; elt = object; @@ -365,9 +366,9 @@ xd_signature (char *signature, unsigned int dtype, unsigned int parent_type, Lis elt = CDR_SAFE (XD_NEXT_VALUE (elt)); } - if (esnprintf (signature, DBUS_MAXIMUM_SIGNATURE_LENGTH, - "%c%s", dtype, subsig) - == DBUS_MAXIMUM_SIGNATURE_LENGTH - 1) + subsiglen = snprintf (signature, DBUS_MAXIMUM_SIGNATURE_LENGTH, + "%c%s", dtype, subsig); + if (! (0 <= subsiglen && subsiglen < DBUS_MAXIMUM_SIGNATURE_LENGTH)) string_overflow (); break; @@ -2088,32 +2089,45 @@ usage: (dbus-register-signal BUS SERVICE PATH INTERFACE SIGNAL HANDLER &rest ARG connection = xd_initialize (bus, TRUE); /* Create a rule to receive related signals. */ - rulelen = esnprintf (rule, sizeof rule, - "type='signal',interface='%s',member='%s'", - SDATA (interface), - SDATA (signal)); + rulelen = snprintf (rule, sizeof rule, + "type='signal',interface='%s',member='%s'", + SDATA (interface), + SDATA (signal)); + if (! (0 <= rulelen && rulelen < sizeof rule)) + string_overflow (); /* Add unique name and path to the rule if they are non-nil. */ if (!NILP (uname)) - rulelen += esnprintf (rule + rulelen, sizeof rule - rulelen, + { + int len = snprintf (rule + rulelen, sizeof rule - rulelen, ",sender='%s'", SDATA (uname)); + if (! (0 <= len && len < sizeof rule - rulelen)) + string_overflow (); + rulelen += len; + } if (!NILP (path)) - rulelen += esnprintf (rule + rulelen, sizeof rule - rulelen, + { + int len = snprintf (rule + rulelen, sizeof rule - rulelen, ",path='%s'", SDATA (path)); + if (! (0 <= len && len < sizeof rule - rulelen)) + string_overflow (); + rulelen += len; + } /* Add arguments to the rule if they are non-nil. */ for (i = 6; i < nargs; ++i) if (!NILP (args[i])) { + int len; CHECK_STRING (args[i]); - rulelen += esnprintf (rule + rulelen, sizeof rule - rulelen, - ",arg%"pD"d='%s'", i - 6, SDATA (args[i])); + len = snprintf (rule + rulelen, sizeof rule - rulelen, + ",arg%"pD"d='%s'", i - 6, SDATA (args[i])); + if (! (0 <= len && len < sizeof rule - rulelen)) + string_overflow (); + rulelen += len; } - if (rulelen == sizeof rule - 1) - string_overflow (); - /* Add the rule to the bus. */ dbus_error_init (&derror); dbus_bus_add_match (connection, rule, &derror); diff --git a/src/dispnew.c b/src/dispnew.c index 0cc888b4b7a..5c28d014819 100644 --- a/src/dispnew.c +++ b/src/dispnew.c @@ -272,16 +272,16 @@ add_window_display_history (struct window *w, const char *msg, int paused_p) buf = redisplay_history[history_idx].trace; ++history_idx; - esnprintf (buf, sizeof redisplay_history[0].trace, - "%"pMu": window %p (`%s')%s\n%s", - history_tick++, - w, - ((BUFFERP (w->buffer) - && STRINGP (BVAR (XBUFFER (w->buffer), name))) - ? SSDATA (BVAR (XBUFFER (w->buffer), name)) - : "???"), - paused_p ? " ***paused***" : "", - msg); + snprintf (buf, sizeof redisplay_history[0].trace, + "%"pMu": window %p (`%s')%s\n%s", + history_tick++, + w, + ((BUFFERP (w->buffer) + && STRINGP (BVAR (XBUFFER (w->buffer), name))) + ? SSDATA (BVAR (XBUFFER (w->buffer), name)) + : "???"), + paused_p ? " ***paused***" : "", + msg); } diff --git a/src/doprnt.c b/src/doprnt.c index dae1dab04d7..638fa4d6312 100644 --- a/src/doprnt.c +++ b/src/doprnt.c @@ -486,27 +486,6 @@ esprintf (char *buf, char const *format, ...) 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 diff --git a/src/font.c b/src/font.c index a5b873aea51..34cacb37ce4 100644 --- a/src/font.c +++ b/src/font.c @@ -1285,14 +1285,14 @@ font_unparse_xlfd (Lisp_Object font, int pixel_size, char *name, int nbytes) } else f[XLFD_AVGWIDTH_INDEX] = "*"; - 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; + 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; } /* Parse NAME (null terminated) and store information in FONT @@ -1593,31 +1593,75 @@ font_unparse_fcname (Lisp_Object font, int pixel_size, char *name, int nbytes) p = name; lim = name + nbytes; if (! NILP (family)) - p += esnprintf (p, lim - p, "%s", SSDATA (family)); + { + int len = snprintf (p, lim - p, "%s", SSDATA (family)); + if (! (0 <= len && len < lim - p)) + return -1; + p += len; + } if (point_size > 0) - p += esnprintf (p, lim - p, "-%d" + (p == name), point_size); + { + int len = snprintf (p, lim - p, "-%d" + (p == name), point_size); + if (! (0 <= len && len < lim - p)) + return -1; + p += len; + } else if (pixel_size > 0) - p += esnprintf (p, lim - p, ":pixelsize=%d", pixel_size); + { + int len = snprintf (p, lim - p, ":pixelsize=%d", pixel_size); + if (! (0 <= len && len < lim - p)) + return -1; + p += len; + } if (! NILP (AREF (font, FONT_FOUNDRY_INDEX))) - p += esnprintf (p, lim - p, ":foundry=%s", - SSDATA (SYMBOL_NAME (AREF (font, - FONT_FOUNDRY_INDEX)))); + { + int len = snprintf (p, lim - p, ":foundry=%s", + SSDATA (SYMBOL_NAME (AREF (font, + FONT_FOUNDRY_INDEX)))); + if (! (0 <= len && len < lim - p)) + return -1; + p += len; + } for (i = 0; i < 3; i++) if (! NILP (styles[i])) - p += esnprintf (p, lim - p, ":%s=%s", style_names[i], - SSDATA (SYMBOL_NAME (styles[i]))); + { + int len = snprintf (p, lim - p, ":%s=%s", style_names[i], + SSDATA (SYMBOL_NAME (styles[i]))); + if (! (0 <= len && len < lim - p)) + return -1; + p += len; + } + if (INTEGERP (AREF (font, FONT_DPI_INDEX))) - p += esnprintf (p, lim - p, ":dpi=%"pI"d", - XINT (AREF (font, FONT_DPI_INDEX))); + { + int len = snprintf (p, lim - p, ":dpi=%"pI"d", + XINT (AREF (font, FONT_DPI_INDEX))); + if (! (0 <= len && len < lim - p)) + return -1; + p += len; + } + if (INTEGERP (AREF (font, FONT_SPACING_INDEX))) - p += esnprintf (p, lim - p, ":spacing=%"pI"d", - XINT (AREF (font, FONT_SPACING_INDEX))); + { + int len = snprintf (p, lim - p, ":spacing=%"pI"d", + XINT (AREF (font, FONT_SPACING_INDEX))); + if (! (0 <= len && len < lim - p)) + return -1; + p += len; + } + if (INTEGERP (AREF (font, FONT_AVGWIDTH_INDEX))) - p += esnprintf (p, lim - p, - (XINT (AREF (font, FONT_AVGWIDTH_INDEX)) == 0 - ? ":scalable=true" - : ":scalable=false")); - return lim - p == 1 ? -1 : p - name; + { + int len = snprintf (p, lim - p, + (XINT (AREF (font, FONT_AVGWIDTH_INDEX)) == 0 + ? ":scalable=true" + : ":scalable=false")); + if (! (0 <= len && len < lim - p)) + return -1; + p += len; + } + + return (p - name); } /* Parse NAME (null terminated) and store information in FONT diff --git a/src/lisp.h b/src/lisp.h index 2f6ec38f228..5c84bb8e06e 100644 --- a/src/lisp.h +++ b/src/lisp.h @@ -2897,8 +2897,6 @@ 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); diff --git a/src/xterm.c b/src/xterm.c index 72e9f2b2236..86393cf411f 100644 --- a/src/xterm.c +++ b/src/xterm.c @@ -7900,8 +7900,8 @@ x_io_error_quitter (Display *display) { char buf[256]; - esnprintf (buf, sizeof buf, "Connection lost to X server `%s'", - DisplayString (display)); + snprintf (buf, sizeof buf, "Connection lost to X server `%s'", + DisplayString (display)); x_connection_closed (display, buf); return 0; } @@ -10278,8 +10278,8 @@ x_term_init (Lisp_Object display_name, char *xrm_option, char *resource_name) atom_names[i] = (char *) atom_refs[i].name; /* Build _XSETTINGS_SN atom name */ - sprintf (xsettings_atom_name, - "_XSETTINGS_S%d", XScreenNumberOfScreen (dpyinfo->screen)); + snprintf (xsettings_atom_name, sizeof (xsettings_atom_name), + "_XSETTINGS_S%d", XScreenNumberOfScreen (dpyinfo->screen)); atom_names[i] = xsettings_atom_name; XInternAtoms (dpyinfo->display, atom_names, total_atom_count,