Xen devel - Dec 2013 - [PATCH] tools/libxc: Fix error checking for xc_get_{cpu, node}map_size() callers

c/s 2e82c18cd850592ae9a1f682eb93965a868b5f2f changed the error returns of
xc_get_{cpu,node}map_size() to now include returning -1.  This invalidated the
error checks from callers, which expected 0 to be the only error case.

Coverity ID: 1135907 1135908 1135909 1135910 1135911 1135912 1135913 1135914

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
 tools/libxc/xc_cpupool.c |    4 ++--
 tools/libxc/xc_domain.c  |    8 ++++----
 tools/libxc/xc_misc.c    |    4 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/libxc/xc_cpupool.c b/tools/libxc/xc_cpupool.c
index b5e0c46..c8c2a33 100644
--- a/tools/libxc/xc_cpupool.c
+++ b/tools/libxc/xc_cpupool.c
@@ -74,7 +74,7 @@ xc_cpupoolinfo_t *xc_cpupool_getinfo(xc_interface *xch,
     DECLARE_HYPERCALL_BUFFER(uint8_t, local);
 
     local_size = xc_get_cpumap_size(xch);
-    if (!local_size)
+    if (local_size <= 0)
     {
         PERROR("Could not get number of cpus");
         return NULL;
@@ -172,7 +172,7 @@ xc_cpumap_t xc_cpupool_freeinfo(xc_interface *xch)
     DECLARE_HYPERCALL_BUFFER(uint8_t, local);
 
     mapsize = xc_get_cpumap_size(xch);
-    if (mapsize == 0)
+    if (mapsize <= 0)
         return NULL;
 
     local = xc_hypercall_buffer_alloc(xch, local, mapsize);
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 1ccafc5..c2fdd74 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -122,7 +122,7 @@ int xc_domain_node_setaffinity(xc_interface *xch,
     int nodesize;
 
     nodesize = xc_get_nodemap_size(xch);
-    if (!nodesize)
+    if (nodesize <= 0)
     {
         PERROR("Could not get number of nodes");
         goto out;
@@ -160,7 +160,7 @@ int xc_domain_node_getaffinity(xc_interface *xch,
     int nodesize;
 
     nodesize = xc_get_nodemap_size(xch);
-    if (!nodesize)
+    if (nodesize <= 0)
     {
         PERROR("Could not get number of nodes");
         goto out;
@@ -200,7 +200,7 @@ int xc_vcpu_setaffinity(xc_interface *xch,
     int cpusize;
 
     cpusize = xc_get_cpumap_size(xch);
-    if (!cpusize)
+    if (cpusize <= 0)
     {
         PERROR("Could not get number of cpus");
         goto out;
@@ -243,7 +243,7 @@ int xc_vcpu_getaffinity(xc_interface *xch,
     int cpusize;
 
     cpusize = xc_get_cpumap_size(xch);
-    if (!cpusize)
+    if (cpusize <= 0)
     {
         PERROR("Could not get number of cpus");
         goto out;
diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index 00cd0d8..3303454 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -88,7 +88,7 @@ xc_cpumap_t xc_cpumap_alloc(xc_interface *xch)
     int sz;
 
     sz = xc_get_cpumap_size(xch);
-    if (sz == 0)
+    if (sz <= 0)
         return NULL;
     return calloc(1, sz);
 }
@@ -98,7 +98,7 @@ xc_nodemap_t xc_nodemap_alloc(xc_interface *xch)
     int sz;
 
     sz = xc_get_nodemap_size(xch);
-    if (sz == 0)
+    if (sz <= 0)
         return NULL;
     return calloc(1, sz);
 }
-- 
1.7.10.4

On Wed, 2013-12-11 at 15:47 +0000, Andrew Cooper wrote:
> c/s 2e82c18cd850592ae9a1f682eb93965a868b5f2f changed the error returns of
> xc_get_{cpu,node}map_size() to now include returning -1.  This invalidated
the
> error checks from callers, which expected 0 to be the only error case.
I don''t think 0 is a valid error value any more. Neither
xc_get_max_cpus
nor xc_get_max_nodes can return 0 and the map_size functions will round
to 1 or more.

So these could all be "< 0" tests think.

On gio, 2013-12-12 at 14:24 +0000, Ian Campbell wrote:
> On Wed, 2013-12-11 at 15:47 +0000, Andrew Cooper wrote:
> > c/s 2e82c18cd850592ae9a1f682eb93965a868b5f2f changed the error returns
of
> > xc_get_{cpu,node}map_size() to now include returning -1.  This
invalidated the
> > error checks from callers, which expected 0 to be the only error case.
> 
> I don''t think 0 is a valid error value any more. Neither
xc_get_max_cpus
> nor xc_get_max_nodes can return 0 and the map_size functions will round
> to 1 or more.
> 
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.
> So these could all be "< 0" tests think.
> 
Indeed.

Anyway, looks like, while I fixed the callers of the xx_get_max_xx
things in that very comment, I didn''t do the same for the
xx_get_*map_xx
ones. Weird, as ISTR doing so too... :-/

Anyway, thanks to Coverity for caching this and to Andrew for the patch.

I''ll reply to v2 (if you''re posting it) but, with the
conditions
converted to "< 0", this can have my:

Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)



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

On 12/12/2013 14:56, Dario Faggioli wrote:
> On gio, 2013-12-12 at 14:24 +0000, Ian Campbell wrote:
>> On Wed, 2013-12-11 at 15:47 +0000, Andrew Cooper wrote:
>>> c/s 2e82c18cd850592ae9a1f682eb93965a868b5f2f changed the error
returns of
>>> xc_get_{cpu,node}map_size() to now include returning -1.  This
invalidated the
>>> error checks from callers, which expected 0 to be the only error
case.
>> I don''t think 0 is a valid error value any more. Neither
xc_get_max_cpus
>> nor xc_get_max_nodes can return 0 and the map_size functions will round
>> to 1 or more.
>>
> 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)
>
>> So these could all be "< 0" tests think.
>>
> Indeed.
>
> Anyway, looks like, while I fixed the callers of the xx_get_max_xx
> things in that very comment, I didn''t do the same for the
xx_get_*map_xx
> ones. Weird, as ISTR doing so too... :-/
>
> Anyway, thanks to Coverity for caching this and to Andrew for the patch.
>
> I''ll reply to v2 (if you''re posting it) but, with the
conditions
> converted to "< 0", this can have my:
>
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
>
> Regards,
> Dario
>
xc_{cpu/node}map_alloc() must strictly still be "<= 0" checks to
avoid
the issue where calloc(1, 0) returns a non-NULL pointer.


Currently, I am of the opinion that the patch is better as is, than
changing some of the checks to being strictly "< 0"

~Andrew

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

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)



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

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

On ven, 2013-12-13 at 00:35 +0000, Andrew Cooper wrote:
> On 12/12/2013 23:59, Dario Faggioli wrote:
> > 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.
> 
Right..
> 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.
> 
Indeed. On the other hand, having the "<=" could confuse people
looking
at the code, tricking them into thinking "why the
''<=''? How is it
possible for this to be ''=0''?". One could stick comments
there, but it
would probably make it too chatty.

Anyway, especially after all this, I''m really fine with either. Even if
''arguing'' is the real big fun in Open Source, I guess
it''s the time to
let this go, and hear what the "bosses" want and do that. :-P
> 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.
> 
Agreed.
> 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.
> 
Yeah... In spite of anyone that thinks standards should promote best
practises, rather than blessing already existing and bad ones! :-P
> Whatever the reason for these quirks existing, they are best avoided
> whenever possible.
> 
Sure!
> 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).
> 
Right. Thanks for the explanation, and for the arguing. :-)

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)



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