public inbox for libc-hacker@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Fix initshells
@ 2006-12-09 12:08 Jakub Jelinek
  0 siblings, 0 replies; only message in thread
From: Jakub Jelinek @ 2006-12-09 12:08 UTC (permalink / raw)
  To: Ulrich Drepper; +Cc: Glibc hackers

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

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

[-- Attachment #2: P3 --]
[-- Type: text/plain, Size: 1544 bytes --]

2006-12-09  Jakub Jelinek  <jakub@redhat.com>

	* 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')

[-- Attachment #3: P5 --]
[-- Type: text/plain, Size: 1468 bytes --]

2006-12-09  Jakub Jelinek  <jakub@redhat.com>

	* 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')

^ permalink raw reply	[flat|nested] only message in thread

only message in thread, other threads:[~2006-12-09 12:08 UTC | newest]

Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-12-09 12:08 [PATCH] Fix initshells Jakub Jelinek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).