MInor fixes and problems for malloced getpw/grxxx functions for bb

tito farmatito at tiscali.it
Sun Jan 4 23:14:04 UTC 2015


Hi Denys,

while trying to fully understand the new pwd_grp code I've spotted
some minor problems that need to be fixed:

1) in function parse_common count should start at 1 as line numbers are off by one.

--- libpwdgrp/pwd_grp.c.original        2015-01-04 22:36:40.000000000 +0100
+++ libpwdgrp/pwd_grp.c 2015-01-04 22:40:17.904401220 +0100
@@ -189,7 +189,7 @@ static int tokenize(char *buffer, int ch
 static char *parse_common(FILE *fp, struct passdb *db,
                const char *key, int field_pos)
 {
-       int count = 0;
+       int count = 1;
        char *buf;
 
        while ((buf = xmalloc_fgetline(fp)) != NULL) {


2) in parse_common the use of key variable seems problematic to me:

	if (!key) {
			/* no key specified: sequential read, return a record */
			break;
	}

This variable string is passed on from the caller and eventually from the user
we have no guarantee that it will be non NULL when we don't want a sequential read.
If the calling app passes a bad pointer it will get a record returned.
I  used field_pos set to -1 to signal the parser that I wanted a sequential read.

				if (field_pos >= 0) {
					if (*(s = nth_string(buf, field_pos))) {
						if (key && *key) {
							if (strcmp(key, s) == 0) {
								/* record found */
								break;
							}
						} /* else skip: caller passed bad key */
					} /* else skip: field is empty */
				} else {  /* field_pos < 0 */
					/* No field specified (-1): sequential read, return current record */
					break;
				}

3)  in parse_common function 

               while ((buf = xmalloc_fgetline(fp)) != NULL) {

     does not set errno to ENOENT on EOF, this makes segfault
     code that works with glibc like e.g.:

    	while (1) {
		ret = getpwent_r(&pwd, buffer, 256, &pw);
		if (ret) {
			if (ret != ENOENT) {
				printf("getpwent_r: ret %s\n", strerror(ret));
			}
			break;
		}
		printf("%s (%d)\tHOME %s\tSHELL %s\n", pw->pw_name,
			pw->pw_uid, pw->pw_dir, pw->pw_shell);
	}

       Man page for getpwent_r says: 

RETURN VALUE
       On  success,  these  functions  return  0  and *pwbufp is a pointer to the struct passwd.  On
       error, these functions return an error value and *pwbufp is NULL.

ERRORS
       ENOENT No more entries.

       ERANGE Insufficient buffer space supplied.  Try again with larger buffer.

The fix with the actual code is not so easy because if you fix the getpwent_r case  (return ENOENT on EOF)
you break the getpwnam_r/getspnam_r case (0 on USER not found).


4) in function parse_common: 

		while (p < S.tokenize_end)
			if (*p++ == ',')
				cnt++;

would be more readable with braces {}, but that is just my personal taste ;-)

Ciao,
Tito




More information about the busybox mailing list