On Mon, Oct 10, 2011 at 8:44 PM, Zdenek Styblik
<zdenek.styb...@gmail.com> wrote:
[...]
> It is good you're using strtol() function rather than atoi(), because
> it does (kind of) validation for you.
> However I think you should check whether the last processed character
> was '\0' and whether there
> was not overflow.
>
>>> + retry = (uint8_t)strtol(optarg, NULL, 0);
>
> ~~~
> #include <errno.h>
> [...]
> char *end_ptr = 0;
> long retry_long = 0;
> errno = 0;
> retry_long = strtol(optarg, &end_ptr, 0);
> if (errno != 0) { /* overflow occured */ }
> if (*end_ptr != '\0' || retry < 0) { /* something like 123abc was
> supplied rather than int */ }
> /* all good */
> retry = (uint8_t)retry_long;
> ~~~
>
> Checking `retry < 0' might be pointless if retry will be of type of
> unsigned long.
>
> Anyway, my point is:
> * as of now, it is possible to input number greater than unsigned long
> and there is no check
> whether user did or did not. Entering such input value will cause
> overflow and value of retry_long
> is going to be "random"(honestly, I don't know whether values after
> overflow can be predicted
> or not. But that's beyond the point here)
[...]
Hello Harshad,
one thing just pop-ed up in my head about this thing. And sadly it led
to another.
First of all, I did little speech about overflow yet there is no check
whether long fits
into unsigned char.
So I've decided to rewrite your patch only to find out
'lib/ipmi_main.c' is full of
these. I'll be working on solution, because it seems the best way to
fix this is to
write a function.
Of course if somebody is willing to fix this, be my guest.
I will apply your patch and continue from there, or shouldn't I?
Best regards,
Z.
>> -----Original Message-----
>> From: Zdenek Styblik [mailto:zdenek.styb...@gmail.com]
>> Sent: Monday, October 10, 2011 10:27 PM
>> To: Harshad Prabhu (hprabhu)
>> Cc: ipmitool-devel@lists.sourceforge.net
>> Subject: Re: [Ipmitool-devel] Regarding Retries/Timeout for LAN/LANPLUS
>> interface
>>
>> On Mon, Oct 10, 2011 at 5:32 PM, Harshad Prabhu (hprabhu)
>> <hpra...@cisco.com> wrote:
>> [...]
>>> + /* Retry and Timeout */
>>> + case 'R':
>>> + retry = (uint8_t)strtol(optarg, NULL, 0);
>>> + break;
>>> + case 'N':
>>> + timeout = (uint8_t)strtol(optarg, NULL, 0);
>>> + break;
>>
>> Hello,
>>
>> it seems you're not worried it is possible to overflow `retry' and
>> `timeout' which will lead to unexpected behavior.
>>
>> Regards,
>> Zdenek
>>
>
------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure contains a
definitive record of customers, application performance, security
threats, fraudulent activity and more. Splunk takes this data and makes
sense of it. Business sense. IT sense. Common sense.
http://p.sf.net/sfu/splunk-d2d-oct
_______________________________________________
Ipmitool-devel mailing list
Ipmitool-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
