[Mesa-dev] [PATCH 1/9] util: fix undefined behavior
nobled at dreamwidth.org
Sun Apr 8 07:25:05 PDT 2012
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:
>> 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
> 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
> 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:
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.
> Thanks and best regards
> Tolga Dalman
More information about the mesa-dev