Re: [PATCH] tools/libxc: Fix error checking for xc_get_{cpu, node}map_size() callers

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 12/12/2013 23:59, Dario Faggioli wrote:
> On gio, 2013-12-12 at 21:05 +0000, Andrew Cooper wrote:
>> On 12/12/2013 14:56, Dario Faggioli wrote:
>>> Yep, I confirm that, after that changeset, neither
>>> xc_get_max_{cpus,nodes}() nor xc_get_{cpu,node}map_size() return 0 as an
>>> error anymore.
>> Zero might not be "the error condition" any more, but it is certainly an
>> error from any of these functions (and possible as
>> xc_get_max_{cpus,nodes}() is capable of returning 0 if Xen hands back -1
>> for physinfo.max_{cpu,node}_id)
>>
> Well, yes, but under what circumstances Xen would do such a thing? As
> far as I can see, max_node_id is just 'MAX_NUMNODES-1'. max_cpu_id is
> 'nr_cpu_ids-1', nr_cpu_ids is '__read_mostly nr_cpu_ids = NR_CPUS'.
>
> I may be wrong, but it looks to me that either both MAX_NUMNODES and
> NR_CPUS (and nr_cpu_ids+1 too, if it changes) are > 0, or the system
> would be experiencing way bigger issues than misdimensioning a bitmap.
>
> What I mean is, if we are there checking, we at least have one node and
> one cpu. In which case, either the call failed and returned <0, or it
> succeeded, and returned >0.
>
> What am I missing?

I didn't wish to imply that I expected Xen to return -1 for either
case.  Stuff would indeed be very broken if this were the case.

As the argument is over the difference between "< 0" and "<= 0",
defensive coding would have the "<= 0" check even if Xen is a trusted
source of information.

>
>> xc_{cpu/node}map_alloc() must strictly still be "<= 0" checks to avoid
>> the issue where calloc(1, 0) returns a non-NULL pointer.
>>
> Here `man calloc' says, among other things: "The memory is set to zero.
> If nmemb or size is 0, then calloc() returns either NULL, or a unique
> pointer value that can later be successfully passed to free()."
>
> Was it that what you were referring to?

Now I come to reconsider this, It wasn't quite the same situation as
libxl_list_vm().

However,

calloc(1, 0) (just like malloc(0) ) can give you a valid pointer to a
buffer you cannot use, and indeed glibc does give you a real buffer of
length 0.

This very dangerous, as traditional thinking says "if I have a non-null
pointer in my hands, its good".  As soon as you dereference this
pointer, you have undefined behaviour.

>From what I understand from comp.lang.c, the only reason this is in the
spec (rather than being a very strict "malloc(0) => NULL") is that
implementations at the time of standardisation already had this behaviour.

Whatever the reason for these quirks existing, they are best avoided
whenever possible.

>
>> Currently, I am of the opinion that the patch is better as is, than
>> changing some of the checks to being strictly "< 0"
>>
> Given the first part of this reply (if I'm not mistaken in there) I'd
> prefer the other way round. I.e., '< 0' whenever it makes sense and, if
> it's an actual issue, '<= 0' in xc_{cpu/node}map_alloc(), perhaps with a
> comment, saying that the '<=' is there to prevent calloc madness. :-)
>
> That being said, I'm happy with whatever solution a tool maintainer
> likes better.
>
> Regards,
> Dario
>

I too will end up deferring to a specific judgement from a tools
maintainer.  I am just taking this opportunity to justify why I chose
"<= 0" in all cases rather than "< 0" (which certainly did get considered).

~Andrew