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

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

[Gianni Tedesco]

Many tools generate xenstore paths and then perform operations on those
paths without checking for NULL. The problem with this is that xs_single
and xs_talkv use iovecs where len is set to strlen(NULL) + 1 leading to
a deref.

While strictly this may be considered a bug in the tools it makes sense
to consider making these no-ops as a convenience measure.

If the iov_len for NULL is set to 0 then this causes xenstored not to
respond and for the client to hang indefinitely. For this reason the
entry to each affected library function is modified to check for NULL.

I have left xs_watch and xs_unwatch as before since there is no
reasonable no-op implementation that I can think of.

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

 xenstore/xs.c  |   18 ++++++++++++++++++
 xenstore/xs.h  |    4 ++++
 3 files changed, 23 insertions(+), 1 deletion(-)


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 16:44:43 2010 +0100
@@ -474,6 +474,9 @@
 	char *strings, *p, **ret;
 	unsigned int len;
 
+    if ( NULL == path )
+        return NULL;
+
 	strings = xs_single(h, t, XS_DIRECTORY, path, &len);
 	if (!strings)
 		return NULL;
@@ -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);
 }
 
@@ -514,6 +519,9 @@
 {
 	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 +537,8 @@
 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 +548,8 @@
 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 +564,9 @@
 	unsigned int len;
 	struct xs_permissions *ret;
 
+    if ( NULL == path )
+        return NULL;
+
 	strings = xs_single(h, t, XS_GET_PERMS, path, &len);
 	if (!strings)
 		return NULL;
@@ -587,6 +602,9 @@
 	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;
 	
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 16:44:43 2010 +0100
@@ -110,6 +110,8 @@
  * When the node (or any child) changes, fd will become readable.
  * Token is returned when watch is read, to allow matching.
  * Returns false on failure.
+ *
+ * path must be non-NULL
  */
 bool xs_watch(struct xs_handle *h, const char *path, const char *token);
 
@@ -124,6 +126,8 @@
 
 /* Remove a watch on a node: implicitly acks any outstanding watch.
  * Returns false on failure (no watch on that node).
+ *
+ * path must be non-NULL
  */
 bool xs_unwatch(struct xs_handle *h, const char *path, const char *token);

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

[Ian Jackson]

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.

> 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.

> @@ -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.

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.

> + *
> + * 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.

Nacked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Sorry,
Ian.

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

[Gianni Tedesco]

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@eu.citrix.com>
> 
> 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@citrix.com>

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;
 	

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

[Paolo Bonzini]

On 07/20/2010 07:02 PM, Gianni Tedesco wrote:
> 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.

Returning EFAULT is one possible outcome, but a SIGSEGV or SIGBUS is 
also valid according to POSIX.

Paolo

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

[Gianni Tedesco]

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.
> 
> > 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 may be defeating my own argument but here is an alternative (let's
forget strdup() out of memory case and let it crash and burn):

8<---------------------
Fix segfault in xl destroy when xenstore has been fowled up

libxl_dirname() returns NULL if a string is passed to it which doesn't
look like a xenstore key. During xl destroy libxl_dirname is used to
generate an xs path in order to remove device backend keys. If a
nonsense key has been put in there resulting in NULL backend path this
will crash xl.

diff -r 1eea12f66951 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c	Tue Jul 20 17:14:43 2010 +0100
+++ b/tools/libxl/libxl_device.c	Tue Jul 20 18:05:19 2010 +0100
@@ -344,7 +344,8 @@ int libxl_devices_destroy(struct libxl_c
     }
     for (i = 0; i < n; i++) {
         flexarray_get(toremove, i, (void**) &path);
-        xs_rm(clone.xsh, XBT_NULL, path);
+        if ( path )
+            xs_rm(clone.xsh, XBT_NULL, path);
     }
     flexarray_free(toremove);
     libxl_ctx_free(&clone);

Re: [PATCH]: fix segfault in xl destroy when xenstore has been fowled up

[Gianni Tedesco]

Oops, forgot signed off by and change subject

8<---------------------
Fix segfault in xl destroy when xenstore has been fowled up

libxl_dirname() returns NULL if a string is passed to it which doesn't
look like a xenstore key. During xl destroy libxl_dirname is used to
generate an xs path in order to remove device backend keys. If a
nonsense key has been put in there resulting in NULL backend path this
will crash xl.

Signed-off-by: <gianni.tedesco@citrix.com>

diff -r 1eea12f66951 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c	Tue Jul 20 17:14:43 2010 +0100
+++ b/tools/libxl/libxl_device.c	Tue Jul 20 18:05:19 2010 +0100
@@ -344,7 +344,8 @@ int libxl_devices_destroy(struct libxl_c
     }
     for (i = 0; i < n; i++) {
         flexarray_get(toremove, i, (void**) &path);
-        xs_rm(clone.xsh, XBT_NULL, path);
+        if ( path )
+            xs_rm(clone.xsh, XBT_NULL, path);
     }
     flexarray_free(toremove);
     libxl_ctx_free(&clone);

Re: [PATCH]: fix segfault in xl destroy when xenstore has been fowled up

[Stefano Stabellini]

On Tue, 20 Jul 2010, Gianni Tedesco (3P) wrote:
> Oops, forgot signed off by and change subject
> 
> 8<---------------------
> Fix segfault in xl destroy when xenstore has been fowled up
> 
> libxl_dirname() returns NULL if a string is passed to it which doesn't
> look like a xenstore key. During xl destroy libxl_dirname is used to
> generate an xs path in order to remove device backend keys. If a
> nonsense key has been put in there resulting in NULL backend path this
> will crash xl.
> 
> Signed-off-by: <gianni.tedesco@citrix.com>
> 
> diff -r 1eea12f66951 tools/libxl/libxl_device.c
> --- a/tools/libxl/libxl_device.c	Tue Jul 20 17:14:43 2010 +0100
> +++ b/tools/libxl/libxl_device.c	Tue Jul 20 18:05:19 2010 +0100
> @@ -344,7 +344,8 @@ int libxl_devices_destroy(struct libxl_c
>      }
>      for (i = 0; i < n; i++) {
>          flexarray_get(toremove, i, (void**) &path);
> -        xs_rm(clone.xsh, XBT_NULL, path);
> +        if ( path )
> +            xs_rm(clone.xsh, XBT_NULL, path);
>      }
>      flexarray_free(toremove);
>      libxl_ctx_free(&clone);
> 
 
I prefer the other approach.

Re: [PATCH]: fix segfault in xl destroy when xenstore has been fowled up

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH]: fix segfault in xl destroy when xenstore has been fowled up"):
> I prefer the other approach.

Yes, for xs_rm I agree.

Ian.

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

[Ian Campbell]

On Tue, 2010-07-20 at 17:27 +0100, Ian Jackson wrote:
> 
> 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.

The lack of this has annoyed me about mercurial for some time and this
message provoked me into looking again at how to make it work. So for
the benefit of others:

"hg diff -p" seems to work (this option didn't exist last time I looked,
which I admit was years ago) and adding 
        [diff]
        showfunc = True
to ~/.hgrc also works and seems to extend to "hg email" and "hg
tip" (which do not take a -p) as well so is probably sufficient for most
usecases.

Hurrah.

Ian.