Xen devel - Sep 2013 - [PATCH] libxl: handle null lists in libxl_string_list_length

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

----- 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

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

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

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.

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.
>

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.
>>

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.

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.