Mail Archives: geda-user/2016/12/20/04:14:34
Hi all,
FlagType is a struct; FLAGS_EQUAL attempts to compare two flags using
memcmp() on the full struct.
While this happens to work at the moment on the most common platforms,
it's wrong, because the C standard lets the compiler to:
- insert padding between struct fields (except before the first), to
achieve whatever alignment it sees fit, in an "implementation-defined
manner"
- may append an "unnamed padding" at the end of the struct, which will be
counted in sizeof()
Thus memcmp() may go and compare potentially uninitialized padding fields,
saying two equal flags are not equal because of differences in
uninitialized bytes.
Why it works at the moment is: the first field is a long, which is wide
enough that the second field won't need alignment on the most common
current platforms, and MAX_LAYER is 16 so t[] ends up being 8 characters
long which is usually good enough not to add pads to the end of the struct
by most compilers today. But neither of these are guaranteed. Raising
MAX_LAYER to 18 could probably break it. The other reason is that the
macro is used only once in the code (minus external plugins) and it
probably gets 0-initialized flag structs all (or most of) the time - I was
too lazy to trace this back.
It is a trap for future development: if you extend the struct, it may get
random new paddings even on current platforms/compilers, which could break
in hard-to-detect ways. (I found this memcmp() only because I did extend
the flag struct in pcb-rnd and it did get some padding - but now I will go
an search for all memcmp()s in the code).
I've fixed this in pcb-rnd; I recommend fixing this in mainline too, by
comparing the struct field by field.
(In theory it could be fixed by making sure all bytes of all flag fields
are always 0-initialized, sure these get memset() to 0 all the time even
by future code sounds less safe. But it's up to mainline devs to decide.)
Regards,
Igor2
- Raw text -