Re: [lwip-users] Alignment problem i pbuf_alloc()

[Bill Knight]

I'll jump in here.  'payload' is declared as a void pointer.  If all
it is ever used for is to access bytes, then 'offset' does not need to
be taken into the alignment calc. when determining the address to
store into 'payload'.  If however, if is used to read or store 16 or
32 bit values from and to the network device, and if it is aligned,
the data would not have to be broken into bytes and then reassembled.
I would suggest adding 'offset' into the alignment calc.

Regards
-Bill Knight
R O SoftWare
theARMPatch



On 25 May 2004 15:30:14 +0100, K.J. Mansley wrote:

>On Tue, 2004-05-25 at 09:11, Lars Thorup wrote:
>> Fra: K.J. Mansley [mailto:address@hidden 
>> > On Tue, 2004-05-25 at 07:05, Lars Thorup wrote:
>> > > This is the original code from pbuf_alloc():
>> > > 
>> > > case PBUF_RAM:
>> > >   /* If pbuf is to be allocated in RAM, allocate memory for it. */
>> > >   p = mem_malloc(MEM_ALIGN_SIZE(sizeof(struct pbuf) + length + 
>> > > offset));
>> > > 
>> > > I have changed the third line to
>> > > 
>> > >   p = mem_malloc(MEM_ALIGN_SIZE(sizeof(struct pbuf) + offset) + 
>> > > MEM_ALIGN_SIZE(length));
>> > 
>> > Would it be better to have:
>> > 
>> > p = mem_malloc(MEM_ALIGN_SIZE(sizeof(struct pbuf)) + 
>> > MEM_ALIGN_SIZE(offset + length));
>> > 
>> > This makes more sense to me, as "offset" and "length" go 
>> > together to make the payload of the pbuf, but it may mean the 
>> > payload pointer starts off pointing at a non-word aligned 
>> > address, which might cause problems?
>> 
>> No that would not work; I just tried it out and got the same memory
>> overwrite as with the original code. The explanation is that the "offset"
>> bytes is not included in the part of the buffer that the payload pointer is
>> set to point at, but rather in the part before the payload pointer. This can
>> be seen four lines ahead in the source code:
>> 
>>      p->payload = MEM_ALIGN((void *)((u8_t *)p + sizeof(struct pbuf) +
>> offset));

>Good point.  I think the difference boils down to this: should the start
>of the possible payload (i.e. p+sizeof(struct pbuf)) be aligned, or the
>initial payload pointer be aligned (i.e. p+sizeof(struct pbuf)+offset)?

>If the former, the we should use the version I suggested, but also
>change the last line above to:

>p->payload = MEM_ALIGN((void *)((u8_t *)p + sizeof(struct pbuf))) + offset;

>i.e. take the "offset" out of the align calculation.

>If the later, your fix will be correct.

>Could anyone with more understanding of the alignment issues than me
>give an opinion on this?

>Thanks

>Kieran



>_______________________________________________
>lwip-users mailing list
>address@hidden
>http://mail.gnu.org/mailman/listinfo/lwip-users