[pacman-dev] [PATCH] Allow -Qo to perform a functional 'which'

[Nathan Wayde]

On 23/12/09 09:20, Allan McRae wrote:
> Nathan Wayde wrote:
[...]
>> * Moved fullname to the stack as it was a constant size and calloc can
>> fail.
the original was:
     +	    fullname = calloc(PATH_MAX+1, sizeof(char));
in this case the size allocated is constant and not very large, by 
allocating it directly on the stack we avoid the dynamic allocation of 
calloc which could return NULL, the return isn't checked either so would 
likely result in a random crash when fullname is written to.

>> * Removed the end / stripping, /bin//sh is perfectly valid and
>> stripping saves all but 1 byte while ignoring paths such as :/bin//:.
>
> I do not understand this point:
>

+		len = strlen(path);
+
+		/* strip the trailing slash if one exists */
+		if(path[len - 1] == '/') {
+				path[len - 1] = '\0';
+		}
+
+		snprintf(fullname, PATH_MAX+1, "%s/%s", path, filename);

it will strip the trailing slash which may come from cases where the 
$PATH contains entries like: PATH=/sbin:/bin/: so /bin/ is fixed to be 
/bin. When snprintf returns we have a path such as /bin/command which, 
although it looks nicer, is no different than /bin//command. It IMHO 
wasted time stripping the last slash but ignores cases where 
PATH=/sbin:/bin//:
if the stripping is desired maybe it'd be better implemented as

+		len = strlen(path);
+		while (path[len - 1] == '/') {
+			path[--len] = '\0';
+		}

that way all the trailing slashes are stripped.

>> * Added a parameter for the (buffer) size of filename, I had a nasty
>> crash when I tested the other tweaks only to find that seaarch_path
>> assumes the size of filename is at least PATH_MAX+1.
>
> filename is created with:
> char *filename = calloc(PATH_MAX+1, sizeof(char));
>
> in query_fileowner which is the value passed to the search_path
> function. So filename is of size PATH_MAX+1.
>
> Can you clarify?
>
> Thanks,
> Allan
>
>
Yes it works fine as-is because the the buffer that query_fileowner 
passes in is of an appropriate size but if you call search_path 
somewhere else then you must also guarantee that the filename can hold 
at least PATH_MAX+1 chars, otherwise you have a buffer overflow when you 
copy fullname into it:

+			strncpy(filename, fullname, PATH_MAX+1);



I think path needs to be checked because it could be an empty string in 
the case of an empty field (PATH=/bin:<empty_field>:<empty_field>) 
otherwise you end up with fullname being "/command" after the snprintf 
which I'm not sure is desired.

+/* check if filename exists in PATH */
+static int search_path(char *filename, size_t filename_len, struct stat 
* bufptr)
+{
+	char *envpath, *envpathsplit, *path, fullname[PATH_MAX+1];
+
+	if ((envpath = getenv("PATH")) == NULL) {
+		return(-1);
+	}
+	if ((envpath = envpathsplit = strdup(envpath)) == NULL) {
+		return(-1);
+	}
+
+	while ((path = strsep(&envpathsplit, ":")) != NULL) {
		/* path can be an empty string */
+		if (*path) {
+			snprintf(fullname, sizeof(fullname), "%s/%s", path, filename);
+
+			if (stat(fullname, bufptr) == 0) {
+				strncpy(filename, fullname, filename_len);
+				free(envpath);
+				return(0);
+			}
		}
+	}
+	free(envpath);
+	return(-1);
+}