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