Subject: Re: Q: why didn't GCC warn about this uninitialized variable?
To: Ingo Molnar <mingo@kernel.org>, Arnaldo Carvalho de Melo <acme@kernel.org>
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>
Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>,
        linux-kernel@vger.kernel.org, Richard Henderson <rth@twiddle.net>,
        Jakub Jelinek <jakub@redhat.com>,
        Dan Carpenter <dan.carpenter@oracle.com>
From: Colin Ian King <colin.king@canonical.com>
Message-ID: <56D82D2F.5000900@canonical.com>
Date: Thu, 3 Mar 2016 12:25:19 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.5.1
MIME-Version: 1.0
In-Reply-To: <20160303121944.GB2484@gmail.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2773
Lines: 79

On 03/03/16 12:19, Ingo Molnar wrote:
> 
> * Arnaldo Carvalho de Melo <acme@kernel.org> 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!

I'm only finding these kind of bugs through use of various tools such as
CoverityScan, cppcheck, smatch, etc.  It is quite amazing how such bugs
don't get picked up by GCC.  The downside is that there are quite a few
false positives to work through, so this is tedious work to separate out
the wheat from the chaff.

> 
> 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...
> 
> Thanks,
> 
> 	Ingo
>