From 17107bb698f82bb5b9c8a732cd6b3faaad4d7af6 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Sun, 28 Aug 2011 16:52:34 -0700 Subject: 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(-) (limited to 'lib-src') 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; -- cgit v1.2.1 From 9250f758254937f43a621a2371e3433ce7daa573 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Sun, 28 Aug 2011 16:55:41 -0700 Subject: * 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(-) (limited to 'lib-src') 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) -- cgit v1.2.1 From 644a0faa36ad8c1e251d198c2bc69f17c8bdb83a Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Sun, 28 Aug 2011 16:57:19 -0700 Subject: * 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(-) (limited to 'lib-src') 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); -- cgit v1.2.1 From 0c6d656d269f84c83c483057eac6081eddfd33a0 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Sun, 28 Aug 2011 16:59:14 -0700 Subject: * 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(-) (limited to 'lib-src') 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; -- cgit v1.2.1 From 005d87bd2306e943a16b86c36d1482651d9932d8 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Sun, 28 Aug 2011 17:27:35 -0700 Subject: Add Bug#. --- lib-src/ChangeLog | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib-src') 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. -- cgit v1.2.1