| Message ID | 20171013111938.2749-1-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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.
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
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.
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)) {
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(-)