[Mesa-dev] [PATCH 1/9] util: fix undefined behavior

[Tolga Dalman]

On 06.04.2012 17:38, nobled wrote:
> On Mon, Apr 2, 2012 at 7:40 AM, Tolga Dalman
> <tolga.dalman at googlemail.com> wrote:
> Hi,
> 
> 
> On 02.04.2012 00:24, nobled wrote:
>>>>  #define LIST_FOR_EACH_ENTRY(pos, head, member)                               \
>>>> -   for (pos = container_of((head)->next, pos, member);                       \
>>>> +   for (pos = NULL, pos = container_of((head)->next, pos, member);   \
>>>>       &pos->member != (head);                                         \
>>>>       pos = container_of(pos->member.next, pos, member))
> 
> The container_of macro is obviously error-prone and leads to future
> problems. Your patch works around that buggy API instead of
> really fixing it.
> 
> container_of could be defined using the offsetof macro, i.e.,
> 
> #define container_of(ptr, type, member) \
>  (void *)((char *)(ptr) - offsetof(type, member))
> 
> As far as I can see, container_of is only used sparsely in
> mesa, so applying my proposal should be possible quite easily.
>> Not really. container_of() is only used *directly* in this one header;
>> but changing it to require a type parameter requires also changing all
>> the macros that use it in turn, which actually are used widely in
>> mesa. 

Widely is a bit exaggerated. Basically, it is only used in the
LIST_FOR_EACH macros. A quick grep results:

grep "[[:space:]]LIST_FOR_EACH" * -R|grep -v u_double_list.h|wc -l
35

I also noticed, that LIST_FOR_EACH_ENTRY_FROM and LIST_FOR_EACH_ENTRY_FROM_REV
are not used anywhere in the mesa source.


>> Adding the type name to every use of the LIST_FOR_EACH_* macros
>> also introduces redundancy and the possibility of error, e.g. if the
>> containing ever changes but not every use of the macro is updated,
>> there's no compile error but it'll go wrong at runtime. That can't
>> happen with the current version of it.

Easy: use typeof(), i.e., the LIST_FOR_EACH_* macros are updated as follows:

#define LIST_FOR_EACH_ENTRY(pos, head, member) \
   for (pos = container_of((head)->next, typeof(pos), member); \
    &pos->member != (head); \
    pos = container_of(pos->member.next, typeof(pos), member))

In the long term, the container_of/LIST_FOR_EACH mess should go away.
Instead, just implement the clean linux kernel list_for_each
implementation (whence, I presume, the Mesa list is borrowed from).

>> container_of() just has one basic caveat that we can easily satisfy,
>> since like you said, it isn't directly used in many places, and I
>> don't think future problems are actually likely--any misuse will
>> trigger uninitialized warnings like the above so they're easily
>> caught, unlike possible typename mistakes.

Agreed.


Thanks and best regards
Tolga Dalman