Discussion:
[Ipmitool-devel] Code Review: ID: 212 - 'lib/ipmi_dcmi.c' - possible int *flow
Zdenek Styblik
2013-07-25 04:40:28 UTC
Permalink
Hi,

attached is a patch to mitigate possible int *flows via user input.
It's pretty much no brainer except those shifts. Therefore I'd
appreciate if somebody could give it a look.
Are those shifts correct?
Will they work correctly on big endian too?

Thanks,
Z.
Dan Gora
2013-07-25 14:52:41 UTC
Permalink
On Thu, Jul 25, 2013 at 1:40 AM, Zdenek Styblik
Post by Zdenek Styblik
Hi,
attached is a patch to mitigate possible int *flows via user input.
It's pretty much no brainer except those shifts. Therefore I'd
appreciate if somebody could give it a look.
Are those shifts correct?
Will they work correctly on big endian too?
well it now packs the data into the buffer in little endian format on
both little endian and big endian hosts, so at least it's now
consistent. It's not clear if the data is _supposed_ to be packed in
little endian format, but presumably it was..

The other thing that I would recommend is that you _not_ initialize
local variables when you declare them. What this does is to preclude
the compiler from finding code paths where the variable is used, but
uninitialized. The compiler is much better at finding those code
paths than humans are.

thanks
dan
Zdenek Styblik
2013-07-25 14:59:16 UTC
Permalink
Post by Dan Gora
On Thu, Jul 25, 2013 at 1:40 AM, Zdenek Styblik
Post by Zdenek Styblik
Hi,
attached is a patch to mitigate possible int *flows via user input.
It's pretty much no brainer except those shifts. Therefore I'd
appreciate if somebody could give it a look.
Are those shifts correct?
Will they work correctly on big endian too?
well it now packs the data into the buffer in little endian format on
both little endian and big endian hosts, so at least it's now
consistent. It's not clear if the data is _supposed_ to be packed in
little endian format, but presumably it was..
First of all, thanks looking into it.
I can compare "old" code with a new one, but I don't anything big
endian ready to test at. And I don't feel like setting up QEMU-ARM
right now. However, I can share small code snippet with a test
code(later).
Post by Dan Gora
The other thing that I would recommend is that you _not_ initialize
local variables when you declare them. What this does is to preclude
the compiler from finding code paths where the variable is used, but
uninitialized. The compiler is much better at finding those code
paths than humans are.
Hm, I see your point. Perhaps a dumb question, but do you mean only
variables or structures/arrays as well? Because I'd think to have
variable initialized to a known value is better, eg. this was proven
in 'ipmi_sel.c' where uninitialized 'struct time_t' had random
results.

Thanks,
Z.
Dan Gora
2013-07-25 15:39:07 UTC
Permalink
On Thu, Jul 25, 2013 at 11:59 AM, Zdenek Styblik
Post by Zdenek Styblik
Post by Dan Gora
On Thu, Jul 25, 2013 at 1:40 AM, Zdenek Styblik
Post by Zdenek Styblik
Hi,
attached is a patch to mitigate possible int *flows via user input.
It's pretty much no brainer except those shifts. Therefore I'd
appreciate if somebody could give it a look.
Are those shifts correct?
Will they work correctly on big endian too?
well it now packs the data into the buffer in little endian format on
both little endian and big endian hosts, so at least it's now
consistent. It's not clear if the data is _supposed_ to be packed in
little endian format, but presumably it was..
First of all, thanks looking into it.
I can compare "old" code with a new one, but I don't anything big
endian ready to test at. And I don't feel like setting up QEMU-ARM
right now. However, I can share small code snippet with a test
code(later).
I have a big endian setup, but honestly I don't even know what this
dcmi stuff is or if it's relevant to my ATCA setup or if I can test
it.. I'll have a quick look later when I get to the office.
Post by Zdenek Styblik
Post by Dan Gora
The other thing that I would recommend is that you _not_ initialize
local variables when you declare them. What this does is to preclude
the compiler from finding code paths where the variable is used, but
uninitialized. The compiler is much better at finding those code
paths than humans are.
Hm, I see your point. Perhaps a dumb question, but do you mean only
variables or structures/arrays as well?
Both. The gcc compiler cannot pick out used, but uninitialized fields
from a structure (other compilers might), but IMHO it's better to
explicitly initialize the structure near where it's going to be used
so that people can see that you're explicitly initializing it and know
that you didn't just forget about it.

For instance:

struct blahblah {
int foo;
int bar;
};

