[pacman-dev] [PATCH] Allow -Qo to perform a functional 'which'
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@gmail.com> [Allan: rework for master, tidy-up] Signed-off-by: Allan McRae <allan@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); +} + static int query_fileowner(alpm_list_t *targets) { int ret = 0; @@ -67,19 +102,31 @@ static int query_fileowner(alpm_list_t *targets) return(1); } + char *filename = calloc(PATH_MAX+1, sizeof(char)); + for(t = targets; t; t = alpm_list_next(t)) { int found = 0; - char *filename = alpm_list_getdata(t); + strncpy(filename, alpm_list_getdata(t), PATH_MAX+1); char *bname, *dname, *rpath; const char *root; struct stat buf; alpm_list_t *i, *j; if(lstat(filename, &buf) == -1) { - pm_fprintf(stderr, PM_LOG_ERROR, _("failed to read file '%s': %s\n"), - filename, strerror(errno)); - ret++; - continue; + /* if it is not a path but a program name, then check in PATH */ + if(strchr(filename, '/') == NULL) { + if(search_path(filename, &buf) == -1) { + pm_fprintf(stderr, PM_LOG_ERROR, _("failed to read file '%s': %s\n"), + filename, strerror(errno)); + ret++; + continue; + } + } else { + pm_fprintf(stderr, PM_LOG_ERROR, _("failed to read file '%s': %s\n"), + filename, strerror(errno)); + ret++; + continue; + } } if(S_ISDIR(buf.st_mode)) { @@ -140,6 +187,7 @@ static int query_fileowner(alpm_list_t *targets) free(rpath); } + free(filename); return ret; } -- 1.6.5.6
On Mon, Dec 21, 2009 at 10:55 AM, Allan McRae <allan@archlinux.org> 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.
Just one quick comment now : The current behavior with pacman -Qo foo (with no /) will check the current directory (well its just stat). With the new behavior, it seems it will no longer do that. But one workaround should be ./foo And maybe that is fine. Correct me if I am wrong :)
Xavier wrote:
On Mon, Dec 21, 2009 at 10:55 AM, Allan McRae <allan@archlinux.org> 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.
Just one quick comment now : The current behavior with pacman -Qo foo (with no /) will check the current directory (well its just stat). With the new behavior, it seems it will no longer do that. But one workaround should be ./foo And maybe that is fine.
Correct me if I am wrong :)
It appears that you are wrong: allan@arch /usr/lib
~/code/pacman/src/pacman/pacman -Qo libalpm.so.4 libalpm.so.4 is owned by pacman 3.3.3-1
/usr/lib is not in my PATH... This makes sense, as it only tests if the variable contains "/" and searches the PATH after the initial stat fails. Allan
On Mon, Dec 21, 2009 at 10:03 PM, Allan McRae <allan@archlinux.org> wrote:
It appears that you are wrong:
allan@arch /usr/lib
~/code/pacman/src/pacman/pacman -Qo libalpm.so.4 libalpm.so.4 is owned by pacman 3.3.3-1
/usr/lib is not in my PATH...
This makes sense, as it only tests if the variable contains "/" and searches the PATH after the initial stat fails.
Sorry, I read the code too quickly and besides that, I don't know what I was thinking but it was completely wrong. Sorry for the noise :P
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@gmail.com> [Allan: rework for master, tidy-up] Signed-off-by: Allan McRae<allan@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().
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@gmail.com> [Allan: rework for master, tidy-up] Signed-off-by: Allan McRae<allan@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
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@gmail.com> [Allan: rework for master, tidy-up] Signed-off-by: Allan McRae<allan@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.
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@gmail.com> [Allan: rework for master, tidy-up] Signed-off-by: Allan McRae<allan@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
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); +}
participants (3)
-
Allan McRae
-
Nathan Wayde
-
Xavier