[Bug analyzer/103217] New: analyzer false positive on leak warning when using indirect strdup

[Bug analyzer/103217] New: analyzer false positive on leak warning when using indirect strdup

[npfhrotynz-ptnqh.myvf at noclue dot notk.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103217

            Bug ID: 103217
           Summary: analyzer false positive on leak warning when using
                    indirect strdup
           Product: gcc
           Version: 11.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: npfhrotynz-ptnqh.myvf at noclue dot notk.org
  Target Milestone: ---

Inlining both the reproducer and fanalyzer warning as they are small enough:

----------
#include <getopt.h>
#include <stdlib.h>
#include <string.h>

char *xstrdup(const char *src) {
        char *val = strdup(src);
        if (!val)
                abort();
        return val;
}

int main(int argc, char *argv[]) {
        char *one = NULL, *two = NULL;
        int rc;

        while ((rc = getopt(argc, argv, "a:b:")) != -1) {
                switch (rc) {
                case 'a':
                        free(one);
                        one = xstrdup(optarg);
                        break;
                case 'b':
                        free(two);
                        two = xstrdup(optarg);
                        break;
                }
        }
        free(one);
        free(two);
        return 0;
}
----------

----------
$ gcc -fanalyzer -o t t.c
t.c: In function ‘main’:
cc1: warning: leak of ‘val’ [CWE-401] [-Wanalyzer-malloc-leak]
  ‘main’: events 1-4
    |
    |t.c:13:5:
    |   13 | int main(int argc, char *argv[]) {
    |      |     ^~~~
    |      |     |
    |      |     (1) entry to ‘main’
    |......
    |   17 |         while ((rc = getopt(argc, argv, "a:b:")) != -1) {
    |      |                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |                                                  |
    |      |                                                  (2) following
‘true’ branch (when ‘rc != -1’)...
    |   18 |                 switch (rc) {
    |      |                 ~~~~~~
    |      |                 |
    |      |                 (3) ...to here
    |......
    |   25 |                         two = xstrdup(optarg);
    |      |                               ~~~~~~~~~~~~~~~
    |      |                               |
    |      |                               (4) calling ‘xstrdup’ from ‘main’
    |
    +--> ‘xstrdup’: events 5-9
           |
           |    6 | char *xstrdup(const char *src) {
           |      |       ^~~~~~~
           |      |       |
           |      |       (5) entry to ‘xstrdup’
           |    7 |         char *val = strdup(src);
           |      |                     ~~~~~~~~~~~
           |      |                     |
           |      |                     (6) allocated here
           |    8 |         if (!val)
           |      |            ~
           |      |            |
           |      |            (7) assuming ‘val’ is non-NULL
           |      |            (8) following ‘false’ branch (when ‘val’ is
non-NULL)...
           |    9 |                 abort();
           |   10 |         return val;
           |      |                ~~~
           |      |                |
           |      |                (9) ...to here
           |
    <------+
    |
  ‘main’: event 10
    |
    |   25 |                         two = xstrdup(optarg);
    |      |                               ^~~~~~~~~~~~~~~
    |      |                               |
    |      |                               (10) returning to ‘main’ from
‘xstrdup’
    |
  ‘main’: event 11
    |
    |cc1:
    | (11): ‘val’ leaks here; was allocated at (6)
    |
----------

As far as I see the conditions seem to be:
 - there have to be at least two cases and two variables, adding more than two
cases leave the error only on second one; interverting the two makes the error
stay on 2nd. Similarly, using the same variable in both cases makes the error
go away.
 - it has to be strdup, replacing strdup with malloc makes the error go away.
 - it has to be indirected, calling strdup() directly in main (with the same
check/abort) makes the error go away. explicit "inline" attribute does not
change behaviour.
 - it doesn't have to be getopt, but there has to be a function call e.g.
replacing getopt() with a locally defined iteration function keeps the error,
but checking argc/argv directly in the loop makes the error disappear

What's also interesting is the event 11 and "(11): ‘val’ leaks here" that
points to... nothing at all? There's no line number or any code quoted to refer
to. I'm a bit at a loss as to what this could mean, exiting from main? where?

[Bug analyzer/103217] analyzer false positive on leak warning when using indirect strdup

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103217

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2021-11-19
     Ever confirmed|0                           |1

--- Comment #1 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Thanks for filing this; I'm testing a fix.

[Bug analyzer/103217] analyzer false positive on leak warning when using indirect strdup

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103217

--- Comment #2 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:f573d35147ca8433c102e1721d8c99fc432cb44b

commit r12-5424-gf573d35147ca8433c102e1721d8c99fc432cb44b
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Thu Nov 18 15:23:30 2021 -0500

    analyzer: fix false leak due to overeager state merging [PR103217]

    PR analyzer/103217 reports a false positive from -Wanalyzer-malloc-leak.

    The root cause is due to overzealous state merger, where the
    state-merging code decided to merge these two states by merging
    the stores:

    state A:
      clusters within frame: âmainâ@1
        cluster for: one_3: CONJURED(val_4 = strdup (src_2(D));, val_4)
        cluster for: two_4: UNKNOWN(char *)
        cluster for: one_21: CONJURED(val_4 = strdup (src_2(D));, val_4)

    state B:
      clusters within frame: âmainâ@1
        cluster for: one_3: UNKNOWN(char *)
        cluster for: two_4: CONJURED(val_4 = strdup (src_2(D));, val_4)
        cluster for: two_18: CONJURED(val_4 = strdup (src_2(D));, val_4)

    into:
      clusters within frame: âmainâ@1
        cluster for: one_3: UNKNOWN(char *)
        cluster for: two_4: UNKNOWN(char *)
        cluster for: one_21: UNKNOWN(char *)
        cluster for: two_18: UNKNOWN(char *)

    despite "CONJURED(val_4 = strdup (src_2(D));, val_4)" having sm-state,
    in this case malloc:nonnull ({free}), thus leading to both references
    to the conjured svalue being lost at merger.

    This patch tweaks the state merger code so that it will not consider
    merging two different svalues for the value of a region if either svalue
    has non-purgable sm-state (in the above example, malloc:nonnull).  This
    fixes the false leak report above.

    Doing so uncovered an issue with explode-2a.c in which the warnings
    moved from the correct location to the "while" stmt.  This turned out
    to be a missing call to detect_leaks in phi-handling, which the patch
    also fixes (in the PK_BEFORE_SUPERNODE case in
    exploded_graph::process_node).  Doing this fixed the regression in
    explode-2a.c and also fixed the location of the leak warning in
    explode-1.c.

    The other side effect of the change is that pr94858-1.c now emits
    a -Wanalyzer-too-complex warning, since pertinent state is no longer
    being thrown away.  There doesn't seem to be a good way of avoiding
    this, so the patch also adds -Wno-analyzer-too-complex to that test
    case (restoring the default).

    gcc/analyzer/ChangeLog:
            PR analyzer/103217
            * engine.cc (exploded_graph::get_or_create_node): Pass in
            m_ext_state to program_state::can_merge_with_p.
            (exploded_graph::process_worklist): Likewise.
            (exploded_graph::maybe_process_run_of_before_supernode_enodes):
            Likewise.
            (exploded_graph::process_node): Add missing call to detect_leaks
            when handling phi nodes.
            * program-state.cc (program_state::can_merge_with_p): Add
            "ext_state" param.  Pass it and state ptrs to
            region_model::can_merge_with_p.
            (selftest::test_program_state_merging): Update for new ext_state
            param of program_state::can_merge_with_p.
            (selftest::test_program_state_merging_2): Likewise.
            * program-state.h (program_state::can_purge_p): Make const.
            (program_state::can_merge_with_p): Add "ext_state" param.
            * region-model.cc: Include "analyzer/program-state.h".
            (region_model::can_merge_with_p): Add params "ext_state",
            "state_a", and "state_b", use them when creating model_merger
            object.
            (model_merger::mergeable_svalue_p): New.
            * region-model.h (region_model::can_merge_with_p): Add params
            "ext_state", "state_a", and "state_b".
            (model_merger::model_merger) Likewise, initializing new fields.
            (model_merger::mergeable_svalue_p): New decl.
            (model_merger::m_ext_state): New field.
            (model_merger::m_state_a): New field.
            (model_merger::m_state_b): New field.
            * svalue.cc (svalue::can_merge_p): Call
            model_merger::mergeable_svalue_p on both states and reject the
            merger accordingly.

    gcc/testsuite/ChangeLog:
            PR analyzer/103217
            * gcc.dg/analyzer/explode-1.c: Update for improvement to location
            of leak warning.
            * gcc.dg/analyzer/pr103217.c: New test.
            * gcc.dg/analyzer/pr94858-1.c: Add -Wno-analyzer-too-complex.

    Signed-off-by: David Malcolm <dmalcolm@redhat.com>

[Bug analyzer/103217] analyzer false positive on leak warning when using indirect strdup

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103217

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #3 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Should be fixed by the above patch on trunk for GCC 12.

[Bug analyzer/103217] analyzer false positive on leak warning when using indirect strdup

[npfhrotynz-ptnqh.myvf at noclue dot notk.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103217

Dominique Martinet <npfhrotynz-ptnqh.myvf at noclue dot notk.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|FIXED                       |---
             Status|RESOLVED                    |REOPENED

--- Comment #4 from Dominique Martinet <npfhrotynz-ptnqh.myvf at noclue dot notk.org> ---
Thanks for the fix!
I've compiled gcc from master and can confirm this fixes the warning for the
example I provided... But unfortunately it doesn't fix it for my real
application, so my reproducer was simplified too much :/

(It fails on this file:
https://github.com/cea-hpc/coordinatool/blob/06c5e319e3c934256d51c2af3ba5f7551bac4bff/copytool/coordinatool.c
, but this project requires lustre development headers to compile so it's not a
good reproducer)


Here's a new attempt, this also gives a similar warning without a precise
location on gcc master:
-------
#include <getopt.h>
#include <stdlib.h>
#include <string.h>

char *xstrdup(const char *src) {
        char *val = strdup(src);
        if (!val)
                abort();
        return val;
}

struct test {
        char *one, *two;
};

int main(int argc, char *argv[]) {
        struct test *options = calloc(1, sizeof(*options));
        int rc;
        if (!options)
                abort();

        while ((rc = getopt(argc, argv, "a:b:")) != -1) {
                switch (rc) {
                case 'a':
                        free(options->one);
                        options->one = xstrdup(optarg);
                        break;
                case 'b':
                        free(options->two);
                        options->two = xstrdup(optarg);
                        break;
                }
        }
        free(options->one);
        free(options->two);
        free(options);
        return 0;
}
-------


----
In function ‘main’:
cc1: warning: leak of ‘<unknown>’ [CWE-401] [-Wanalyzer-malloc-leak]
  ‘main’: events 1-2
    |
    |/tmp/t4.c:16:5:
    |   16 | int main(int argc, char *argv[]) {
    |      |     ^~~~
    |      |     |
    |      |     (1) entry to ‘main’
    |......
    |   19 |         if (!options)
    |      |            ~
    |      |            |
    |      |            (2) following ‘false’ branch (when ‘options’ is
non-NULL)...
    |
  ‘main’: event 3
    |
    |cc1:
    | (3): ...to here
    |
  ‘main’: events 4-6
    |
    |   22 |         while ((rc = getopt(argc, argv, "a:b:")) != -1) {
    |      |                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
    |      |                                                  |
    |      |                                                  (4) following
‘true’ branch (when ‘rc != -1’)...
    |   23 |                 switch (rc) {
    |      |                 ~~~~~~                            
    |      |                 |
    |      |                 (5) ...to here
    |......
    |   30 |                         options->two = xstrdup(optarg);
    |      |                                        ~~~~~~~~~~~~~~~
    |      |                                        |
    |      |                                        (6) calling ‘xstrdup’ from
‘main’
    |
    +--> ‘xstrdup’: events 7-11
           |
           |    5 | char *xstrdup(const char *src) {
           |      |       ^~~~~~~
           |      |       |
           |      |       (7) entry to ‘xstrdup’
           |    6 |         char *val = strdup(src);
           |      |                     ~~~~~~~~~~~
           |      |                     |
           |      |                     (8) allocated here
           |    7 |         if (!val)
           |      |            ~
           |      |            |
           |      |            (9) assuming ‘val’ is non-NULL
           |      |            (10) following ‘false’ branch (when ‘val’ is
non-NULL)...
           |    8 |                 abort();
           |    9 |         return val;
           |      |                ~~~
           |      |                |
           |      |                (11) ...to here
           |
    <------+
    |
  ‘main’: event 12
    |
    |   30 |                         options->two = xstrdup(optarg);
    |      |                                        ^~~~~~~~~~~~~~~
    |      |                                        |
    |      |                                        (12) returning to ‘main’
from ‘xstrdup’
    |
  ‘main’: event 13
    |
    |cc1:
    | (13): ‘<unknown>’ leaks here; was allocated at (8)
    |
----

I'm not sure it's accurate enough for my usecase though: in my project the
"option" struct is declared on the stack and not dynamically allocated... and I
cannot reproduce with "struct test option = { 0 };" so I'm not sure where this
is different; I'll try a bit more and update this ticket if I find a better
reproducer.

(What do you prefer to move forward -- I've tried reopening the bug but you
really fixed something so should I open a new bz instead and keep this
resolved?)

[Bug analyzer/103217] analyzer false positive on leak warning when using indirect strdup

[npfhrotynz-ptnqh.myvf at noclue dot notk.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103217

--- Comment #5 from Dominique Martinet <npfhrotynz-ptnqh.myvf at noclue dot notk.org> ---
Ah, this apparently needed the unused fields in the struct, and the extra
config_init() call.

Here's something trimmed down from the actual program instead of building back
up:
-----
#define _POSIX_C_SOURCE 200809L

#include <getopt.h>
#include <stdlib.h>
#include <string.h>

struct state {
  const char *confpath;
  const char *host;
  const char *port;
  const char *state_dir_prefix;
};

static inline char *xstrdup(const char *s) { 
        char *val = strdup(s);
        if (!val)
                abort();
        return val;
}

int config_init(struct state *config);

int main(int argc, char *argv[]) { 
        int rc;
        struct state state = { 0 };

        config_init(&state);

        while ((rc = getopt(argc, argv, "H:p:")) != -1) { 
                switch (rc) { 
                case 'H':
                        free((void*)state.host);
                        state.host = xstrdup(optarg);
                        break;
                case 'p':
                        free((void*)state.port);
                        state.port = xstrdup(optarg);
                        break;
                } 
        } 

        free((void*)state.host);
        free((void*)state.port);
        return rc;
}
-----

-----
 /opt/gcc/bin/gcc -fanalyzer -c ../copytool/coordinatool.c
In function ‘main’:
cc1: warning: leak of ‘<unknown>’ [CWE-401] [-Wanalyzer-malloc-leak]
  ‘main’: events 1-4
    |
    |../copytool/coordinatool.c:25:5:
    |   25 | int main(int argc, char *argv[]) {
    |      |     ^~~~
    |      |     |
    |      |     (1) entry to ‘main’
    |......
    |   31 |         while ((rc = getopt(argc, argv, "H:p:")) != -1) {
    |      |                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |                                                  |
    |      |                                                  (2) following
‘true’ branch (when ‘rc != -1’)...
    |   32 |                 switch (rc) {
    |      |                 ~~~~~~
    |      |                 |
    |      |                 (3) ...to here
    |......
    |   39 |                         state.port = xstrdup(optarg);
    |      |                                      ~~~~~~~~~~~~~~~
    |      |                                      |
    |      |                                      (4) calling ‘xstrdup’ from
‘main’
    |
    +--> ‘xstrdup’: events 5-9
           |
           |   16 | static inline char *xstrdup(const char *s) {
           |      |                     ^~~~~~~
           |      |                     |
           |      |                     (5) entry to ‘xstrdup’
           |   17 |         char *val = strdup(s);
           |      |                     ~~~~~~~~~
           |      |                     |
           |      |                     (6) allocated here
           |   18 |         if (!val)
           |      |            ~         
           |      |            |
           |      |            (7) assuming ‘val’ is non-NULL
           |      |            (8) following ‘false’ branch (when ‘val’ is
non-NULL)...
           |   19 |                 abort();
           |   20 |         return val;
           |      |                ~~~   
           |      |                |
           |      |                (9) ...to here
           |
    <------+
    |
  ‘main’: event 10
    |
    |   39 |                         state.port = xstrdup(optarg);
    |      |                                      ^~~~~~~~~~~~~~~
    |      |                                      |
    |      |                                      (10) returning to ‘main’ from
‘xstrdup’
    |
  ‘main’: event 11
    |
    |cc1:
    | (11): ‘<unknown>’ leaks here; was allocated at (6)
    |
-----

Thanks!

[Bug analyzer/103217] analyzer false positive on leak warning when using indirect strdup

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103217

--- Comment #6 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
(In reply to Dominique Martinet from comment #4)
[...snip...]

Thanks for re-testing it, and the new test cases.

> (What do you prefer to move forward -- I've tried reopening the bug but you
> really fixed something so should I open a new bz instead and keep this
> resolved?)

For now, let's just reuse this bug ID, to keep the conversation in one place,
and since the reproducers have such similarity (regardless of whether the
underlying causes are different).

[Bug analyzer/103217] analyzer false positive on leak warning when using indirect strdup

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103217

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |ASSIGNED

--- Comment #7 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Am testing a fix...

[Bug analyzer/103217] analyzer false positive on leak warning when using indirect strdup

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103217

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:132902177138c09803d639e12b1daebf2b9edddc

commit r12-5585-g132902177138c09803d639e12b1daebf2b9edddc
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Mon Nov 29 11:47:47 2021 -0500

    analyzer: further false leak fixes due to overzealous state merging
[PR103217]

    Commit r12-5424-gf573d35147ca8433c102e1721d8c99fc432cb44b fixed a false
    positive from -Wanalyzer-malloc-leak due to overzealous state merging,
    erroneously merging two different svalues bound to a particular part
    of the store when one has sm-state.

    A further case was discovered by the reporter of PR analyzer/103217,
    which this patch fixes.  In this variant, different states have set
    different fields of a struct, and on attempting to merge them, the
    states have a different set of binding keys, leading to one state
    having an svalue with sm-state, and its peer state having a NULL value
    for that binding key.  The state merger code was erroneously treating
    them as mergeable to "UNKNOWN".  This followup patch fixes things by
    rejecting such mergers if the non-NULL svalue is not mergeable with
    "UNKNOWN".

    gcc/analyzer/ChangeLog:
            PR analyzer/103217
            * store.cc (binding_cluster::can_merge_p): For the "key is bound"
            vs "key is not bound" merger case, check that the bound svalue
            is mergeable before merging it to "unknown", rejecting the merger
            otherwise.

    gcc/testsuite/ChangeLog:
            PR analyzer/103217
            * gcc.dg/analyzer/pr103217-2.c: New test.
            * gcc.dg/analyzer/pr103217-3.c: New test.
            * gcc.dg/analyzer/pr103217-4.c: New test.
            * gcc.dg/analyzer/pr103217-5.c: New test.

    Signed-off-by: David Malcolm <dmalcolm@redhat.com>

[Bug analyzer/103217] analyzer false positive on leak warning when using indirect strdup

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103217

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #9 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
The cases in comment #4 and comment #5 should be fixed by the above commit;
marking this as RESOLVED.  Please reopen it if this still doesn't fix the false
positive on your application.

[Bug analyzer/103217] analyzer false positive on leak warning when using indirect strdup

[npfhrotynz-ptnqh.myvf at noclue dot notk.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103217

--- Comment #10 from Dominique Martinet <npfhrotynz-ptnqh.myvf at noclue dot notk.org> ---
Thank you for this work! I can confirm this indeed fixed the warning in my
application as well, time to add a fanalyzer build to my CI :)

[Bug analyzer/103217] analyzer false positive on leak warning when using indirect strdup

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103217

--- Comment #11 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Excellent!  Thanks for the feedback.