int somefunc(void)
{
struct blahblah test;

<code>
<code>

/* initialize test to all 0's */
memset(test, 0, sizeof(test));
test.bar = 1;

printf ("foo=%d bar=%d\n", test.foo, test.bar);
}

Also, remember that:

uint8_t data[256] = {0};

does not zero all the elements of 'data', only the first element.
Post by Zdenek Styblik
Because I'd think to have
variable initialized to a known value is better, eg. this was proven
in 'ipmi_sel.c' where uninitialized 'struct time_t' had random
results.
I'm not sure really about this particular case, but (IHMO) you should
leave it uninitialized in the declaration and if you know that there
is a code path where it's not going to get initialized, initialize it
in that code path. Either that or set it to some default value close
by the first use of that variable so that the reader knows that it's
supposed to have some default value and that default value has not
scrolled off the top of the screen.

thanks
d
Zdenek Styblik
2013-07-25 18:11:03 UTC
Permalink
Post by Dan Gora
Post by Zdenek Styblik
First of all, thanks looking into it.
I can compare "old" code with a new one, but I don't anything big
endian ready to test at. And I don't feel like setting up QEMU-ARM
right now. However, I can share small code snippet with a test
code(later).
I have a big endian setup, but honestly I don't even know what this
dcmi stuff is or if it's relevant to my ATCA setup or if I can test
it.. I'll have a quick look later when I get to the office.
DCMI is some M$ server power management thing or whatnot. I believe
trying out similar code snippet should be enough to verify it works as
expected.
Please, can you test http://pastebin.com/1ideZhjn ?

Not that it's of any help, but output at x86-64:
~~~
0: '128'
1: '62'
2: '128'
3: '62'
4: '160'
5: '255'
6: '255'
7: '255'
8: '160'
9: '255'
10: '255'
11: '255'
12: '56'
13: '81'
14: '56'
15: '81'
16: '0'
17: '0'
18: '0'
19: '0'
~~~
Post by Dan Gora
Post by Zdenek Styblik
Hm, I see your point. Perhaps a dumb question, but do you mean only
variables or structures/arrays as well?
Both. The gcc compiler cannot pick out used, but uninitialized fields
from a structure (other compilers might), but IMHO it's better to
explicitly initialize the structure near where it's going to be used
so that people can see that you're explicitly initializing it and know
that you didn't just forget about it.
[...code example...]

I sort of wish I knew why, but ok and I'll try to remember. Still, the
diff between ``int i = 0;'' V. ``int i; i = 0;'' is just ... I guess
beyond my understanding, resp. knowledge :|
Post by Dan Gora
uint8_t data[256] = {0};
does not zero all the elements of 'data', only the first element.
Damn! You're absolutely right! I always forget about this. I did write
it down this time and I hope I'll remember it from now on.
Also, 'data' is being initialized like two lines below. Ehm.
Post by Dan Gora
I'm not sure really about this particular case, but (IHMO) you should
leave it uninitialized in the declaration and if you know that there
is a code path where it's not going to get initialized, initialize it
in that code path. Either that or set it to some default value close
by the first use of that variable so that the reader knows that it's
supposed to have some default value and that default value has not
scrolled off the top of the screen.
The problem is nobody does it(init). And further you go from your
declarations the easier is to forget about it. Then only one bit is
initialized and the rest of structure is left at "random". But this is
beyond discussion whether to initialize in the declaration or later in
the code.
Simply - ok.

Z.
Dan Gora
2013-07-25 18:48:34 UTC
Permalink
On Thu, Jul 25, 2013 at 3:11 PM, Zdenek Styblik
Post by Zdenek Styblik
Post by Dan Gora
Both. The gcc compiler cannot pick out used, but uninitialized fields
from a structure (other compilers might), but IMHO it's better to
explicitly initialize the structure near where it's going to be used
so that people can see that you're explicitly initializing it and know
that you didn't just forget about it.
[...code example...]
I sort of wish I knew why, but ok and I'll try to remember. Still, the
diff between ``int i = 0;'' V. ``int i; i = 0;'' is just ... I guess
beyond my understanding, resp. knowledge :|
Let's consider an example:

int foo(int arg)
{
int i;

if (arg < 1)
i = 1;
else if (arg >= 2)
i = 2;

printf("i=%d\n", i);
}

