Bug 96651 - gcc 10 -fanalyzer fail to track (static) global variable in a switch
Summary: gcc 10 -fanalyzer fail to track (static) global variable in a switch
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: analyzer (show other bugs)
Version: 10.1.0
: P3 normal
Target Milestone: ---
Assignee: David Malcolm
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-08-17 10:45 UTC by Matthias Gatto
Modified: 2020-08-19 01:30 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2020-08-17 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Gatto 2020-08-17 10:45:21 UTC
When compiling this code with -fanalyzer:

static int a;

int main(void)
{
        char *src = NULL;
        char buf[128];

        switch (a) {
        case 1:
                strcpy(buf, src);
                break;
        case 0:
                strcpy(buf, "hello");
        }
        printf("%s\n", buf);
}

GCC seems to think the code can enter case 1 and use strcpy with a NULL value, but it can't because a is initialize to 0, and isn't touch anywhere.

It also find have the same error if a isn't static.

Note: I've create a small snippet of code that allow to reproduce the error, I've actually encounter the error here: https://github.com/curl/curl/pull/5815 in sws.c
Comment 1 David Malcolm 2020-08-17 19:46:04 UTC
Thanks for filing this.

Note that the analyzer is still experimental.

The issue is that the analyzer is treating the values of globals as arbitrary unknown values, ignoring their initialization values, even for code paths from the entrypoint of "main".

I'm looking at patching it so that code paths from "main" can assume these initial values (until a call to code outside the TU occurs on the path), so that we can assume that a is 0 at the start of main.
Comment 2 GCC Commits 2020-08-19 01:22:39 UTC
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:623bc0276849d48ada5a7a2e3e94bd79de42c3db

commit r11-2754-g623bc0276849d48ada5a7a2e3e94bd79de42c3db
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Mon Aug 17 16:35:10 2020 -0400

    analyzer: consider initializers for globals [PR96651]
    
    PR analyzer/96651 reports a false positive in which a global
    that can't have been touched yet is checked in "main".  The analyzer
    fails to reject code paths in which the initial value of the global
    makes the path condition impossible.
    
    This patch detects cases where the code path begins at the entrypoint
    of "main", and extracts values from initializers for globals that
    can't have been touched yet, rather than using a symbolic
    "INIT_VAL(REG)", fixing the false positive.
    
    gcc/analyzer/ChangeLog:
            PR analyzer/96651
            * region-model.cc (region_model::called_from_main_p): New.
            (region_model::get_store_value): Move handling for globals into...
            (region_model::get_initial_value_for_global): ...this new
            function, and add logic for extracting values from decl
            initializers.
            * region-model.h (decl_region::get_svalue_for_constructor): New
            decl.
            (decl_region::get_svalue_for_initializer): New decl.
            (region_model::called_from_main_p): New decl.
            (region_model::get_initial_value_for_global): New.
            * region.cc (decl_region::maybe_get_constant_value): Move logic
            for getting an svalue from a CONSTRUCTOR node to...
            (decl_region::get_svalue_for_constructor): ...this new function.
            (decl_region::get_svalue_for_initializer): New.
            * store.cc (get_svalue_for_ctor_val): Rewrite in terms of
            region_model::get_rvalue.
            * store.h (binding_cluster::get_map): New accessor.
    
    gcc/testsuite/ChangeLog:
            PR analyzer/96651
            * gcc.dg/analyzer/pr96651-1.c: New test.
            * gcc.dg/analyzer/pr96651-2.c: New test.
Comment 3 David Malcolm 2020-08-19 01:30:08 UTC
Should be fixed by the above patch; marking as resolved.

That said, does this fix the false positives from curl?  In the first example in https://github.com/curl/curl/pull/5815 I see various function calls on the path before the "switch", and if those are in a different source file the analyzer ought to conservatively assume that non-static globals could get written to.