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: References: From: "Chad Parker (parker DOT charles AT gmail DOT com) [via geda-user AT delorie DOT com]" Date: Tue, 20 Dec 2016 08:03:10 -0500 Message-ID: Subject: Re: [geda-user] [pcb] bugreport To: geda-user AT delorie DOT com Content-Type: multipart/alternative; boundary=047d7bf10b4e893989054416a84e 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 Precedence: bulk --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, 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
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 memcm= p() on the full struct.

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:

- insert padding between struct fields (except before the first), to achiev= e 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 uninitialize= d bytes.

Why it works at the moment is: the first field is a long, which is wide eno= ugh that the second field won'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.

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

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 ar= e always 0-initialized, sure these get memset() to 0 all the time even by f= uture code sounds less safe. But it's up to mainline devs to decide.)
Regards,

Igor2

--047d7bf10b4e893989054416a84e--