Re: [PATCH] login-utils: Removed upper limit on passwd line size in is_local, added tests.

[Karel Zak <kzak@xxxxxxxxxx>]

On Sun, Apr 06, 2008 at 02:03:45PM +0100, James Youngman wrote:
>  login-utils/Makefile.am |    7 ++-
>  login-utils/islocal.c   |  164 +++++++++++++++++++++++++++++++++++++++++-----
>  login-utils/islocal.h   |    2 +-
>  3 files changed, 153 insertions(+), 20 deletions(-)

 Hmm...

>  int
> -is_local(char *user)
> +is_local(const char *user)
>  {
> -	FILE *fd;
> -	char line[MAX_LENGTH];
>  	int local = 0;
> -	size_t len;
> -
> -        if(!(fd = fopen(_PATH_PASSWD, "r"))) {
> -                fprintf(stderr,_("Can't read %s, exiting."),_PATH_PASSWD);
> -                exit(1);
> -        }
> +	size_t match;
> +	int chin, skip;
> +	FILE *f = fopen_passwd_file();
>  
> -	len = strlen(user);
> -        while(fgets(line, MAX_LENGTH, fd)) {
> -                if(!strncmp(line, user, len) && line[len] == ':') {
> -			local = 1;
> -			break;
> -                }
> +	match = 0u;
> +	skip = 0;
> +	while ((chin=getc(f)) != EOF) {
> +		if (skip) {
> +			/* Looking for the start of the next line. */
> +			if ('\n' == chin) {
> +				/* Start matching username at the next char. */
> +				skip = 0;
> +				match = 0u;
> +			}
> +		} else {
> +			if (':' == chin) {
> +				if (0 == user[match]) {
> +					local = 1; /* Success. */
> +					/* next line has no test coverage, but it is 
> +					 * just an optimisation anyway. */
> +					break;
> +				} else {
> +					/* we read a whole username, but it is 
> +					 * the wrong user.  Skip to the next 
> +					 * line. */
> +					skip = 1;
> +				}
> +			} else if ('\n' == chin) {
> +				/* This line contains no colon; it's malformed.
> +				 * No skip since we are already at the start of
> +				 * the next line. */
> +				match = 0u;
> +			} else if (chin != user[match]) {
> +				/* username does not match. */
> +				skip = 1;
> +			} else {
> +				++match;
> +			}
> +		}
>  	}
> -	fclose(fd);
> +	fclose(f);
>  	return local;
>  }

What about:

 login-utils/islocal.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/login-utils/islocal.c b/login-utils/islocal.c
index 2976980..7fa1434 100644
--- a/login-utils/islocal.c
+++ b/login-utils/islocal.c
@@ -24,13 +24,11 @@
 #include "pathnames.h"
 #include "islocal.h"
 
-#define MAX_LENGTH	1024
-
 int
 is_local(char *user)
 {
 	FILE *fd;
-	char line[MAX_LENGTH];
+	char *line;
 	int local = 0;
 	size_t len;
 
@@ -40,12 +38,17 @@ is_local(char *user)
         }
 
 	len = strlen(user);
-        while(fgets(line, MAX_LENGTH, fd)) {
-                if(!strncmp(line, user, len) && line[len] == ':') {
+	line = malloc(len + 2);         /* 2 = ":\0" */
+
+        while(fgets(line, len + 2, fd)) {
+		int c = line[len];
+                if(!strncmp(line, user, len) && c == ':') {
 			local = 1;
 			break;
                 }
+		while(c != '\n' && (c = fgetc(fd)) != EOF);
 	}
+	free(line);
 	fclose(fd);
 	return local;
 }


 It could be also easily reimplemented with fixed-length (LOGIN_NAME_MAX)
 buffer and fscanf().

> +#if MAIN_TEST_ISLOCAL 
> +static unsigned failures = 0u;
> +static const char *testdata = 
> +"root:x:0:0:root:/root:/bin/bash\n"
> +"sys:x:3:3:sys:/dev:/bin/sh\n"
> +"orac:x:33:33:sys:/dev:/bin/youngman\n"
> +/* Next line should be >1024 but <2048 */
> +"long:x:4:4:foo:/home/loooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo
>  oooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooonnnggg:/bin/sh\n"
> +"al:x:5:5:fnord:/dev:/bin/sh\n"
> +"abcdefgh:x:6:3:fnord:/dev:/bin/sh\n"
> +"malformed\n"
> +"\n"				/* see test case znobody */
> +"nobody:x:65534:65534:nobody:/nonexistent:/bin/sh\n"
> +"nonl:x:65532:65532:no newline:/:/bin/false";

 I really don't like this kind of test. We have the test/ directory
 where you can store all test data. Your MAIN_TEST_XXX needs to
 include really necessary _code_ only.

    Karel

-- 
 Karel Zak  <kzak@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe util-linux-ng" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html