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: Shankarjatheendra@gmail.com [Allan: rework for master, tidy-up] Signed-off-by: Allan McRaeallan@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

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: Shankarjatheendra@gmail.com [Allan: rework for master, tidy-up] Signed-off-by: Allan McRaeallan@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