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

[nobled]

On Sun, Apr 8, 2012 at 4:28 AM, Tolga Dalman
<tolga.dalman at googlemail.com> wrote:
> 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:
Love to, but typeof() is a GNU extension:
http://gcc.gnu.org/onlinedocs/gcc/Typeof.html

Unless MSVC suddenly started supporting it, it's a no-go in gallium
code (which is supposed to be portable so it has to stick to C89).
>
> #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
>