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

[Nathan Wayde]

On 21/12/09 23:19, Allan McRae wrote:
> Nathan Wayde wrote:
>> On 21/12/09 09:55, Allan McRae wrote:
>>> When pacman queries the ownership of an object that is not a path,
>>> it will check in the users PATH for a match. Implements FS#8798.
>>>
>>> Patch-by: Shankar<jatheendra at gmail.com>
>>> [Allan: rework for master, tidy-up]
>>> Signed-off-by: Allan McRae<allan at archlinux.org>
>>> ---
>>>
>>> This was original posted a year ago...
>>> http://mailman.archlinux.org/pipermail/pacman-dev/2008-November/007659.html
>>>
>>>
>>> The comments then were that we should maybe refactor out stuff
>>> dealing with
>>> file paths, specifically removing trailing backslashes. That appears
>>> to only
>>> occur in two other places and is very simple so I do not thing it needs
>>> refactored and can be done in a spearate patch if necessary.
>>>
>>> The only other part I do not like is the "failed to read file" error
>>> block is
>>> repeated, but I can not see a nice way to fix that.
>>>
>>> src/pacman/query.c | 58
>>> +++++++++++++++++++++++++++++++++++++++++++++++----
>>> 1 files changed, 53 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/pacman/query.c b/src/pacman/query.c
>>> index 6b6a25d..6f579fd 100644
>>> --- a/src/pacman/query.c
>>> +++ b/src/pacman/query.c
>>> @@ -56,6 +56,41 @@ static char *resolve_path(const char* file)
>>> return(str);
>>> }
>>>
>>> +/* check if filename exists in PATH */
>>> +static int search_path(char *filename, struct stat * bufptr)
>>> +{
>>> + char *envpath, *path, *fullname;
>>> + int len;
>>> +
>>> + if ((envpath = getenv("PATH")) == NULL) {
>>> + return(-1);
>>> + }
>>> + if ((envpath = strdup(envpath)) == NULL) {
>>> + return(-1);
>>> + }
>>> +
>>> + fullname = calloc(PATH_MAX+1, sizeof(char));
>>> +
>>> + while ((path = strsep(&envpath, ":")) != NULL) {
>>> + 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);
>>> +
>>> + if(stat(fullname, bufptr) == 0) {
>>> + strncpy(filename, fullname, PATH_MAX+1);
>>> + free(fullname);
>>> + return(0);
>>> + }
>>> + }
>>> + free(fullname);
>>> + return(-1);
>>> +}
>>> +
>>
>> just a heads-up, search_path() contains a memory leak; envpath is
>> never free()'d. Note, the original envpath is lost in the call to
>> strsep().
>>
>
> So it does... someone did not run valgrind on the patch :P
>
> If I understand this correctly, I need to have another variable that
> keeps track of the pointer to the strdup object while the original gets
> split. I.e.:
>
> +/* check if filename exists in PATH */
> +static int search_path(char *filename, struct stat * bufptr)
> +{
> + char *envpath, *envpathsplit, *path, *fullname;
> + int len;
> +
> + if ((envpath = getenv("PATH")) == NULL) {
> + return(-1);
> + }
> + if ((envpath = envpathsplit = strdup(envpath)) == NULL) {
> + return(-1);
> + }
> +
> + fullname = calloc(PATH_MAX+1, sizeof(char));
> +
> + while ((path = strsep(&envpathsplit, ":")) != NULL) {
> + 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);
> +
> + if(stat(fullname, bufptr) == 0) {
> + strncpy(filename, fullname, PATH_MAX+1);
> + free(envpath);
> + free(fullname);
> + return(0);
> + }
> + }
> + free(envpath);
> + free(fullname);
> + return(-1);
> +}
>
> That seems to make valgrind happy and keeps everything working. Does
> that make sense or am I doing something stupid...
>
> Updated patch on my working branch.
>
> Allan
>
>
Yeah that's correct.
I looked at it again tweaked it a bit because I had nothing better to 
do, if you're interested:

+/* 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) {
+        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);
+}

* Moved fullname to the stack as it was a constant size and calloc can 
fail.
* Removed the end / stripping, /bin//sh is perfectly valid and stripping 
saves all but 1 byte while ignoring paths such as :/bin//:.
* 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.