[PATCH] libxl: handle null lists in libxl_string_list_length

[PATCH] libxl: handle null lists in libxl_string_list_length

[Matthew Daley]

After commit b0be2b12 ("libxl: fix libxl_string_list_length and its only
caller") libxl_string_list_length no longer handles null (empty) lists. Fix
so they are handled, returning length 0.

While at it, remove the unneccessary undereferenced null pointer check
and tidy the layout of the function.

Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
I've verified that this fixes the no-bootloader-arguments case.

 tools/libxl/libxl.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index eeaaee8..058bef2 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -200,9 +200,12 @@ void libxl_string_list_dispose(libxl_string_list *psl)
 
 int libxl_string_list_length(const libxl_string_list *psl)
 {
-    if (!psl) return 0;
     int i = 0;
-    while ((*psl)[i]) i++;
+
+    if (*psl)
+        while ((*psl)[i])
+            i++;
+
     return i;
 }
 
-- 
1.7.10.4

Re: [PATCH] libxl: handle null lists in libxl_string_list_length

[Ian Campbell]

On Fri, 2013-09-27 at 23:29 +1200, Matthew Daley wrote:
> After commit b0be2b12 ("libxl: fix libxl_string_list_length and its only
> caller") libxl_string_list_length no longer handles null (empty) lists. Fix
> so they are handled, returning length 0.
> 
> While at it, remove the unneccessary undereferenced null pointer check
> and tidy the layout of the function.
> 
> Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Matthew Daley <mattjd@gmail.com>

acked + applied, thanks.

Ian.

Re: [PATCH] libxl: handle null lists in libxl_string_list_length

[Boris Ostrovsky]

----- mattjd@gmail.com wrote:

> After commit b0be2b12 ("libxl: fix libxl_string_list_length and its
> only
> caller") libxl_string_list_length no longer handles null (empty)
> lists. Fix
> so they are handled, returning length 0.
> 
> While at it, remove the unneccessary undereferenced null pointer
> check

Are you sure this check should be removed? This routine can be called
from anywhere (at least within libxl it seems) and one day someone will
call it with NULL argument.

I'd probably leave this check in.

-boris


> and tidy the layout of the function.
> 
> Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Matthew Daley <mattjd@gmail.com>
> ---
> I've verified that this fixes the no-bootloader-arguments case.
> 
>  tools/libxl/libxl.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index eeaaee8..058bef2 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -200,9 +200,12 @@ void libxl_string_list_dispose(libxl_string_list
> *psl)
>  
>  int libxl_string_list_length(const libxl_string_list *psl)
>  {
> -    if (!psl) return 0;
>      int i = 0;
> -    while ((*psl)[i]) i++;
> +
> +    if (*psl)
> +        while ((*psl)[i])
> +            i++;
> +
>      return i;
>  }
>  
> -- 
> 1.7.10.4

Re: [PATCH] libxl: handle null lists in libxl_string_list_length

[Matthew Daley]

On Sat, Sep 28, 2013 at 12:08 AM, Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
>
> ----- mattjd@gmail.com wrote:
>
>> After commit b0be2b12 ("libxl: fix libxl_string_list_length and its
>> only
>> caller") libxl_string_list_length no longer handles null (empty)
>> lists. Fix
>> so they are handled, returning length 0.
>>
>> While at it, remove the unneccessary undereferenced null pointer
>> check
>
> Are you sure this check should be removed? This routine can be called
> from anywhere (at least within libxl it seems) and one day someone will
> call it with NULL argument.
>
> I'd probably leave this check in.

I would argue that any such invocation would be an error by the caller
and should fail noisily, similar to how passing NULL into strlen
should not return 0. libxl_{string,key_value}_list_dispose similarly
assumes non-NULL pointers, FWIW.

Ian C., do you have an opinion either way?

- Matthew

Re: [PATCH] libxl: handle null lists in libxl_string_list_length

[Andrew Cooper]

On 27/09/13 13:20, Matthew Daley wrote:
> On Sat, Sep 28, 2013 at 12:08 AM, Boris Ostrovsky
> <boris.ostrovsky@oracle.com> wrote:
>> ----- mattjd@gmail.com wrote:
>>
>>> After commit b0be2b12 ("libxl: fix libxl_string_list_length and its
>>> only
>>> caller") libxl_string_list_length no longer handles null (empty)
>>> lists. Fix
>>> so they are handled, returning length 0.
>>>
>>> While at it, remove the unneccessary undereferenced null pointer
>>> check
>> Are you sure this check should be removed? This routine can be called
>> from anywhere (at least within libxl it seems) and one day someone will
>> call it with NULL argument.
>>
>> I'd probably leave this check in.
> I would argue that any such invocation would be an error by the caller
> and should fail noisily, similar to how passing NULL into strlen
> should not return 0. libxl_{string,key_value}_list_dispose similarly
> assumes non-NULL pointers, FWIW.
>
> Ian C., do you have an opinion either way?
>
> - Matthew

I would agree that any passing of NULL is a caller error.  Possibly an
explicit check and abort()? If it is going to be noisy, we should be
nice and help out the debugger.

~Andrew

Re: [PATCH] libxl: handle null lists in libxl_string_list_length

[Ian Campbell]

On Sat, 2013-09-28 at 00:20 +1200, Matthew Daley wrote:
> On Sat, Sep 28, 2013 at 12:08 AM, Boris Ostrovsky
> <boris.ostrovsky@oracle.com> wrote:
> >
> > ----- mattjd@gmail.com wrote:
> >
> >> After commit b0be2b12 ("libxl: fix libxl_string_list_length and its
> >> only
> >> caller") libxl_string_list_length no longer handles null (empty)
> >> lists. Fix
> >> so they are handled, returning length 0.
> >>
> >> While at it, remove the unneccessary undereferenced null pointer
> >> check
> >
> > Are you sure this check should be removed? This routine can be called
> > from anywhere (at least within libxl it seems) and one day someone will
> > call it with NULL argument.
> >
> > I'd probably leave this check in.
> 
> I would argue that any such invocation would be an error by the caller
> and should fail noisily, similar to how passing NULL into strlen
> should not return 0. libxl_{string,key_value}_list_dispose similarly
> assumes non-NULL pointers, FWIW.
> 
> Ian C., do you have an opinion either way?

I think a zero length list is a bit different to a NULL string and
should return 0.

But libxl_string_list is already char** so this function is taking
char***. The check for char *** == NULL, which is being removed, appears
to be unnecessary. A zero length list would be char ** == NULL, which
should be handled (and is I think?). char * == NULL would be a "" entry
in the string list.

Confused? I know I am ;-)

Ian.

Re: [PATCH] libxl: handle null lists in libxl_string_list_length

[Matthew Daley]

On Sat, Sep 28, 2013 at 12:28 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Sat, 2013-09-28 at 00:20 +1200, Matthew Daley wrote:
>> On Sat, Sep 28, 2013 at 12:08 AM, Boris Ostrovsky
>> <boris.ostrovsky@oracle.com> wrote:
>> >
>> > ----- mattjd@gmail.com wrote:
>> >
>> >> After commit b0be2b12 ("libxl: fix libxl_string_list_length and its
>> >> only
>> >> caller") libxl_string_list_length no longer handles null (empty)
>> >> lists. Fix
>> >> so they are handled, returning length 0.
>> >>
>> >> While at it, remove the unneccessary undereferenced null pointer
>> >> check
>> >
>> > Are you sure this check should be removed? This routine can be called
>> > from anywhere (at least within libxl it seems) and one day someone will
>> > call it with NULL argument.
>> >
>> > I'd probably leave this check in.
>>
>> I would argue that any such invocation would be an error by the caller
>> and should fail noisily, similar to how passing NULL into strlen
>> should not return 0. libxl_{string,key_value}_list_dispose similarly
>> assumes non-NULL pointers, FWIW.
>>
>> Ian C., do you have an opinion either way?
>
> I think a zero length list is a bit different to a NULL string and
> should return 0.

Perhaps it was a bad analogy, but passing NULL to this function isn't
giving it an empty list, it's giving it no (NULL!) list. We don't
check for null pointers everywhere else non-optional pointers are
passed (at least, we shouldn't be, IMO...)

>
> But libxl_string_list is already char** so this function is taking
> char***. The check for char *** == NULL, which is being removed, appears
> to be unnecessary. A zero length list would be char ** == NULL, which
> should be handled (and is I think?). char * == NULL would be a "" entry
> in the string list.

This was my intention in this patch; only the char *** == NULL check
is removed, and the char ** == NULL for empty lists is handled by the
newly added if condition.

But char * == NULL doesn't mean an "" entry, doesn't it instead mark
the end of the list (see xlu_cfg_get_list_as_string_list for example)?
This is currently being checked for in the while loop condition. To
continue using your notation, instead char == NULL is an empty string
value in the list.

>
> Confused? I know I am ;-)

:)

- Matthew

>
> Ian.
>

Re: [PATCH] libxl: handle null lists in libxl_string_list_length

[Boris Ostrovsky]

On 09/27/2013 09:14 AM, Matthew Daley wrote:
> On Sat, Sep 28, 2013 at 12:28 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> On Sat, 2013-09-28 at 00:20 +1200, Matthew Daley wrote:
>>> On Sat, Sep 28, 2013 at 12:08 AM, Boris Ostrovsky
>>> <boris.ostrovsky@oracle.com> wrote:
>>>> ----- mattjd@gmail.com wrote:
>>>>
>>>>> After commit b0be2b12 ("libxl: fix libxl_string_list_length and its
>>>>> only
>>>>> caller") libxl_string_list_length no longer handles null (empty)
>>>>> lists. Fix
>>>>> so they are handled, returning length 0.
>>>>>
>>>>> While at it, remove the unneccessary undereferenced null pointer
>>>>> check
>>>> Are you sure this check should be removed? This routine can be called
>>>> from anywhere (at least within libxl it seems) and one day someone will
>>>> call it with NULL argument.
>>>>
>>>> I'd probably leave this check in.
>>> I would argue that any such invocation would be an error by the caller
>>> and should fail noisily, similar to how passing NULL into strlen
>>> should not return 0. libxl_{string,key_value}_list_dispose similarly
>>> assumes non-NULL pointers, FWIW.
>>>
>>> Ian C., do you have an opinion either way?
>> I think a zero length list is a bit different to a NULL string and
>> should return 0.
> Perhaps it was a bad analogy, but passing NULL to this function isn't
> giving it an empty list, it's giving it no (NULL!) list. We don't
> check for null pointers everywhere else non-optional pointers are
> passed (at least, we shouldn't be, IMO...)

What if someone assigns 'libxl_string_list *psl = NULL' if, say,
main()'s argc is 1 (i.e. there is no argument list) and then, later, calls
libxl_string_list_length(psl) to find out whether something needs
to be allocated for the list. Isn't getting a zero back an expected
answer?

(I am afraid we are approaching rathole territory here.)

-boris

>
>> But libxl_string_list is already char** so this function is taking
>> char***. The check for char *** == NULL, which is being removed, appears
>> to be unnecessary. A zero length list would be char ** == NULL, which
>> should be handled (and is I think?). char * == NULL would be a "" entry
>> in the string list.
> This was my intention in this patch; only the char *** == NULL check
> is removed, and the char ** == NULL for empty lists is handled by the
> newly added if condition.
>
> But char * == NULL doesn't mean an "" entry, doesn't it instead mark
> the end of the list (see xlu_cfg_get_list_as_string_list for example)?
> This is currently being checked for in the while loop condition. To
> continue using your notation, instead char == NULL is an empty string
> value in the list.
>
>> Confused? I know I am ;-)
> :)
>
> - Matthew
>
>> Ian.
>>

Re: [PATCH] libxl: handle null lists in libxl_string_list_length

[Ian Campbell]

On Fri, 2013-09-27 at 09:28 -0400, Boris Ostrovsky wrote:
> On 09/27/2013 09:14 AM, Matthew Daley wrote:
> > On Sat, Sep 28, 2013 at 12:28 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> >> On Sat, 2013-09-28 at 00:20 +1200, Matthew Daley wrote:
> >>> On Sat, Sep 28, 2013 at 12:08 AM, Boris Ostrovsky
> >>> <boris.ostrovsky@oracle.com> wrote:
> >>>> ----- mattjd@gmail.com wrote:
> >>>>
> >>>>> After commit b0be2b12 ("libxl: fix libxl_string_list_length and its
> >>>>> only
> >>>>> caller") libxl_string_list_length no longer handles null (empty)
> >>>>> lists. Fix
> >>>>> so they are handled, returning length 0.
> >>>>>
> >>>>> While at it, remove the unneccessary undereferenced null pointer
> >>>>> check
> >>>> Are you sure this check should be removed? This routine can be called
> >>>> from anywhere (at least within libxl it seems) and one day someone will
> >>>> call it with NULL argument.
> >>>>
> >>>> I'd probably leave this check in.
> >>> I would argue that any such invocation would be an error by the caller
> >>> and should fail noisily, similar to how passing NULL into strlen
> >>> should not return 0. libxl_{string,key_value}_list_dispose similarly
> >>> assumes non-NULL pointers, FWIW.
> >>>
> >>> Ian C., do you have an opinion either way?
> >> I think a zero length list is a bit different to a NULL string and
> >> should return 0.
> > Perhaps it was a bad analogy, but passing NULL to this function isn't
> > giving it an empty list, it's giving it no (NULL!) list. We don't
> > check for null pointers everywhere else non-optional pointers are
> > passed (at least, we shouldn't be, IMO...)
> 
> What if someone assigns 'libxl_string_list *psl = NULL' if, say,
> main()'s argc is 1 (i.e. there is no argument list) and then, later, calls
> libxl_string_list_length(psl) to find out whether something needs
> to be allocated for the list. Isn't getting a zero back an expected
> answer?

This is the distinction Matthew are I were trying to make. In the case
you describe you would do libxl_string_list psl = NULL (a zero length
argument list) and call libxl_string_list_length(&psl).

libxl_string_list_length(NULL) is not asking for the length of a
zero-length, list, it's asking for the length of no-list at all. If this
were floating point the answer would be NaN ;-) Instead we get a
segfault...

Ian.