On Wed, May 04, 2016 at 11:37:05PM +0100, Olly Betts wrote: > On Wed, May 04, 2016 at 05:53:20PM +0100, Colin Watson wrote: > > A more complete fix would be to make sure that structures are always > > zero-filled when they are allocated, but that probably requires work > > throughout the codebase. > > The new settings are always initialised from the existing settings: > > pcsNew = osnew(settings); > *pcsNew = *pcs; /* copy contents */ > > So the root cause is that begin_survey is never actually initialised in > the first place.
Ah yes, I managed to read past that. > I suspect the compiler is doing that for us on some architectures (some > fields are explicitly initialised to NULL or 0 so it probably decides a > memset() of the whole structure is more efficient), but not on hurd-i386 > for some reason, and that's why valgrind doesn't pick this up on x86-64. I actually encountered this on amd64 and valgrind picked it up there. There may be differences between toolchain versions; this was on Ubuntu yakkety, our current development series. I only guessed that it might be the same as this Hurd issue; unfortunately the log isn't detailed enough for me to be sure of that. valgrind's data flow analysis is probably managing to notice that the original pcs structure that is being copied doesn't have an initialised begin_survey, and indeed the code in src/cavern.c:main that allocates the first structure in the list only initialises some fields, so as you say a memset there would be a good idea. Still, something like my patch is likely still a good idea to make the behaviour clearer in error cases, especially as it results in shorter code. :-) > I'll push a fix upstream and let it flow back in the next release, but > if the hurd-i386 issue is a problem for you, feel free to NMU (the patch > as-is looks good to me for that). Tobias, perhaps you could see if this patch fixes the Hurd issue? If not, apologies for dogpiling onto this bug. Thanks, -- Colin Watson [cjwat...@ubuntu.com]
