[PATCH] libxc/pm: Fix NULL pointer checks.

[PATCH] libxc/pm: Fix NULL pointer checks.

[Andrew Cooper]

Discovered by Coverity,
CIDs 1054968 1054969 1054970 1054971 1054972 1054973 10549704

This was broken by c/s 5cc436c1d2b3b0 which did a blanket change of 'int
xc_handle' -> 'xc_interface *xch'.  The types got updated, but error
conditions were left as-were.  (I suspect some sed was involved originally)

Also while playing around in this area, fix up some of the bracketing style to
match the Xen coding style.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxc/xc_pm.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/libxc/xc_pm.c b/tools/libxc/xc_pm.c
index fa9b246..ea1e251 100644
--- a/tools/libxc/xc_pm.c
+++ b/tools/libxc/xc_pm.c
@@ -285,7 +285,7 @@ int xc_set_cpufreq_gov(xc_interface *xch, int cpuid, char *govname)
     DECLARE_SYSCTL;
     char *scaling_governor = sysctl.u.pm_op.u.set_gov.scaling_governor;
 
-    if ( (xch < 0) || (!govname) )
+    if ( !xch || !govname )
         return -EINVAL;
 
     sysctl.cmd = XEN_SYSCTL_pm_op;
@@ -302,7 +302,7 @@ int xc_set_cpufreq_para(xc_interface *xch, int cpuid,
 {
     DECLARE_SYSCTL;
 
-    if ( xch < 0 )
+    if ( !xch )
         return -EINVAL;
 
     sysctl.cmd = XEN_SYSCTL_pm_op;
@@ -319,7 +319,7 @@ int xc_get_cpufreq_avgfreq(xc_interface *xch, int cpuid, int *avg_freq)
     int ret = 0;
     DECLARE_SYSCTL;
 
-    if ( (xch < 0) || (!avg_freq) )
+    if ( !xch || !avg_freq )
         return -EINVAL;
 
     sysctl.cmd = XEN_SYSCTL_pm_op;
@@ -384,7 +384,7 @@ int xc_get_cpuidle_max_cstate(xc_interface *xch, uint32_t *value)
     int rc;
     DECLARE_SYSCTL;
 
-    if ( xch < 0 || !value )
+    if ( !xch || !value )
         return -EINVAL;
 
     sysctl.cmd = XEN_SYSCTL_pm_op;
@@ -401,7 +401,7 @@ int xc_set_cpuidle_max_cstate(xc_interface *xch, uint32_t value)
 {
     DECLARE_SYSCTL;
 
-    if ( xch < 0 )
+    if ( !xch )
         return -EINVAL;
 
     sysctl.cmd = XEN_SYSCTL_pm_op;
@@ -416,7 +416,7 @@ int xc_enable_turbo(xc_interface *xch, int cpuid)
 {
     DECLARE_SYSCTL;
 
-    if ( xch < 0 )
+    if ( !xch )
         return -EINVAL;
 
     sysctl.cmd = XEN_SYSCTL_pm_op;
@@ -429,7 +429,7 @@ int xc_disable_turbo(xc_interface *xch, int cpuid)
 {
     DECLARE_SYSCTL;
 
-    if ( xch < 0 )
+    if ( !xch )
         return -EINVAL;
 
     sysctl.cmd = XEN_SYSCTL_pm_op;
-- 
1.7.10.4

Re: [PATCH] libxc/pm: Fix NULL pointer checks.

[Ian Campbell]

On Tue, 2013-09-10 at 10:29 +0100, Andrew Cooper wrote:
> Discovered by Coverity,
> CIDs 1054968 1054969 1054970 1054971 1054972 1054973 10549704
> 
> This was broken by c/s 5cc436c1d2b3b0 which did a blanket change of 'int
> xc_handle' -> 'xc_interface *xch'.  The types got updated, but error
> conditions were left as-were.  (I suspect some sed was involved originally)
> 
> Also while playing around in this area, fix up some of the bracketing style to
> match the Xen coding style.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>

Acked + applied, thanks.

Re: [PATCH] libxc/pm: Fix NULL pointer checks.

[Jan Beulich]

>>> On 10.09.13 at 11:29, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- a/tools/libxc/xc_pm.c
> +++ b/tools/libxc/xc_pm.c
> @@ -285,7 +285,7 @@ int xc_set_cpufreq_gov(xc_interface *xch, int cpuid, char *govname)
>      DECLARE_SYSCTL;
>      char *scaling_governor = sysctl.u.pm_op.u.set_gov.scaling_governor;
>  
> -    if ( (xch < 0) || (!govname) )
> +    if ( !xch || !govname )

I'm very surprised the compiler didn't reject this - I'm unaware of
an extension that would allow pointers to be compared by other
than == and != (plus it's all but clear what e.g. a "negative"
pointer really is).

Jan

Re: [PATCH] libxc/pm: Fix NULL pointer checks.

[Ian Campbell]

On Tue, 2013-09-10 at 13:23 +0100, Jan Beulich wrote:
> >>> On 10.09.13 at 11:29, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> > --- a/tools/libxc/xc_pm.c
> > +++ b/tools/libxc/xc_pm.c
> > @@ -285,7 +285,7 @@ int xc_set_cpufreq_gov(xc_interface *xch, int cpuid, char *govname)
> >      DECLARE_SYSCTL;
> >      char *scaling_governor = sysctl.u.pm_op.u.set_gov.scaling_governor;
> >  
> > -    if ( (xch < 0) || (!govname) )
> > +    if ( !xch || !govname )
> 
> I'm very surprised the compiler didn't reject this - I'm unaware of
> an extension that would allow pointers to be compared by other
> than == and != (plus it's all but clear what e.g. a "negative"
> pointer really is).

We were just discussing this at lunch and couldn't work it out either,
but indeed both gcc 4.7.[23] and clang 3.2 accept this when building
with -Wall:
int main(int argc, char **argv)
{
	if ( argv[1] < 0 )
		printf("ARGV[1] < 0\n");
	else
		printf("ARGV[1] >= 0\n");
	return 0;
}

Ian.

Re: [PATCH] libxc/pm: Fix NULL pointer checks.

[Juergen Gross]

On 10.09.2013 14:48, Ian Campbell wrote:
> On Tue, 2013-09-10 at 13:23 +0100, Jan Beulich wrote:
>>>>> On 10.09.13 at 11:29, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> --- a/tools/libxc/xc_pm.c
>>> +++ b/tools/libxc/xc_pm.c
>>> @@ -285,7 +285,7 @@ int xc_set_cpufreq_gov(xc_interface *xch, int cpuid, char *govname)
>>>       DECLARE_SYSCTL;
>>>       char *scaling_governor = sysctl.u.pm_op.u.set_gov.scaling_governor;
>>>
>>> -    if ( (xch < 0) || (!govname) )
>>> +    if ( !xch || !govname )
>>
>> I'm very surprised the compiler didn't reject this - I'm unaware of
>> an extension that would allow pointers to be compared by other
>> than == and != (plus it's all but clear what e.g. a "negative"
>> pointer really is).
>
> We were just discussing this at lunch and couldn't work it out either,
> but indeed both gcc 4.7.[23] and clang 3.2 accept this when building
> with -Wall:
> int main(int argc, char **argv)
> {
> 	if ( argv[1] < 0 )
> 		printf("ARGV[1] < 0\n");
> 	else
> 		printf("ARGV[1] >= 0\n");
> 	return 0;
> }

-Wextra is needed to produce an error:
http://gcc.gnu.org/onlinedocs/gcc-4.7.3/gcc/Warning-Options.html#Warning-Options


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PBG PDG ES&S SWE OS6                   Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@ts.fujitsu.com
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: [PATCH] libxc/pm: Fix NULL pointer checks.

[Jan Beulich]

>>> On 10.09.13 at 14:48, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2013-09-10 at 13:23 +0100, Jan Beulich wrote:
>> >>> On 10.09.13 at 11:29, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> > --- a/tools/libxc/xc_pm.c
>> > +++ b/tools/libxc/xc_pm.c
>> > @@ -285,7 +285,7 @@ int xc_set_cpufreq_gov(xc_interface *xch, int cpuid, 
> char *govname)
>> >      DECLARE_SYSCTL;
>> >      char *scaling_governor = sysctl.u.pm_op.u.set_gov.scaling_governor;
>> >  
>> > -    if ( (xch < 0) || (!govname) )
>> > +    if ( !xch || !govname )
>> 
>> I'm very surprised the compiler didn't reject this - I'm unaware of
>> an extension that would allow pointers to be compared by other
>> than == and != (plus it's all but clear what e.g. a "negative"
>> pointer really is).
> 
> We were just discussing this at lunch and couldn't work it out either,
> but indeed both gcc 4.7.[23] and clang 3.2 accept this when building
> with -Wall:
> int main(int argc, char **argv)
> {
> 	if ( argv[1] < 0 )
> 		printf("ARGV[1] < 0\n");
> 	else
> 		printf("ARGV[1] >= 0\n");
> 	return 0;
> }

Right. The warning is hidden behind -Wextra (i.e. there's no
more specific flag controlling this), and there's no way to turn off
that behavior altogether. Odd, but I guess we have to live with
it (entering this as a bug would just produce another of the many
orphaned entries in their bugzilla I'm afraid).

Jan

Re: [PATCH] libxc/pm: Fix NULL pointer checks.

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxc/pm: Fix NULL pointer checks."):
> On Tue, 2013-09-10 at 13:23 +0100, Jan Beulich wrote:
> > I'm very surprised the compiler didn't reject this - I'm unaware of
> > an extension that would allow pointers to be compared by other
> > than == and != (plus it's all but clear what e.g. a "negative"
> > pointer really is).

It is legal to compare for inequality[1] pointers into the same object.
However, it is not legal to compare for inequality any null pointer;
doing so is undefined behaviour.  C99 6.5.8(5).

> We were just discussing this at lunch and couldn't work it out either,
> but indeed both gcc 4.7.[23] and clang 3.2 accept this when building
> with -Wall:
> int main(int argc, char **argv)
> {
> 	if ( argv[1] < 0 )
> 		printf("ARGV[1] < 0\n");
> 	else
> 		printf("ARGV[1] >= 0\n");
> 	return 0;

I think it would be legal for this program to be compiled into
 #!/bin/sh
 echo hahahah

In the future, I wouldn't be surprised if a compiler were to
maliciously optimise away the test and one of the arms, or perhaps the
whole of the function.

Ian.

[1] < > <= >= are inequalities.  == and != are not.

Re: [PATCH] libxc/pm: Fix NULL pointer checks.

[Ian Jackson]

Andrew Cooper writes ("[PATCH] libxc/pm: Fix NULL pointer checks."):
> Discovered by Coverity,
> CIDs 1054968 1054969 1054970 1054971 1054972 1054973 10549704
> 
> This was broken by c/s 5cc436c1d2b3b0 which did a blanket change of 'int
> xc_handle' -> 'xc_interface *xch'.  The types got updated, but error
> conditions were left as-were.  (I suspect some sed was involved originally)

I see this has been committed, but I feel moved to comment:

...
> diff --git a/tools/libxc/xc_pm.c b/tools/libxc/xc_pm.c
> index fa9b246..ea1e251 100644
> --- a/tools/libxc/xc_pm.c
> +++ b/tools/libxc/xc_pm.c
> @@ -285,7 +285,7 @@ int xc_set_cpufreq_gov(xc_interface *xch, int cpuid, char *govname)
>      DECLARE_SYSCTL;
>      char *scaling_governor = sysctl.u.pm_op.u.set_gov.scaling_governor;
>  
> -    if ( (xch < 0) || (!govname) )
> +    if ( !xch || !govname )
>          return -EINVAL;

TBH I think callers who pass null xch's into xc should get a null
pointer dereference, not EINVAL.

But I don't propose to try to go through libxc and drain its crazy
error handling swamp.

Ian.