Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756508AbcCCMcG (ORCPT ); Thu, 3 Mar 2016 07:32:06 -0500 Received: from unicorn.mansr.com ([81.2.72.234]:44738 "EHLO unicorn.mansr.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755239AbcCCMcE convert rfc822-to-8bit (ORCPT ); Thu, 3 Mar 2016 07:32:04 -0500 From: =?iso-8859-1?Q?M=E5ns_Rullg=E5rd?= To: Ingo Molnar Cc: Arnaldo Carvalho de Melo , Peter Zijlstra , Colin King , Ingo Molnar , linux-kernel@vger.kernel.org, Richard Henderson , Jakub Jelinek , Dan Carpenter Subject: Re: Q: why didn't GCC warn about this uninitialized variable? References: <1456923322-29697-1-git-send-email-colin.king@canonical.com> <20160302125901.GF6356@twins.programming.kicks-ass.net> <20160302130350.GO3604@kernel.org> <20160302132127.GG6356@twins.programming.kicks-ass.net> <20160302132323.GP3604@kernel.org> <20160303121944.GB2484@gmail.com> Date: Thu, 03 Mar 2016 12:31:59 +0000 In-Reply-To: <20160303121944.GB2484@gmail.com> (Ingo Molnar's message of "Thu, 3 Mar 2016 13:19:44 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3087 Lines: 81 Ingo Molnar writes: > * Arnaldo Carvalho de Melo wrote: > >> Em Wed, Mar 02, 2016 at 02:21:27PM +0100, Peter Zijlstra escreveu: >> > On Wed, Mar 02, 2016 at 10:03:50AM -0300, Arnaldo Carvalho de Melo wrote: >> > > > Would not something like: >> > > > >> > > > sa = (struct sigaction){ >> > > > .sa_sigaction = segfault_handler, >> > > > }; >> > > > sigfillset(&sa.sa_mask); >> > > > >> > > > Be better? >> > > >> > > I thought about that, but isn't that set in stone? This would be a 4 >> > > liner, while his is a one' :-) >> > >> > Dunno, you're right that its rather unlikely struct sigaction is going >> > to grow another member, but I like the above pattern better in general, >> > makes it harder to end up with uninitalized bits. >> > >> > When performance matters the above pattern isn't ideal, but that should >> > not be a concern here. >> >> Right, I also always use : >> >> >> struct foo bar = { >> .baz = 1, >> .name = "whatever", >> }; >> >> Even more compact than using that cast. But didn't bother changing in >> this case. > > So the source of the bug was: > > struct sigaction sa; > > ... > > sigfillset(&sa.sa_mask); > sa.sa_sigaction = segfault_handler; > sigaction(SIGSEGV, &sa, NULL); > > ... which uninitialized sa.sa_flags field GCC merrily accepted as > proper C code, despite us turning on essentially _all_ GCC warnings > for the perf build that exist under the sun: > > gcc -Wbad-function-cast -Wdeclaration-after-statement -Wformat-security -Wformat-y2k \ > -Winit-self -Wmissing-declarations -Wmissing-prototypes -Wnested-externs \ > -Wno-system-headers -Wold-style-definition -Wpacked -Wredundant-decls \ > -Wshadow -Wstrict-aliasing=3 -Wstrict-prototypes -Wswitch-default -Wswitch-enum \ > -Wundef -Wwrite-strings -Wformat \ > -Werror -O6 -fno-omit-frame-pointer -ggdb3 -funwind-tables -Wall -Wextra -std=gnu99 -fstack-protector-all -D_FORTIFY_SOURCE=2 > > This is a _trivial_ uninitialized variable bug, yet GCC never warned > about it. Why? > > People build perf with a wide range of GCC versions, from old ones to > trunk. I cannot believe it that none of those GCC versions warned > about this trivial looking bug! > > And yes, I know that unitialized structures on the stack are valid C > code, yet it's one of the most fragile aspects of C and it was the > source of countless security holes in the past... Passing a pointer to an uninitialised object is typically not warned about since the purpose of the call might be to initialise it in the first place. Now the second argument of sigaction() is a pointer to const, so the compiler should be able to see that this isn't the case. Maybe it's not warning because some fields in the struct are initialised and the function, as far as the compiler knows, might only be accessing those. (There's certainly code out there using that pattern.) If this is the case here, a flag to warn unless the object is fully initialised would be useful to catch bugs like this. -- M?ns Rullg?rd