Re: [Xen-devel] [PATCH 08/13 v5] libxl: don't leak ptr in libxl_list_vm error case [and 1 more messages]

[Matthew Daley <mattd@xxxxxxxxxxx>]

On Sat, Dec 14, 2013 at 12:22 PM, Matthew Daley <mattd@xxxxxxxxxxx> wrote:
> On Sat, Dec 14, 2013 at 5:52 AM, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> 
> wrote:
>> Ian Campbell writes ("Re: [PATCH 08/13 v5] libxl: don't leak ptr in 
>> libxl_list_vm error case"):
>>> On Tue, 2013-12-03 at 14:29 +1300, Matthew Daley wrote:
>>> > +    /*
>>> > +  * Always make sure to allocate at least one element; if we don't and we
>>> > +  * request zero, libxl__calloc (might) think its internal call to calloc
>>> > +  * has failed (if it returns null), if so it would kill our process.
>> [rewrapped -iwj]
>>>
>>> Is size==0 something we could/should handle in our libxl__*alloc
>>> wrappers?
>>
>> I think so.  I think they should promise that if you pass size==0 you
>> get a non-null pointer.  Calling realloc with size==0 should crash.
>>
>> Matthew Daley writes ("Re: [PATCH 08/13 v5] libxl: don't leak ptr in 
>> libxl_list_vm error case"):
>>> Ping?
>>
>> See Ian C's comment above, which AFAICT hasn't been answered.
>
> Sorry, I implicitly agreed with Andrew's comment (at least, the second 
> part...)
>
> For the record, for custom wrapper functions like these I usually
> prefer to stick as close to the existing functions' semantics as
> possible, not so much out of love for standards but out of desiring
> the absence of surprising behaviour.

Although, I suppose changing things wrt. the standards would just
introduce new slightly-surprising behaviours vs. having existing
really-surprising ones...

- Matthew

>
> As for the second part of the comment, as Andrew already mentioned,
> the nb_domains output argument is sufficient to distinguish failure
> vs. 0 domains from libxl_list_vm already.
>
> Out of curiosity, looking through libxl_list_vm's existing callers:
> - libxl__domain_make is only (ab)using libxl_list_vm to get the number
> of vms via nb_domains... it just frees the vm list right afterward!
> - xl_cmdimpl.c:print_uptime doesn't even check libxl_list_vm's result
> :D  I will patch this shortly.
>
> (Also, I see you have applied my patch regardless of this discussion; thanks.)
>
> - Matthew
>
>>
>> Thanks,
>> Ian.

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