On Sat, Mar 6, 2010 at 10:02 PM, 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.
Original-patch-by: Shankar <jatheendra@gmail.com> Signed-off-by: Allan McRae <allan@archlinux.org> ---
This should address all comments I had when this was submitted a few months back.
Until I'm about to merge it tonight! I have a few small ideas/tweaks. :P
src/pacman/query.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 52 insertions(+), 5 deletions(-)
diff --git a/src/pacman/query.c b/src/pacman/query.c index 6b6a25d..038072f 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -56,10 +56,44 @@ static char *resolve_path(const char* file) return(str); }
+/* check if filename exists in PATH */ +static int search_path(char *filename, size_t size, struct stat * bufptr) no space needed between * and bufptr +{ + char *envpath, *envpathsplit, *path, fullname[PATH_MAX+1];
Perhaps I am being a bit overkill, but I'd prefer: char *envpath, *envpathsplit, *path; char fullname[PATH_MAX+1]; just to separate the ptrs from the real thing.
+ int len; + + if ((envpath = getenv("PATH")) == NULL) { + return(-1); + }
Not that you'd see this regularly, but sudo can sometimes play weird games with envvars among other things. This will cause a rather nonsensical error: $ PATH= ./src/pacman/.libs/lt-pacman -Qo pacman error: failed to read file 'pacman': No such file or directory $ ./src/pacman/.libs/lt-pacman -Qo pacman /usr/bin/pacman is owned by pacman-git 20100404-1
+ if ((envpath = envpathsplit = strdup(envpath)) == NULL) { + return(-1); + }
Same kind of deal. I think we need better wording to indicate we attempted to search the $PATH and failed, as opposed to couldn't locate a file in the current directory and/or looked for an absolute file path. $ ./src/pacman/.libs/lt-pacman -Qo foobar error: failed to read file 'foobar': No such file or directory
+ + while ((path = strsep(&envpathsplit, ":")) != NULL) { + len = strlen(path); + + /* strip the trailing slash if one exists */ + while(path[len - 1] == '/') { + path[--len] = '\0'; + } + + snprintf(fullname, sizeof(fullname), "%s/%s", path, filename); + + if(stat(fullname, bufptr) == 0) { Shouldn't we be using lstat just like the query_fileowner() code? Otherwise broken symlink from package X will be unqueryable?
+ strncpy(filename, fullname, size); + free(envpath); + return(0); + } + } + free(envpath); + return(-1); +} + static int query_fileowner(alpm_list_t *targets) { int ret = 0; alpm_list_t *t; + size_t len=PATH_MAX+1; We use spaces between operators everywhere else; should use them here too please.
/* This code is here for safety only */ if(targets == NULL) { @@ -67,19 +101,31 @@ static int query_fileowner(alpm_list_t *targets) return(1); }
+ char *filename = calloc(len, sizeof(char));
Why do we go through the "trouble" of allocating this one on the heap when every single call to search_path() just allocates something the same size on the stack?
+ for(t = targets; t; t = alpm_list_next(t)) { int found = 0; - char *filename = alpm_list_getdata(t); + strncpy(filename, alpm_list_getdata(t), len); 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, len, &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 +186,7 @@ static int query_fileowner(alpm_list_t *targets) free(rpath); }
+ free(filename); return ret; }
Oh, and more weirdness (my working directory was ~/projects/pacman/ with no etc in sight): $ ./src/pacman/.libs/lt-pacman -Qo etc error: cannot determine ownership of a directory $ pacman -Qo etc error: cannot determine ownership of a directory So this is not new, but worth looking at perhaps? -Dan