Re: [Xen-devel] [PATCH]: fix crash in various tools by permitting xs_*() with NULL path

[Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>]

On Tue, 2010-07-20 at 17:27 +0100, Ian Jackson wrote:
> Gianni Tedesco writes ("[Xen-devel] [PATCH]: fix crash in various tools by 
> permitting xs_*() with NULL path"):
> > Many tools generate xenstore paths and then perform operations on those
> > paths without checking for NULL.
> 
> Do they ?  Those tools are buggy.

Yes, but for example libxl has around 300 calls to libxl_sprintf() which
are used un-checked as xenstore paths... We could either check every
result or check for ENOMEM in one place and abort() since NULL deref has
much the same effect.

> > While strictly this may be considered a bug in the tools it makes sense
> > to consider making these no-ops as a convenience measure.
> 
> I think this is fine for things like deletion but not otherwise.  So I
> think in the case of libxenstore only xs_rm should be modified like
> this.

I'm not so sure, I think NULL path should have some coherent meaning -
whether that's just to segfault or something else is another discussion.

> > @@ -503,6 +506,8 @@
> >  void *xs_read(struct xs_handle *h, xs_transaction_t t,
> >           const char *path, unsigned int *len)
> >  {
> > +    if ( NULL == path )
> > +        return NULL;
> >     return xs_single(h, t, XS_READ, path, len);
> >  }
> >  
> 
> This pattern is wrong.  Firstly, all functions in libxenstore set
> errno when returning errors and if you are going to return NULL you
> need to to do so as well.  Secondly, it is not appropriate to turn
> xs_read(,,NULL,) into an error.  It should crash.
> 
> Compare the C standard library.  If you call fprintf(NULL,...) it
> doesn't return EOF setting errno to EFAULT, it coredumps, and rightly
> so.

On the contrary open(NULL, O_RDONLY) will... The difference is FILE * is
a struct wheras the change I am proposing in this case is to treat NULL
as the empty string in the case of paths.

> Finally, to review this patch, it would be much more helpful if you
> used a diff tool which includes the function name in the diff output.
> Without that we have to apply the patch to a tree of our own and
> regenerate the diff.

good point

> > + *
> > + * path must be non-NULL
> 
> This should not be added here.  Competent C programmers will not
> expect to be able to pass NULL to things unless told they can.  So the
> assumption is the other way around.  You should add a note saying
> that NULL is permitted where it is.

OK

> Nacked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> 
> Sorry,
> Ian.

How about the following? If this is a total no-go then I'll fix the
callers.

Thanks for review

Signed-off-by: Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>

diff -r 108ee7b37ac4 tools/xenstore/xs.h
--- a/tools/xenstore/xs.h       Tue Jul 20 15:01:15 2010 +0100
+++ b/tools/xenstore/xs.h       Tue Jul 20 17:56:34 2010 +0100
@@ -34,6 +34,11 @@ typedef uint32_t xs_transaction_t;
 
 /* On failure, these routines set errno. */
 
+/* In general path may be NULL and represents the empty string,
+ * write type operations will always succeed and read type operations
+ * will always fail. The watch/unwatch operations will segfault.
+ */
+
 /* Connect to the xs daemon.
  * Returns a handle or NULL.
  */
diff -r 108ee7b37ac4 tools/xenstore/xs.c
--- a/tools/xenstore/xs.c       Tue Jul 20 15:01:15 2010 +0100
+++ b/tools/xenstore/xs.c       Tue Jul 20 17:56:34 2010 +0100
@@ -474,6 +474,11 @@ char **xs_directory(struct xs_handle *h,
        char *strings, *p, **ret;
        unsigned int len;
 
+    if ( NULL == path ) {
+        errno = ENOENT;
+        return NULL;
+    }
+
        strings = xs_single(h, t, XS_DIRECTORY, path, &len);
        if (!strings)
                return NULL;
@@ -503,6 +508,10 @@ char **xs_directory(struct xs_handle *h,
 void *xs_read(struct xs_handle *h, xs_transaction_t t,
              const char *path, unsigned int *len)
 {
+    if ( NULL == path ) {
+        errno = ENOENT;
+        return NULL;
+    }
        return xs_single(h, t, XS_READ, path, len);
 }
 
@@ -514,6 +523,9 @@ bool xs_write(struct xs_handle *h, xs_tr
 {
        struct iovec iovec[2];
 
+    if ( NULL == path )
+        return true;
+
        iovec[0].iov_base = (void *)path;
        iovec[0].iov_len = strlen(path) + 1;
        iovec[1].iov_base = (void *)data;
@@ -529,6 +541,8 @@ bool xs_write(struct xs_handle *h, xs_tr
 bool xs_mkdir(struct xs_handle *h, xs_transaction_t t,
              const char *path)
 {
+    if ( NULL == path )
+        return true;
        return xs_bool(xs_single(h, t, XS_MKDIR, path, NULL));
 }
 
@@ -538,6 +552,8 @@ bool xs_mkdir(struct xs_handle *h, xs_tr
 bool xs_rm(struct xs_handle *h, xs_transaction_t t,
           const char *path)
 {
+    if ( NULL == path )
+        return true;
        return xs_bool(xs_single(h, t, XS_RM, path, NULL));
 }
 
@@ -552,6 +568,11 @@ struct xs_permissions *xs_get_permission
        unsigned int len;
        struct xs_permissions *ret;
 
+    if ( NULL == path ) {
+        errno = ENOENT;
+        return NULL;
+    }
+
        strings = xs_single(h, t, XS_GET_PERMS, path, &len);
        if (!strings)
                return NULL;
@@ -587,6 +608,9 @@ bool xs_set_permissions(struct xs_handle
        unsigned int i;
        struct iovec iov[1+num_perms];
 
+    if ( NULL == path )
+        return true;
+
        iov[0].iov_base = (void *)path;
        iov[0].iov_len = strlen(path) + 1;
        



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel