[Pkg-shadow-devel] [PATCH 04/11] Add backend support for suboridnate uids and gids

[Eric W. Biederman]

"Serge E. Hallyn" <serge at hallyn.com> writes:

> Quoting Eric W. Biederman (ebiederm at xmission.com):
>> +static void *subordinate_parse (const char *line)
>> +{
>> +	static struct subordinate_range range;
>> +	char rangebuf[1024];
>> +	int i;
>> +	char *cp;
>> +	char *fields[NFIELDS];
>> +
>> +	/*
>> +	 * Copy the string to a temporary buffer so the substrings can
>> +	 * be modified to be NULL terminated.
>> +	 */
>> +	if (strlen (line) >= sizeof rangebuf)
>> +		return NULL;	/* fail if too long */
>> +	strcpy (rangebuf, line);
>> +
>> +	/*
>> +	 * Save a pointer to the start of each colon separated
>> +	 * field.  The fields are converted into NUL terminated strings.
>> +	 */
>> +
>> +	for (cp = rangebuf, i = 0; (i < NFIELDS) && (NULL != cp); i++) {
>> +		fields[i] = cp;
>> +		while (('\0' != *cp) && (':' != *cp)) {
>> +			cp++;
>> +		}
>> +
>> +		if ('\0' != *cp) {
>> +			*cp = '\0';
>> +			cp++;
>> +		} else {
>> +			cp = NULL;
>> +		}
>> +	}
>> +
>> +	/*
>> +	 * There must be exactly NFIELDS colon separated fields or
>> +	 * the entry is invalid.  Also, fields must be non-blank.
>> +	 */
>> +	if (i != NFIELDS || *fields[0] == '\0' || *fields[1] == '\0' || *fields[2] == '\0')
>> +		return NULL;
>> +	range.owner = fields[0];
>> +	if (getulong (fields[1], &range.start) == 0)
>> +		return NULL;
>> +	if (getulong (fields[2], &range.count) == 0)
>> +		return NULL;
>> +
>> +	return ⦥
>
> This function worries me, because you're returning local contents (both range
> itself, and range.owner).

In practice that function is fine, and is mostly in the style of the
other functions that implement parse.  As parse is expected to return
a reference to a statically allocated buffer.

Furthermore the only use in commonio.c is to do:
	if (NULL != parse(...))
        	dup(...)

But it does look like when I made the user name a string I did deviate
from the norm and have failed to copy the string to a statically
allocated buffer.

In practice it doesn't matter but it looks worth fixing for the sake of
long term maintenance.

Eric