Libguestfs - Jun 2015 - Re: [PATCH] daemon: Rewrite prog_exists so it uses the actual PATH, not hard-coded list.

---
 daemon/guestfsd.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index 21b3600..c9cc8c5 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -243,9 +243,6 @@ main (int argc, char *argv[])
   /* Set up a basic environment.  After we are called by /init the
    * environment is essentially empty.
    * https://bugzilla.redhat.com/show_bug.cgi?id=502074#c5
-   *
-   * NOTE: if you change $PATH, you must also change 'prog_exists'
-   * function below.
    */
   setenv ("PATH", "/sbin:/usr/sbin:/bin:/usr/bin", 1);
   setenv ("SHELL", "/bin/sh", 1);
@@ -1440,22 +1437,34 @@ mountable_to_string (const mountable_t *mountable)
   }
 }
 
-/* Check program exists and is executable on $PATH.  Actually, we
- * just assume PATH contains the default entries (see main() above).
- */
+/* Check program exists and is executable on $PATH. */
 int
 prog_exists (const char *prog)
 {
-  static const char * const dirs[] -    { "/sbin",
"/usr/sbin", "/bin", "/usr/bin" };
-  size_t i;
-  char buf[1024];
-
-  for (i = 0; i < sizeof dirs / sizeof dirs[0]; ++i) {
-    snprintf (buf, sizeof buf, "%s/%s", dirs[i], prog);
-    if (access (buf, X_OK) == 0)
+  const char *pathc = getenv ("PATH");
+
+  if (!pathc)
+    return 0;
+
+  const char *elem;
+  char *saveptr;
+  size_t len = strlen (pathc) + 1;
+  char path[len];
+  strcpy (path, pathc);
+
+  elem = strtok_r (path, ":", &saveptr);
+  while (elem) {
+    size_t n = strlen (elem) + strlen (prog) + 2;
+    char testprog[n];
+
+    snprintf (testprog, n, "%s/%s", elem, prog);
+    if (access (testprog, X_OK) == 0)
       return 1;
+
+    elem = strtok_r (NULL, ":", &saveptr);
   }
+
+  /* Not found. */
   return 0;
 }
 
-- 
2.3.1

In data martedì 23 giugno 2015 14:34:18, Richard W.M. Jones ha
scritto:
> ---
>  daemon/guestfsd.c | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
> index 21b3600..c9cc8c5 100644
> --- a/daemon/guestfsd.c
> +++ b/daemon/guestfsd.c
> @@ -243,9 +243,6 @@ main (int argc, char *argv[])
>    /* Set up a basic environment.  After we are called by /init the
>     * environment is essentially empty.
>     * https://bugzilla.redhat.com/show_bug.cgi?id=502074#c5
> -   *
> -   * NOTE: if you change $PATH, you must also change 'prog_exists'
> -   * function below.
>     */
>    setenv ("PATH", "/sbin:/usr/sbin:/bin:/usr/bin", 1);
>    setenv ("SHELL", "/bin/sh", 1);
> @@ -1440,22 +1437,34 @@ mountable_to_string (const mountable_t *mountable)
>    }
>  }
>  
> -/* Check program exists and is executable on $PATH.  Actually, we
> - * just assume PATH contains the default entries (see main() above).
> - */
> +/* Check program exists and is executable on $PATH. */
>  int
>  prog_exists (const char *prog)
>  {
> -  static const char * const dirs[] > -    { "/sbin",
"/usr/sbin", "/bin", "/usr/bin" };
> -  size_t i;
> -  char buf[1024];
> -
> -  for (i = 0; i < sizeof dirs / sizeof dirs[0]; ++i) {
> -    snprintf (buf, sizeof buf, "%s/%s", dirs[i], prog);
> -    if (access (buf, X_OK) == 0)
> +  const char *pathc = getenv ("PATH");
> +
> +  if (!pathc)
> +    return 0;
> +
> +  const char *elem;
> +  char *saveptr;
> +  size_t len = strlen (pathc) + 1;
> +  char path[len];
> +  strcpy (path, pathc);
> +
> +  elem = strtok_r (path, ":", &saveptr);
> +  while (elem) {
> +    size_t n = strlen (elem) + strlen (prog) + 2;
> +    char testprog[n];
> +
> +    snprintf (testprog, n, "%s/%s", elem, prog);
> +    if (access (testprog, X_OK) == 0)
>        return 1;
> +
> +    elem = strtok_r (NULL, ":", &saveptr);
>    }
> +
> +  /* Not found. */
>    return 0;
>  }
LGTM.

The only minor note, "strlen (prog)" could be cached outside the while
loop.

-- 
Pino Toscano

On Tue, Jun 23, 2015 at 07:58:52PM +0200, Pino Toscano
wrote:
> In data martedì 23 giugno 2015 14:34:18, Richard W.M. Jones ha scritto:
> > ---
> >  daemon/guestfsd.c | 37 +++++++++++++++++++++++--------------
> >  1 file changed, 23 insertions(+), 14 deletions(-)
> > 
> > diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
> > index 21b3600..c9cc8c5 100644
> > --- a/daemon/guestfsd.c
> > +++ b/daemon/guestfsd.c
> > @@ -243,9 +243,6 @@ main (int argc, char *argv[])
> >    /* Set up a basic environment.  After we are called by /init the
> >     * environment is essentially empty.
> >     * https://bugzilla.redhat.com/show_bug.cgi?id=502074#c5
> > -   *
> > -   * NOTE: if you change $PATH, you must also change
'prog_exists'
> > -   * function below.
> >     */
> >    setenv ("PATH",
"/sbin:/usr/sbin:/bin:/usr/bin", 1);
> >    setenv ("SHELL", "/bin/sh", 1);
> > @@ -1440,22 +1437,34 @@ mountable_to_string (const mountable_t
*mountable)
> >    }
> >  }
> >  
> > -/* Check program exists and is executable on $PATH.  Actually, we
> > - * just assume PATH contains the default entries (see main() above).
> > - */
> > +/* Check program exists and is executable on $PATH. */
> >  int
> >  prog_exists (const char *prog)
> >  {
> > -  static const char * const dirs[] > > -    {
"/sbin", "/usr/sbin", "/bin", "/usr/bin"
};
> > -  size_t i;
> > -  char buf[1024];
> > -
> > -  for (i = 0; i < sizeof dirs / sizeof dirs[0]; ++i) {
> > -    snprintf (buf, sizeof buf, "%s/%s", dirs[i], prog);
> > -    if (access (buf, X_OK) == 0)
> > +  const char *pathc = getenv ("PATH");
> > +
> > +  if (!pathc)
> > +    return 0;
> > +
> > +  const char *elem;
> > +  char *saveptr;
> > +  size_t len = strlen (pathc) + 1;
> > +  char path[len];
> > +  strcpy (path, pathc);
> > +
> > +  elem = strtok_r (path, ":", &saveptr);
> > +  while (elem) {
> > +    size_t n = strlen (elem) + strlen (prog) + 2;
> > +    char testprog[n];
> > +
> > +    snprintf (testprog, n, "%s/%s", elem, prog);
> > +    if (access (testprog, X_OK) == 0)
> >        return 1;
> > +
> > +    elem = strtok_r (NULL, ":", &saveptr);
> >    }
> > +
> > +  /* Not found. */
> >    return 0;
> >  }
> 
> LGTM.
> 
> The only minor note, "strlen (prog)" could be cached outside the
while
> loop.
Yup.  Kind of hoping that gcc would do that, but I just checked the
assembler output at -O2 and as best I can tell it doesn't :-(

I will modify the source ..

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW