Date: Thu, 3 Mar 2016 14:47:16 +0100
From: Ingo Molnar <mingo@kernel.org>
To: Jakub Jelinek <jakub@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Colin King <colin.king@canonical.com>, Ingo Molnar <mingo@redhat.com>,
        linux-kernel@vger.kernel.org, Richard Henderson <rth@twiddle.net>,
        Dan Carpenter <dan.carpenter@oracle.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Andrew Morton <akpm@linux-foundation.org>
Subject: Re: Q: why didn't GCC warn about this uninitialized variable? (was:
 Re: [PATCH] perf tests: initialize sa.sa_flags)
Message-ID: <20160303134716.GA9860@gmail.com>
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>
 <20160303125542.GD3017@tucnak.redhat.com>
 <20160303132433.GA9460@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20160303132433.GA9460@gmail.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4038
Lines: 143


* Ingo Molnar <mingo@kernel.org> wrote:

> So it's all highly inefficient and fragile.
> 
> There's also another cost, the cost of finding the bugs themselves - for example 
> here's a recent upstream kernel fix:
> 
>   commit e01d8718de4170373cd7fbf5cf6f9cb61cebb1e9
>   Author: Peter Zijlstra <peterz@infradead.org>
>   Date:   Wed Jan 27 23:24:29 2016 +0100
> 
>     perf/x86: Fix uninitialized value usage
>     
>     When calling intel_alt_er() with .idx != EXTRA_REG_RSP_* we will not
>     initialize alt_idx and then use this uninitialized value to index an
>     array.
>     
>     When that is not fatal, it can result in an infinite loop in its
>     caller __intel_shared_reg_get_constraints(), with IRQs disabled.
>     
>     Alternative error modes are random memory corruption due to the
>     cpuc->shared_regs->regs[] array overrun, which manifest in either
>     get_constraints or put_constraints doing weird stuff.
>     
>     Only took 6 hours of painful debugging to find this. Neither GCC nor
>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>     Smatch warnings flagged this bug.
>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
>   --- a/arch/x86/kernel/cpu/perf_event_intel.c
>   +++ b/arch/x86/kernel/cpu/perf_event_intel.c
>   @@ -1960,7 +1960,8 @@ intel_bts_constraints(struct perf_event *event)
>  
>    static int intel_alt_er(int idx, u64 config)
>    {
>   -       int alt_idx;
>   +       int alt_idx = idx;
>   +
>           if (!(x86_pmu.flags & PMU_FL_HAS_RSP_1))
>                   return idx;
> 
> 6 hours of PeterZ time translates to quite a bit of code restructuring overhead to 
> eliminate false positive warnings...

Btw., here's the wider context of that bug:

static int intel_alt_er(int idx, u64 config)
{
        int alt_idx;

        if (!(x86_pmu.flags & PMU_FL_HAS_RSP_1))
                return idx;

        if (idx == EXTRA_REG_RSP_0)
                alt_idx = EXTRA_REG_RSP_1;

        if (idx == EXTRA_REG_RSP_1)
                alt_idx = EXTRA_REG_RSP_0;

        if (config & ~x86_pmu.extra_regs[alt_idx].valid_mask)
                return idx;

        return alt_idx;
}

so it's a straightforward uninitialized variable bug.

I tried to distill a testcase out of it, and the following silly hack seems to 
trigger it:

------------------------------->
#include <stdio.h>

#define PMU_FL_HAS_RSP_1 1
#define EXTRA_REG_RSP_1 2
#define EXTRA_REG_RSP_0 4

int global_flags = -1;

static int intel_alt_er(int idx, unsigned long long config)
{
        int alt_idx;
	int uninitialized = 1;

	printf("idx: %d, config: %Ld\n", idx, config);

        if (!(global_flags & PMU_FL_HAS_RSP_1))
                return idx;

        if (idx == EXTRA_REG_RSP_0) {
                alt_idx = EXTRA_REG_RSP_1;
		uninitialized = 0;
	}

        if (idx == EXTRA_REG_RSP_1) {
                alt_idx = EXTRA_REG_RSP_0;
		uninitialized = 0;
	}

        if (config & ~0xff)
                return idx;

	if (uninitialized)
		printf("ugh, using uninitialized alt_idx (%d)!\n", alt_idx);

        return alt_idx;
}

int main(int argc, char **argv)
{
	argv++;

	return intel_alt_er(argc, argc);
}
<-------------------------------

built with:

 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 \
     -o uninitialized uninitialized.c

gives:

 triton:~> ./uninitialized 1
 idx: 2, config: 2

 triton:~> ./uninitialized 0 0
 idx: 3, config: 3
 ugh, using uninitialized alt_idx (2)!

I.e. I cannot get GCC to warn about this seemingly trivial bug, using:

  gcc version 5.2.1 20151010 (Ubuntu 5.2.1-22ubuntu2) 

Thanks,

	Ingo