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