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

[Allan McRae]

Nathan Wayde wrote:
> 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//:.

I do not understand this point:

> * 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