[PATCH for-4.10] libxl: handle NULL in libxl__enum_from_string

[PATCH for-4.10] libxl: handle NULL in libxl__enum_from_string

[Wei Liu]

Discovered by Coverity.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl_utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 507ee56c7c..9433693e72 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -1017,7 +1017,7 @@ int libxl_get_max_nodes(libxl_ctx *ctx)
 int libxl__enum_from_string(const libxl_enum_string_table *t,
                             const char *s, int *e)
 {
-    if (!t) return ERROR_INVAL;
+    if (!t || !s) return ERROR_INVAL;
 
     for( ; t->s; t++) {
         if (!strcasecmp(t->s, s)) {
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.10] libxl: handle NULL in libxl__enum_from_string

[Ian Jackson]

Wei Liu writes ("[PATCH for-4.10] libxl: handle NULL in libxl__enum_from_string"):
> Discovered by Coverity.

But.  Surely it is very wrong

> @@ -1017,7 +1017,7 @@ int libxl_get_max_nodes(libxl_ctx *ctx)
>  int libxl__enum_from_string(const libxl_enum_string_table *t,
>                              const char *s, int *e)
>  {
> -    if (!t) return ERROR_INVAL;
> +    if (!t || !s) return ERROR_INVAL;

to call this function with s==NULL.

I'm not generally in favour of turning such mistakes from
easy-to-debug crashes into hard-to-track-down error codes (especially
with our nonspecific error codes).

Where does that occur ?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.10] libxl: handle NULL in libxl__enum_from_string

[Wei Liu]

On Fri, Oct 13, 2017 at 01:46:57PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH for-4.10] libxl: handle NULL in libxl__enum_from_string"):
> > Discovered by Coverity.
> 
> But.  Surely it is very wrong
> 
> > @@ -1017,7 +1017,7 @@ int libxl_get_max_nodes(libxl_ctx *ctx)
> >  int libxl__enum_from_string(const libxl_enum_string_table *t,
> >                              const char *s, int *e)
> >  {
> > -    if (!t) return ERROR_INVAL;
> > +    if (!t || !s) return ERROR_INVAL;
> 
> to call this function with s==NULL.
> 
> I'm not generally in favour of turning such mistakes from
> easy-to-debug crashes into hard-to-track-down error codes (especially
> with our nonspecific error codes).
> 
> Where does that occur ?
> 

libxl_*_type_from_string.

I agree they shouldn't be called with NULL. We should guard against
error (here or the libxl_*_type_from_string) or annotate the input can't
be NULL.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.10] libxl: handle NULL in libxl__enum_from_string

[Ian Jackson]

Wei Liu writes ("Re: [PATCH for-4.10] libxl: handle NULL in libxl__enum_from_string"):
> I agree they shouldn't be called with NULL. We should guard against
> error (here or the libxl_*_type_from_string) or annotate the input can't
> be NULL.

I mean, who calls any  libxl_*_from_string  with s==NULL ?

Also I don't think we should annotate that the input value can't be
NULL, especially in a situation like this where the semantics could
only be "this is wrong".  The API (and the internal calling
conventions) are full of functions taking pointer arguments - are we
going to annotate each one of those to say that it cannot be NULL ?

Instead, what we have actually done so far, is annotate when a pointer
parameter *may* be NULL, and, in that case, what that means:

 * If ao_how==NULL, the function will be synchronous.
 * If ao_how->callback==NULL, a libxl_event will be generated which
  /* if old_name is NULL, any old name is OK; otherwise we check
/* May be called with info_r == NULL to check for domain's existence.
   * event_occurs may be NULL if mask is 0.
   * disaster may be NULL.  If it is, or if _register_callbacks has
 * The application may call beforepoll with fds==NULL and
_hidden void libxl__ptr_add(libxl__gc *gc_opt, void *ptr /* may be * NULL */) NN1;
_hidden char *libxl__strdup(libxl__gc *gc_opt,
                            const char *c /* may be NULL */) NN1;

etc. etc.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.10] libxl: handle NULL in libxl__enum_from_string

[Andrew Cooper]

On 13/10/17 14:01, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH for-4.10] libxl: handle NULL in libxl__enum_from_string"):
>> I agree they shouldn't be called with NULL. We should guard against
>> error (here or the libxl_*_type_from_string) or annotate the input can't
>> be NULL.
> I mean, who calls any  libxl_*_from_string  with s==NULL ?
>
> Also I don't think we should annotate that the input value can't be
> NULL, especially in a situation like this where the semantics could
> only be "this is wrong".  The API (and the internal calling
> conventions) are full of functions taking pointer arguments - are we
> going to annotate each one of those to say that it cannot be NULL ?
>
> Instead, what we have actually done so far, is annotate when a pointer
> parameter *may* be NULL, and, in that case, what that means:

This is exactly what attribute nonnull exists for.  As a bonus, using
the attribute will have the compiler complain at you if it spots a way
NULL gets passed, and UBSAN will add specific instrumentation to check.

Alternatively, you could assert(s) which would catch all (ab)uses and
also quiesce Coverity.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.10] libxl: handle NULL in libxl__enum_from_string

[Ian Jackson]

Andrew Cooper writes ("Re: [Xen-devel] [PATCH for-4.10] libxl: handle NULL in libxl__enum_from_string"):
> On 13/10/17 14:01, Ian Jackson wrote:
> > Instead, what we have actually done so far, is annotate when a pointer
> > parameter *may* be NULL, and, in that case, what that means:
> 
> This is exactly what attribute nonnull exists for.  As a bonus, using
> the attribute will have the compiler complain at you if it spots a way
> NULL gets passed, and UBSAN will add specific instrumentation to check.

Thanks for that excellent suggestion, which I ought to have thought of
myself.

I'd be quite happy with patches to add the nonnull attribute to the
parameters.  We already have that for a number of the *alloc*
functions - git-grep libxl for "NN1".

I don't mind the idea of adding that to some more functions now, even
if we don't have complete coverage.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel