On Mon, Apr 1, 2013 at 8:59 PM, Dan Gora <[email protected]> wrote:
> On Mon, Apr 1, 2013 at 3:48 PM, Zdenek Styblik <[email protected]>
> wrote:
>> On Fri, Mar 22, 2013 at 1:17 AM, Dan Gora <[email protected]> wrote:
>>> If the offset was greater than 127, then the multi_offset would end
>>> up being a negative value, which is not what we want.
>>>
>>
>> Dan,
>>
>> I actually don't understand this patch. Please, can you dumb it down for me?
>> Also, given your description, I actually don't think problem has been
>> fixed. Shouldn't there, somewhere, be a check whether offset is less
>> than 127? Because what you've described is integer *flow going on.
>
I've moved up the following piece of text.
> I didn't actually see this bug, but it looked wrong. The offset
> should never be negative, but there's nothing saying that it has to be
> less than 128.
>
Alright, that was my point. Some comments follow.
> Here's the patched code:
>
> FILE * input_file;
> unsigned char data;
> unsigned char last_record = 0;
> unsigned int multi_offset;
> int record_count = 0;
>
> input_file = fopen ( filename, "r");
> if ( input_file == NULL ){
> lprintf(LOG_ERR, "File: '%s' is not found", filename);
> return ERROR_STATUS;
> }
>
> fseek ( input_file, START_DATA_OFFSET, SEEK_SET );
> fread ( &data, 1, 1, input_file );
> if ( data <= 0 ){
This has to be changed to: ``if (data == 0)'' or ``if (data < 1)''
since 'data' is unsigned now or it will yield compiler warning.
> lprintf(LOG_NOTICE, "There is no multi record in the file
> %s\n",
> filename);
This LOG_NOTICE -> LOG_ERR
> fclose( input_file );
> return ERROR_STATUS;
> }
> /*the offset value is in multiple of 8 bytes.*/
> multi_offset = data * 8;
> if ( verbose == LOG_DEBUG ) {
This looks odd to me. I mean, I've seen code like ``if (verbose)'' or
``if (verbose > 2)'' or whatever, but nothing like this. I think this
is wrong.
> printf( "start multi offset = 0x%02lx\n", multi_offset );
> }
> fseek ( input_file, multi_offset, SEEK_SET );
> while ( !feof( input_file ) ) {
> *list_record = malloc ( sizeof (struct ipmi_ek_multi_header)
> );
> fread ( &(*list_record)->header, START_DATA_OFFSET, 1,
> input_file);
> if ( (*list_record)->header.len == 0 ){
> record_count++;
> continue;
> }
>
>
> So, 'data' is 1 byte long and it's reading an offset field. The
> offset cannot be negative, but it *could* (I guess, in theory, but it
> probably never is in practice) be greater than 127. Since the offset
> is always positive you want 'data' and 'multi_offset' to be positive,
> so we should use unsigned types for these. If 'data' was 128 or more,
> it would have yielded a negative offset for the fseek in "fseek (
> input_file, multi_offset, SEEK_SET );"
>
> thanks
> d
Z.
------------------------------------------------------------------------------
Own the Future-Intel® Level Up Game Demo Contest 2013
Rise to greatness in Intel's independent game demo contest.
Compete for recognition, cash, and the chance to get your game
on Steam. $5K grand prize plus 10 genre and skill prizes.
Submit your demo by 6/6/13. http://p.sf.net/sfu/intel_levelupd2d
_______________________________________________
Ipmitool-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
