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@xxxxxxxxxxxxx>
Sorry,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home