Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

smatch misidentifies uninitialized variable after switch with no default: #3

Open
jbrandeb opened this issue Dec 21, 2021 · 1 comment

Comments

@jbrandeb
Copy link

I've got a simple reproducer for the issue that I found when scanning our ice driver in the kernel with smatch.

original repro against smatch HEAD 0951ed5 ("db: fix uninitialized variable false positives")
smatch reported:
~/git/smatch/smatch_scripts/kchecker drivers/net/ethernet/intel/ice/ice_ptp_hw.c
drivers/net/ethernet/intel/ice/ice_ptp_hw.c:2852 ice_ptp_port_cmd_e810() error: uninitialized symbol 'cmd_val'.

Below is a simple c-code reproducer, compile with:
gcc -o srt -Wextra -Wall smatch_switch_repro.c
see error with

~/git/smatch/smatch smatch_switch_repro.c
smatch_switch_repro.c:43 badfunc() error: uninitialized symbol 'my_int'.

One bit of data that might be useful: it works fine with badfunc content inline in main() and fails when badfunc is a function with the enum argument. It also succeeds when there is a "default:" label and a simple assignment in that case (see the reproducer below and bit of commented out code)

// SPDX-License-Identifier: BSD-3-Clause
/*
 * Copyright 2021, Intel Corporation
 *
 * A quick demo of a smatch false positive
 */

#include <stdio.h>

enum three_values
{
        value_one,
        value_two,
        value_three
};

void badfunc(const enum three_values cmd)
{
        unsigned int my_int, new_int; //uninitialized

        switch (cmd) {
        case value_one:
                printf("one\n");
                my_int = 1;
                break;
        case value_two:
                printf("two\n");
                my_int = 2;
                break;
        case value_three:
                printf("three\n");
                my_int = 3;
                break;
        /* no default because all enum values handled, which has value
         * to developers because it forces compile error if not all enum values
         * handled and enum is changed */
        //default:
                //my_int = 4;
                //break;
        }

        new_int = 0;
        new_int |= my_int;

        printf("data: %d\n", new_int);
}

int main(int argc __attribute__((unused)), char **argv __attribute__((unused)))
{
        enum three_values my_enum = value_two;

        badfunc(my_enum);

        return 0;
}

@jbrandeb
Copy link
Author

@error27 - this one worked out pretty well, I hope this simple reproducer helps you figure out a fix.

error27 pushed a commit that referenced this issue Apr 21, 2022
Unrelated fix: In match_memdup() then I added some add_dereference(left/right)
calls.

Problem: This codes a tangled mess and zeroing memory does not work.

This code is supposed to handle three things.  #1 Copying a known struct
to a different struct of the same type.  #2 Copying unknown data to a
struct.  #3 Copying a zeroed buffer to a struct.

I think #1 basically works.  It's hard to get #2 wrong so I think that
works but probably in the wrong way.  But #3 was not working.

In the original code, it treated "struct = struct" as different from
memcpying.  Which is sort of not a bad idea, but not how it's implemented.
So get rid of that.  Just say COPY_NORMAL.  Use COPY_UNKNOWN for #2 of
an unknown buffer and COPY_ZERO for a zeroed buffer.

Signed-off-by: Dan Carpenter <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant