Messages in this thread | | | Date | Thu, 3 Mar 2016 14:47:16 +0100 | From | Ingo Molnar <> | Subject | Re: Q: why didn't GCC warn about this uninitialized variable? (was: Re: [PATCH] perf tests: initialize sa.sa_flags) |
| |
* 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
| |