Messages in this thread | | | From | Alexander Potapenko <> | Date | Wed, 19 Feb 2020 18:56:38 +0100 | Subject | Re: [PATCH] lib/test_stackinit: move a local outside the switch statement |
| |
On Wed, Feb 19, 2020 at 6:36 PM Kees Cook <keescook@chromium.org> wrote: > > On Tue, Feb 18, 2020 at 10:48:15AM +0100, glider@google.com wrote: > > Right now CONFIG_INIT_STACK_ALL is unable to initialize locals declared > > in switch statements, see http://llvm.org/PR44916. > > Move the variable declaration outside the switch in lib/test_stackinit.c > > to prevent potential test failures until this is sorted out. > > > > Cc: Kees Cook <keescook@chromium.org> > > Signed-off-by: Alexander Potapenko <glider@google.com> > > Er, no. This test is specifically to catch that case. (i.e. the proposed > GCC version of this feature misses this case too.) We absolutely want > this test to continue to fail until it's fixed: > > [ 65.546670] test_stackinit: switch_1_none FAIL (uninit bytes: 8) > [ 65.547478] test_stackinit: switch_2_none FAIL (uninit bytes: 8) > > What would be nice is if Clang could at least _warn_ about these > conditions. GCC does this in the same situation: > > fs/fcntl.c: In function ‘send_sigio_to_task’: > fs/fcntl.c:738:13: warning: statement will never be executed [-Wswitch-unreachable] > siginfo_t si; > ^~
Is this really a bug from the C Standard point of view? Initializing the switch-local variable or executing other code after the switch would indeed be a problem, but isn't it correct to declare an uninitialized local as long as it's initialized in the branches that use it?
> I have a patch to fix all the switch statement variables, though: > > https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=kspp/gcc-plugin/stackinit&id=35ed32e16e13a86370e4b70991db8d5f771ba898
Am I understanding right that these warnings only show up in the instrumented build? According to the GCC manual:
-Wswitch-unreachable does not warn if the statement between the controlling expression and the first case label is just a declaration
> -Kees > > > --- > > lib/test_stackinit.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/lib/test_stackinit.c b/lib/test_stackinit.c > > index 2d7d257a430e..41e2a6e0cdaa 100644 > > --- a/lib/test_stackinit.c > > +++ b/lib/test_stackinit.c > > @@ -282,9 +282,9 @@ DEFINE_TEST(user, struct test_user, STRUCT, none); > > */ > > static int noinline __leaf_switch_none(int path, bool fill) > > { > > - switch (path) { > > - uint64_t var; > > + uint64_t var; > > > > + switch (path) { > > case 1: > > target_start = &var; > > target_size = sizeof(var); > > -- > > 2.25.0.265.gbab2e86ba0-goog > > > > -- > Kees Cook
-- Alexander Potapenko Software Engineer
Google Germany GmbH Erika-Mann-Straße, 33 80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg
| |