[PATCH 21.4] gnuclient buffer overflow fixes

PATCH 21.4 Vin Shelton <acs(a)xemacs.org&gt; wrote: Okay, here you go. lib-src/ChangeLog addition: 2006-08-11 Jerry James <james(a)xemacs.org&gt; * gnuslib.c (disconnect_from_server): shutdown() has been fine on Linux for a long time now; use it. Also, don't use length to access the buffer unless it is positive, not just nonzero. * gnuclient.c (filename_expand): Initialize the last array element to get a valid C string in case of overflow. Use strncat to avoid buffer overruns. * gnuclient.c (main): Use strncpy to avoid buffer overruns. xemacs-21.4 source patch: Diff command: cvs -q diff -uN Files affected: lib-src/gnuslib.c lib-src/gnuclient.c Index: lib-src/gnuclient.c =================================================================== RCS file: /pack/xemacscvs/XEmacs/xemacs/lib-src/gnuclient.c,v retrieving revision 1.22.2.1 diff -d -u -r1.22.2.1 gnuclient.c --- lib-src/gnuclient.c 2002/08/20 11:34:19 1.22.2.1 +++ lib-src/gnuclient.c 2006/08/11 17:26:50 ＠＠ -198,7 +198,7 ＠＠ #endif int len; - fullpath[0] = '\0'; + fullpath[0] = fullpath[MAXPATHLEN] = '\0'; #ifdef CYGWIN /* ＠＠ -211,7 +211,7 ＠＠ if (filename[0] && filename[0] == '/') { /* Absolute (unix-style) pathname. Do nothing */ - strcat (fullpath, filename); + strncat (fullpath, filename, MAXPATHLEN); } else { ＠＠ -219,15 +219,18 ＠＠ and prepend it. FIXME: need to fix the case of DOS paths like "\foo", where we need to get the current drive. */ - strcat (fullpath, get_current_working_directory ()); + strncat (fullpath, get_current_working_directory (), MAXPATHLEN); len = strlen (fullpath); - if (len > 0 && fullpath[len-1] == '/') /* trailing slash already? */ - ; /* yep */ - else - strcat (fullpath, "/"); /* nope, append trailing slash */ + /* If no trailing slash, add one */ + if (len <= 0 || (fullpath[len - 1] != '/' && len < MAXPATHLEN)) + { + strcat (fullpath, "/"); + len++; + } + /* Don't forget to add the filename! */ - strcat (fullpath,filename); + strncat (fullpath, filename, MAXPATHLEN - len); } } /* filename_expand */ ＠＠ -439,7 +442,7 ＠＠ break; case 'r': GET_ARGUMENT (remotearg, "-r"); - strcpy (remotepath, remotearg); + strncpy (remotepath, remotearg, MAXPATHLEN); rflg = 1; break; #endif /* INTERNET_DOMAIN_SOCKETS */ ＠＠ -594,7 +597,7 ＠＠ * to this machine */ if ((ptr = getenv ("GNU_NODE")) != NULL) /* user specified a path */ - strcpy (remotepath, ptr); + strncpy (remotepath, ptr, MAXPATHLEN); } #if 0 /* This is really bogus... re-enable it if you must have it! */ #if defined (hp9000s300) || defined (hp9000s800) Index: lib-src/gnuslib.c =================================================================== RCS file: /pack/xemacscvs/XEmacs/xemacs/lib-src/gnuslib.c,v retrieving revision 1.10.2.1 diff -d -u -r1.10.2.1 gnuslib.c --- lib-src/gnuslib.c 2001/07/25 07:47:25 1.10.2.1 +++ lib-src/gnuslib.c 2006/08/11 17:26:51 ＠＠ -411,13 +411,11 ＠＠ send_string(s,EOT_STR); /* make sure server gets string */ -#if !defined (linux) && !defined (_SCO_DS) +#if !defined (_SCO_DS) /* - * shutdown is completely hozed under linux. If s is a unix domain socket, - * you'll get EOPNOTSUPP back from it. If s is an internet socket, you get - * a broken pipe when you try to read a bit later. The latter - * problem is fixed for linux versions >= 1.1.46, but the problem - * with unix sockets persists. Sigh. + * There used to be a comment here complaining about ancient Linux + * versions. It is no longer relevant. I don't know why _SCO_DS is + * verboten here, as the original comment did not say. */ if (shutdown(s,1) == -1) { ＠＠ -436,7 +434,7 ＠＠ #else while ((length = read(s,buffer,GSERV_BUFSZ)) > 0 || (length == -1 && errno == EINTR)) { - if (length) { + if (length > 0) { buffer[length] = '\0'; if (echo) { fputs(buffer,stdout); -- Jerry James, Assistant Professor james(a)xemacs.org Computer Science Department http://www.cs.usu.edu/~jerry/ Utah State University