From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13002 invoked by alias); 9 Dec 2006 12:08:30 -0000 Received: (qmail 12981 invoked by uid 22791); 9 Dec 2006 12:08:29 -0000 X-Spam-Check-By: sourceware.org Received: from sunsite.ms.mff.cuni.cz (HELO sunsite.mff.cuni.cz) (195.113.15.26) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sat, 09 Dec 2006 12:08:17 +0000 Received: from sunsite.mff.cuni.cz (localhost.localdomain [127.0.0.1]) by sunsite.mff.cuni.cz (8.13.8/8.13.8) with ESMTP id kB9C8FcL004323; Sat, 9 Dec 2006 13:08:15 +0100 Received: (from jakub@localhost) by sunsite.mff.cuni.cz (8.13.8/8.13.8/Submit) id kB9C8FxP004321; Sat, 9 Dec 2006 13:08:15 +0100 Date: Sat, 09 Dec 2006 12:08:00 -0000 From: Jakub Jelinek To: Ulrich Drepper Cc: Glibc hackers Subject: [PATCH] Fix initshells Message-ID: <20061209120814.GD3819@sunsite.mff.cuni.cz> Reply-To: Jakub Jelinek Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="dDRMvlgZJXvWKvBx" Content-Disposition: inline User-Agent: Mutt/1.4.2.2i Mailing-List: contact libc-hacker-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-hacker-owner@sourceware.org X-SW-Source: 2006-12/txt/msg00009.txt.bz2 --dDRMvlgZJXvWKvBx Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-length: 936 Hi! If /etc/shells ends with an empty line, initshells ends with an endless loop (because fgets with length 1 always succeeds, doesn't need to read anything from the file, just stores '\0'). In addition to this if /etc/shells contains e.g. / / / / / / / / then setusershell (); endusershell (); causes heap corruption. I'm attaching two patches, some fixes are common to both patches, but they differ in how the endless loop with empty newline at end is solved. P3 adds an extra condition to the loop, P5 increases the strings buffer by one byte, so that fgets itself cures this. I have also coded up a third solution, as we never shrink the strings buffer, we might as well just fread the whole file into the strings buffer and instead of fgets simply look for '\n's in it. While that is tiny bit more efficient, it compiled into bigger code (on x86-64) and I believe for this type of functions smaller code is better code. Jakub --dDRMvlgZJXvWKvBx Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename=P3 Content-length: 1544 2006-12-09 Jakub Jelinek * misc/getusershell.c (initshells): Check for integer overflows. Avoid endless loop if /etc/shells ends with an empty line. Don't use calloc for shells array. Disallow / as shell. --- libc/misc/getusershell.c.jj 2006-05-15 20:56:36.000000000 +0200 +++ libc/misc/getusershell.c 2006-12-09 12:16:30.000000000 +0100 @@ -98,7 +98,7 @@ initshells() register char **sp, *cp; register FILE *fp; struct stat64 statb; - int flen; + size_t flen; free(shells); shells = NULL; @@ -114,9 +114,11 @@ initshells() okshells[1] = _PATH_CSHELL; return (char **) okshells; } - if ((strings = malloc((u_int)statb.st_size + 1)) == NULL) + if (statb.st_size > ~(size_t)0 / sizeof (char *) * 3) goto init_okshells; - shells = calloc((unsigned)statb.st_size / 3, sizeof (char *)); + if ((strings = malloc(statb.st_size + 1)) == NULL) + goto init_okshells; + shells = malloc(statb.st_size / 3 * sizeof (char *)); if (shells == NULL) { free(strings); strings = NULL; @@ -124,11 +126,12 @@ initshells() } sp = shells; cp = strings; - flen = statb.st_size; - while (fgets_unlocked(cp, flen - (cp - strings), fp) != NULL) { + flen = statb.st_size + 1; + while (cp < strings + flen - 1 + && fgets_unlocked(cp, flen - (cp - strings), fp) != NULL) { while (*cp != '#' && *cp != '/' && *cp != '\0') cp++; - if (*cp == '#' || *cp == '\0') + if (*cp == '#' || *cp == '\0' || cp[1] == '\0') continue; *sp++ = cp; while (!isspace(*cp) && *cp != '#' && *cp != '\0') --dDRMvlgZJXvWKvBx Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename=P5 Content-length: 1468 2006-12-09 Jakub Jelinek * misc/getusershell.c (initshells): Check for integer overflows. Make strings buffer one bigger as fgets always succeeds when second argument is 1. Don't use calloc for shells array. Disallow / as shell. --- libc/misc/getusershell.c.jj 2006-05-15 20:56:36.000000000 +0200 +++ libc/misc/getusershell.c 2006-12-09 12:53:05.000000000 +0100 @@ -98,7 +98,7 @@ initshells() register char **sp, *cp; register FILE *fp; struct stat64 statb; - int flen; + size_t flen; free(shells); shells = NULL; @@ -114,9 +114,11 @@ initshells() okshells[1] = _PATH_CSHELL; return (char **) okshells; } - if ((strings = malloc((u_int)statb.st_size + 1)) == NULL) + if (statb.st_size > ~(size_t)0 / sizeof (char *) * 3) goto init_okshells; - shells = calloc((unsigned)statb.st_size / 3, sizeof (char *)); + if ((strings = malloc(statb.st_size + 2)) == NULL) + goto init_okshells; + shells = malloc(statb.st_size / 3 * sizeof (char *)); if (shells == NULL) { free(strings); strings = NULL; @@ -124,11 +126,11 @@ initshells() } sp = shells; cp = strings; - flen = statb.st_size; + flen = statb.st_size + 2; while (fgets_unlocked(cp, flen - (cp - strings), fp) != NULL) { while (*cp != '#' && *cp != '/' && *cp != '\0') cp++; - if (*cp == '#' || *cp == '\0') + if (*cp == '#' || *cp == '\0' || cp[1] == '\0') continue; *sp++ = cp; while (!isspace(*cp) && *cp != '#' && *cp != '\0') --dDRMvlgZJXvWKvBx--