Now in this case if 'arg' is 1, then the value printed will be
undefined. However the compiler will spit a warning "i may be used
uninitialized" (or something to that effect), so you know that you
have an unused case there. If you just blindly initialize 'int i=0;'
at the top of the function then you will not get that warning. Maybe
you really wanted i to be 3 if arg was 1, but you just forgot to
handle the case where arg == 1. A better style would be:

int foo(int arg)
{
int i;

if (arg < 1)
i = 1;
else if (arg >= 2)
i = 2;
else
i = 0;

printf("i=%d\n", i);
}

because then you know that you have all of the cases covered and the
compiler will not spit a warning about uninitialized variables being
used.

Now of course there are exceptions to this "rule". Something like:

int foo (int arg)
{
int ret = 0;

if (arg)
{
ret = func1();
}
return (ret);
}

Is fine and you see that a lot. In this case you have a known default
value that you are setting ret to. You're not just initializing ret
to something just to initialize it or (worse yet) to get rid of
warnings about ret being used without being initialized. However to
my mind it's better to not initialize ret in the declaration and just
initialize it close to where it's first used.:

int foo (int arg)
{
   int ret;

/* default to success! */
ret = 0;
   if (arg)
     {
     ret = func1();
     }
   return (ret);
}

This just tells me, as a reader, that the author _knows_ that ret may
not be set by the body of the function and wants ret set to some
default value. But this is a minor quibble for small functions.
Post by Zdenek Styblik
Post by Dan Gora
uint8_t data[256] = {0};
does not zero all the elements of 'data', only the first element.
Damn! You're absolutely right! I always forget about this. I did write
it down this time and I hope I'll remember it from now on.
Also, 'data' is being initialized like two lines below. Ehm.
yeah that another thing that these patches had.. Something like:

int ret = 0;

ret = scanf(fd.....);

The ret = 0 in the declaration is totally unnecessary (and will just
be optimized out) since we're immediately going to set ret to the
return value from scanf.
Post by Zdenek Styblik
The problem is nobody does it(init).
And further you go from your
declarations the easier is to forget about it. Then only one bit is
initialized and the rest of structure is left at "random". But this is
beyond discussion whether to initialize in the declaration or later in
the code.
Simply - ok.
I agree, there are not hard and fast rules for everything, you have to
use a bit of sense, I'm just saying that it's better not to do the
initialization in the declaration unless there's a good reason to
since it can suppress these valuable compile-time warnings.

thanks
dan
Zdenek Styblik
2013-07-26 08:18:13 UTC
Permalink
On Thu, Jul 25, 2013 at 8:48 PM, Dan Gora <***@gmail.com> wrote:
[...]
Post by Dan Gora
int foo(int arg)
{
int i;
if (arg < 1)
i = 1;
else if (arg >= 2)
i = 2;
printf("i=%d\n", i);
}
Now in this case if 'arg' is 1, then the value printed will be
undefined. However the compiler will spit a warning "i may be used
uninitialized" (or something to that effect), so you know that you
have an unused case there. If you just blindly initialize 'int i=0;'
at the top of the function then you will not get that warning. Maybe
you really wanted i to be 3 if arg was 1, but you just forgot to
int foo(int arg)
{
int i;
if (arg < 1)
i = 1;
else if (arg >= 2)
i = 2;
else
i = 0;
printf("i=%d\n", i);
}
because then you know that you have all of the cases covered and the
compiler will not spit a warning about uninitialized variables being
used.
int foo (int arg)
{
int ret = 0;
if (arg)
{
ret = func1();
}
return (ret);
}
Is fine and you see that a lot. In this case you have a known default
value that you are setting ret to. You're not just initializing ret
to something just to initialize it or (worse yet) to get rid of
warnings about ret being used without being initialized. However to
my mind it's better to not initialize ret in the declaration and just
int foo (int arg)
{
int ret;
/* default to success! */
ret = 0;
if (arg)
{
ret = func1();
}
return (ret);
}
This just tells me, as a reader, that the author _knows_ that ret may
not be set by the body of the function and wants ret set to some
default value. But this is a minor quibble for small functions.
Post by Zdenek Styblik
Post by Dan Gora
uint8_t data[256] = {0};
does not zero all the elements of 'data', only the first element.
Damn! You're absolutely right! I always forget about this. I did write
it down this time and I hope I'll remember it from now on.
Also, 'data' is being initialized like two lines below. Ehm.
int ret = 0;
ret = scanf(fd.....);
The ret = 0 in the declaration is totally unnecessary (and will just
be optimized out) since we're immediately going to set ret to the
return value from scanf.
Post by Zdenek Styblik
The problem is nobody does it(init).
And further you go from your
declarations the easier is to forget about it. Then only one bit is
initialized and the rest of structure is left at "random". But this is
beyond discussion whether to initialize in the declaration or later in
the code.
Simply - ok.
I agree, there are not hard and fast rules for everything, you have to
use a bit of sense, I'm just saying that it's better not to do the
initialization in the declaration unless there's a good reason to
since it can suppress these valuable compile-time warnings.
thanks
dan
Oh, I see your point now. I was thinking something different - CPU
code execution optimization etc.
It makes sense, especially in case of somebody's else code, but it
will be hard to fight/resist doing it.

