Re: [PATCH] tools lib traceevent: Mask higher bits of str addresses for 32-bit traces

[Kapileshwar Singh <kapileshwar.singh@arm.com>]

Hi Steve,

On 18/09/15 14:45, Steven Rostedt wrote:
> On Fri, 18 Sep 2015 11:55:47 +0100
> Kapileshwar Singh <kapileshwar.singh@arm.com> wrote:
> 
>>>>> Perhaps we need to make addr into a unsigned long long, and then add:
>>>>>
>>>>>       addr = (pevent->long_size == 8) ?
>>>>>               *(unsigned long long *)(data + field->offset) :
>>>>>               (unsigned long long )*(unsigned int *)(data + field->offset);
>>>
>>> What about this? (untested)
>>>
>>>     addr = *(uint64_t *)(data + field->offset) &
>>>                         ((1ULL << pevent->long_size * 8) - 1);
>>
>> I tested this and it works fine.
> 
> Except that I think it may be buggy.
> 
>>
>>>
>>> Do we also need to consider byte endians?  Maybe it'd be better adding
>>> a helper to dereference pointers then..
> 
> Yes and no.
> 
>>
>> In this particular case, since the address is just a key for a lookup into the 
>> printk_map, which seems like a (addr -> const char *) mapping for string 
>> literals in the trace file, the endian-ness should not matter (I could be wrong though).
> 
> Correct, which is why I said "no", BUT! this is why I think Namhyung's
> version may be buggy (besides the overflow of the buffer).
> 
> If this is a 64 bit big endian reading a 32 bit little endian file, I
> think the result will be incorrect.
> 
> The *(uint64_t *) will return a 64bit number, but the address (with
> long_size == 4) only needs 32bits. Thus, we are getting 32 more bits
> than needed. Let's say the address is 0x12345678 that is loaded in the
> file. Being little endian, it would be loaded as "78 56 34 12". Let's
> say the 32bits after that is 0xDEADBEEF, loaded as "EF BE AD DE". Now
> the number returned to addr (being a 64 bit big endian) would be:
> 0x785643412EFBEADDE  But then we do the shift:
> 
>  (1ULL << pevent->long_size * 8) - 1; which would leave us with:
> 
> 0xEFBEADDE
> 
> Not what we wanted.

Agreed.

> 
> My version only reads the necessary bytes, and also wont suffer from
> reading past the data size of the buffer (which is another bug).
> 

Thanks for noticing and explaining this, makes perfect sense now!

Will submit a v3 for this.

Regards, 
KP

> -- Steve
> 
> 
>