X-Authentication-Warning: delorie.com: mail set sender to geda-user-bounces using -f
X-Recipient: geda-user AT delorie DOT com
Date: Tue, 20 Dec 2016 10:18:31 +0100 (CET)
X-X-Sender: igor2 AT igor2priv
To: geda-user AT delorie DOT com
X-Debug: to=geda-user AT delorie DOT com from="gedau AT igor2 DOT repo DOT hu"
From: gedau AT igor2 DOT repo DOT hu
Subject: [geda-user] [pcb] bugreport
Message-ID: <alpine.DEB.2.00.1612200951240.7286@igor2priv>
User-Agent: Alpine 2.00 (DEB 1167 2008-08-23)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; format=flowed; charset=US-ASCII
Reply-To: geda-user AT delorie DOT com

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