Thanks for explanation,
Z.
Dan Gora
2013-08-05 18:43:06 UTC
Permalink
Hi Zdenek,

Sorry it took so long to get back to this...

On Thu, Jul 25, 2013 at 3:11 PM, Zdenek Styblik
Post by Zdenek Styblik
DCMI is some M$ server power management thing or whatnot. I believe
trying out similar code snippet should be enough to verify it works as
expected.
Please, can you test http://pastebin.com/1ideZhjn ?
~~~
0: '128'
1: '62'
2: '128'
3: '62'
4: '160'
5: '255'
6: '255'
7: '255'
8: '160'
9: '255'
10: '255'
11: '255'
12: '56'
13: '81'
14: '56'
15: '81'
16: '0'
17: '0'
18: '0'
19: '0'
~~~
Here's the output on a big endian machine (cavium MIPS 64 bits):

***@octeon:~# ./z
sample: 20792
limit: 16000
correction: 4294967200
0: '128'
1: '62'
2: '62'
3: '128'
4: '160'
5: '255'
6: '255'
7: '255'
8: '255'
9: '255'
10: '255'
11: '160'
12: '56'
13: '81'
14: '81'
15: '56'
16: '0'
17: '0'
18: '0'
19: '0'

This is why you have to use:

data[4] = correction >> 0;
data[5] = correction >> 8;
data[6] = correction >> 16;
data[7] = correction >> 24;

Instead of:

*(uint32_t*)(&data[8]) = correction;

to make the code endian neutral...

thanks
d
Zdenek Styblik
2013-08-06 18:45:28 UTC
Permalink
Post by Dan Gora
Hi Zdenek,
Sorry it took so long to get back to this...
Hello Dan,

it's better late than never. We're all busy :)
[...]
Post by Dan Gora
data[4] = correction >> 0;
data[5] = correction >> 8;
data[6] = correction >> 16;
data[7] = correction >> 24;
*(uint32_t*)(&data[8]) = correction;
to make the code endian neutral...
thanks
d
Thanks for confirmation. I still haven't committed those changes,
although it was due to lack of time/prio to do so.

Regards,
Z.
Liebig, Holger
2013-09-09 15:14:49 UTC
Permalink
Post by Zdenek Styblik
Hi,
attached is a patch to mitigate possible int *flows via user input.
It's pretty much no brainer except those shifts. Therefore I'd appreciate
if somebody could give it a look.
Are those shifts correct?
Will they work correctly on big endian too?
[Liebig, Holger]
Just a heads up:
unfortunately the change breaks 'ipmitool dcmi power set_limit action power_off' due to the test for a valid integer value at the beginning of the ipmi_dcmi_pwr_slimit() function which should be valid only for the numeric options.

Also, the dcmi_pwrmgmt_action_vals array has only the long description strings and no value strings, so the above command would still fail without additional work and last but not least, 'no_action' cannot be specified.

Since the window for the 1.8.13 is most likely closed, these should be documented as limitations/known bugs somewhere. I will provide a patch proposal, but would like to check the other stuff
Zdenek Styblik
2013-09-09 17:40:14 UTC
Permalink
On Mon, Sep 9, 2013 at 5:14 PM, Liebig, Holger
Post by Liebig, Holger
[Liebig, Holger]
unfortunately the change breaks 'ipmitool dcmi power set_limit action power_off' due to the test for a valid integer value at the beginning of the ipmi_dcmi_pwr_slimit() function which should be valid only for the numeric options.
Also, the dcmi_pwrmgmt_action_vals array has only the long description strings and no value strings, so the above command would still fail without additional work and last but not least, 'no_action' cannot be specified.
Since the window for the 1.8.13 is most likely closed, these should be documented as limitations/known bugs somewhere. I will provide a patch proposal, but would like to check the other stuff as well before sending it.
Holger
I can't come up with anything else, but ... funny, very funny.

Best regards,
Z.

Loading...