WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH]: fix crash in various tools by permitting xs_*() with NULL path
From: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
Date: Tue, 20 Jul 2010 17:27:17 +0100
Cc: Xen Devel <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Tue, 20 Jul 2010 09:28:03 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1279641356.1723.1940.camel@xxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Newsgroups: chiark.mail.xen.devel
References: <1279641356.1723.1940.camel@xxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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