[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH]: fix crash in various tools by permitting xs_*() with NULL path
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |