lkml.org 
[lkml]   [2020]   [Feb]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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

\
 
 \ /
  Last update: 2020-02-19 18:57    [W:0.071 / U:0.228 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site