X-Authentication-Warning:	delorie.com: mail set sender to geda-user-bounces using -f
X-Recipient:	geda-user AT delorie DOT com
X-Original-DKIM-Signature:	v=1; a=rsa-sha256; c=relaxed/relaxed;
	d=gmail.com; s=20161025;
	h=mime-version:in-reply-to:references:from:date:message-id:subject:to;
	bh=y2ZO825H0LaBt+7fMn3wxRa+qw2zPTdwqK6cI4amd2A=;
	b=LX4AEYiS1v5J9r48oj+Rraf+Qfnp2u4NLDpqihaplxt80Q1XxpYca9EemR/7qMKw7P
	2+SJiuVRiU6LNi5cwJGfLIvNGjCYulpGL1K/UC9pAedvv7Rq3Mve8HqW7rZnHuJMxkJR
	yimwcYeustglq5vfEeNAs/iWpAKeM+2Lc9O2sO7/jpswfp6GNyxVd9HtpYISnwgA2Fbc
	LQLFvxhZz21HfdG1TwiebMang4xEj+zZURrEuf04x3cHtqDL61kqB2BvXNt65Jag06BK
	snjIIll7gj1gApFHfMh/wf9WDLD//Na1Wf1JpzhxmRUE8IBCPNb/CxRiJk9RWO9BW1lA
	3rpQ==
X-Google-DKIM-Signature:	v=1; a=rsa-sha256; c=relaxed/relaxed;
	d=1e100.net; s=20161025;
	h=x-gm-message-state:mime-version:in-reply-to:references:from:date
	:message-id:subject:to;
	bh=y2ZO825H0LaBt+7fMn3wxRa+qw2zPTdwqK6cI4amd2A=;
	b=LJf8H3d1cIX42ncDQHzQYHs6MXairRBpScabvT+YtIvncLh0dkQK62FjnQzl7QjTkj
	0cEEBKKdvUnouc4cO2ik92Z799/I+ZjAEcz5hkhn7//6Atzs+e0nD/rwBihyBxbvH6zt
	rkZgDj2chKjZzuV8AKKNJCM4Ek2/DELdZu55s1blfR7sDWMUlKG4fWMxlMMFvCd/yu2y
	+22C89nZ+8cjMP9De+W3CVtfpS641e1T34J7NpAW4itAJOfxFCnwpZWTzxWloviKuxfj
	Mu5Oz82V1el3ftYhdp3ppAJcAH/Bv5mKnPxr3udVcxx+myZG5y8tWDcoyVHqti2IfaIb
	lIlQ==
X-Gm-Message-State:	AIkVDXJnVKZ/kHdGTKzdr6HyMydWOgjYRFfLoDzczX/hh4WvUYYkANIn307M964TqCZhO2zHlgPUOlKDRDYYNg==
X-Received:	by 10.194.109.168 with SMTP id ht8mr18168819wjb.36.1482238991085;
	Tue, 20 Dec 2016 05:03:11 -0800 (PST)
MIME-Version:	1.0
In-Reply-To:	<alpine.DEB.2.00.1612200951240.7286@igor2priv>
References:	<alpine DOT DEB DOT 2 DOT 00 DOT 1612200951240 DOT 7286 AT igor2priv>
From:	"Chad Parker (parker DOT charles AT gmail DOT com) [via geda-user AT delorie DOT com]" <geda-user AT delorie DOT com>
Date:	Tue, 20 Dec 2016 08:03:10 -0500
Message-ID:	<CAJZxidAeHVwHXq9jdvnMF08Z_nXK+tHVf6MLB-YiJrOT4zUJDg@mail.gmail.com>
Subject:	Re: [geda-user] [pcb] bugreport
To:	geda-user AT delorie DOT com
Reply-To:	geda-user AT delorie DOT com
Errors-To:	nobody AT delorie DOT com
X-Mailing-List:	geda-user AT delorie DOT com
X-Unsubscribes-To:	listserv AT delorie DOT com

--047d7bf10b4e893989054416a84e
Content-Type: text/plain; charset=UTF-8

Thanks. I filed the bug report in Launchpad.

On Tue, Dec 20, 2016 at 4:18 AM, <gedau AT igor2 DOT repo DOT hu> wrote:

> 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
>

--047d7bf10b4e893989054416a84e
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Thanks. I filed the bug report in Launchpad.<br></div><div=
 class=3D"gmail_extra"><br><div class=3D"gmail_quote">On Tue, Dec 20, 2016 =
at 4:18 AM,  <span dir=3D"ltr">&lt;<a href=3D"mailto:gedau AT igor2 DOT repo DOT hu" t=
arget=3D"_blank">gedau AT igor2 DOT repo DOT hu</a>&gt;</span> wrote:<br><blockquote c=
lass=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;=
padding-left:1ex">Hi all,<br>
<br>
FlagType is a struct; FLAGS_EQUAL attempts to compare two flags using memcm=
p() on the full struct.<br>
<br>
While this happens to work at the moment on the most common platforms, it&#=
39;s wrong, because the C standard lets the compiler to:<br>
<br>
- insert padding between struct fields (except before the first), to achiev=
e whatever alignment it sees fit, in an &quot;implementation-defined manner=
&quot;<br>
<br>
- may append an &quot;unnamed padding&quot; at the end of the struct, which=
 will be counted in sizeof()<br>
<br>
Thus memcmp() may go and compare potentially uninitialized padding fields, =
saying two equal flags are not equal because of differences in uninitialize=
d bytes.<br>
<br>
Why it works at the moment is: the first field is a long, which is wide eno=
ugh that the second field won&#39;t need alignment on the most common curre=
nt platforms, and MAX_LAYER is 16 so t[] ends up being 8 characters long wh=
ich 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 onl=
y once in the code (minus external plugins) and it probably gets 0-initiali=
zed flag structs all (or most of) the time - I was too lazy to trace this b=
ack.<br>
<br>
It is a trap for future development: if you extend the struct, it may get r=
andom new paddings even on current platforms/compilers, which could break i=
n 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).<br>
<br>
I&#39;ve fixed this in pcb-rnd; I recommend fixing this in mainline too, by=
 comparing the struct field by field.<br>
<br>
(In theory it could be fixed by making sure all bytes of all flag fields ar=
e always 0-initialized, sure these get memset() to 0 all the time even by f=
uture code sounds less safe. But it&#39;s up to mainline devs to decide.)<b=
r>
<br>
Regards,<br>
<br>
Igor2<br>
</blockquote></div><br></div>

--047d7bf10b4e893989054416a84e--

- Raw text -