[Bug analyzer/107582] New: - -Wanalyzer-use-of-uninitialized-value false positive using pthread_cond_timedwait

[Bug analyzer/107582] New: - -Wanalyzer-use-of-uninitialized-value false positive using pthread_cond_timedwait

[dimitri at ouroboros dot rocks]

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

            Bug ID: 107582
           Summary: - -Wanalyzer-use-of-uninitialized-value false positive
                    using pthread_cond_timedwait
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: dimitri at ouroboros dot rocks
  Target Milestone: ---

Created attachment 53852
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53852&action=edit
test case,

[Bug analyzer/107582] - -Wanalyzer-use-of-uninitialized-value false positive using pthread_cond_timedwait

[dimitri at ouroboros dot rocks]

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

--- Comment #1 from dimitri at ouroboros dot rocks ---
Created attachment 53853
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53853&action=edit
failing test case

[Bug analyzer/107582] - -Wanalyzer-use-of-uninitialized-value false positive using pthread_cond_timedwait

[dimitri at ouroboros dot rocks]

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

--- Comment #2 from dimitri at ouroboros dot rocks ---
I've run into a weird false positive for the analyzer that seems to only occur
with pthread_cond_timedwait.

Compile the test file using

gcc -c -fanalyzer test_if_else_pthread.c

This will work fine.

But if the while loop is enabled (which should be there in correct code):

gcc -c -fanalyzer test_if_else_pthread_fp.c

the following false-positive occurs:


    |   38 |         while (z == 0 && ret != ETIMEDOUT)
    |      |                       ^
    |      |                       |
    |      |                       (4) following ‘true’ branch...
    |   39 | #endif
    |   40 |                 ret = pthread_cond_timedwait(&cond, &mutex, &now);
    |      |                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |                       |
    |      |                       (5) ...to here
    |   41 | 
    |   42 |         if (ret != ETIMEDOUT)
    |      |            ~           
    |      |            |
    |      |            (6) following ‘false’ branch (when ‘ret == 110’)...
    |
  ‘main’: event 7
    |
    |   45 |         pthread_cleanup_pop(1);
    |      |         ^~~~~~~~~~~~~~~~~~~
    |      |         |
    |      |         (7) ...to here
    |
  ‘main’: events 8-10
    |
    |   47 |         if (ret == ETIMEDOUT)
    |      |            ^
    |      |            |
    |      |            (8) following ‘false’ branch (when ‘ret != 110’)...
    |......
    |   50 |         printf("x = %d\n", *x);
    |      |         ~~~~~~~~~~~~~~~~~~~~~~
    |      |         |
    |      |         (9) ...to here
    |      |         (10) use of uninitialized value ‘x’ here

Obviously, ret == ETIMEDOUT and ret != ETIMEDOUT can't both be false.

I've tried writing a smaller example using mock functions that randomly return
ETIMEDOUT instead of pthread_cond_timeout and that did not reproduce the false
positive.

[Bug analyzer/107582] - -Wanalyzer-use-of-uninitialized-value false positive using pthread_cond_timedwait

[dimitri at ouroboros dot rocks]

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

dimitri at ouroboros dot rocks changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #53852|0                           |1
        is obsolete|                            |
  Attachment #53853|0                           |1
        is obsolete|                            |

--- Comment #3 from dimitri at ouroboros dot rocks ---
Created attachment 53854
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53854&action=edit
simpler test case

Got a simpler reproducer.

gcc -c -fanalyzer test_if_else_pthread_min.c

also reproduces the issue.

It's probably the pthread_cleanup handlers (macros) with an internal while loop
that cause the issue rather than the pthread_cond_timedwait.

[Bug analyzer/107582] - -Wanalyzer-use-of-uninitialized-value false positive with while loop in pthread_cleanup_push

[dmalcolm at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2022-11-18

--- Comment #4 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Thanks for filing this bug.  Am debugging it now...

[Bug analyzer/107582] - -Wanalyzer-use-of-uninitialized-value false positive with while loop in pthread_cleanup_push

[dmalcolm at gcc dot gnu.org]

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

--- Comment #5 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
It's a bug in feasibility-checking when jumping through a function pointer:
dynamic_call_info_t::update_model blindly copies over the state from the
exploded_node's state, overwriting the precise knowledge of "ret" (which was
known to be == 110) with the UNKNOWN svalue.

[Bug analyzer/107582] - -Wanalyzer-use-of-uninitialized-value false positive with while loop in pthread_cleanup_push

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 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:a7aef0a5a2b7e20048275a29bd80674c1a061a24

commit r13-4158-ga7aef0a5a2b7e20048275a29bd80674c1a061a24
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Fri Nov 18 19:38:25 2022 -0500

    analyzer: fix feasibility false +ve on jumps through function ptrs
[PR107582]

    PR analyzer/107582 reports a false +ve from
    -Wanalyzer-use-of-uninitialized-value where
    the analyzer's feasibility checker erroneously decides
    that point (B) in the code below is reachable, with "x" being
    uninitialized there:

        pthread_cleanup_push(func, NULL);

        while (ret != ETIMEDOUT)
            ret = rand() % 1000;

        /* (A): after the while loop  */

        if (ret != ETIMEDOUT)
          x = &z;

        pthread_cleanup_pop(1);

        if (ret == ETIMEDOUT)
          return 0;

        /* (B): after not bailing out  */

    due to these contradictionary conditions somehow both holding:
      * (ret == ETIMEDOUT), at (A) (skipping the initialization of x), and
      * (ret != ETIMEDOUT), at (B)

    The root cause is that after the while loop, state merger puts ret in
    the exploded graph in an UNKNOWN state, and saves the diagnostic at (B).

    Later, as we explore the feasibilty of reaching the enode for (B),
    dynamic_call_info_t::update_model is called to push/pop the
    frames for handling the call to "func" in pthread_cleanup_pop.
    The "ret" at these nodes in the feasible_graph has a conjured_svalue for
    "ret", and a constraint on it being either == *or* != ETIMEDOUT.

    However dynamic_call_info_t::update_model blithely clobbers the
    model with a copy from the exploded_graph, in which "ret" is UNKNOWN.

    This patch fixes dynamic_call_info_t::update_model so that it
    simulates pushing/popping a frame on the model we're working with,
    preserving knowledge of the constraint on "ret", and enabling the
    analyzer to "know" that the bail-out must happen.

    Doing so fixes the false positive.

    gcc/analyzer/ChangeLog:
            PR analyzer/107582
            * engine.cc (dynamic_call_info_t::update_model): Update the model
            by pushing or pop a frame, rather than by clobbering it with the
            model from the exploded_node's state.

    gcc/testsuite/ChangeLog:
            PR analyzer/107582
            * gcc.dg/analyzer/feasibility-4.c: New test.
            * gcc.dg/analyzer/feasibility-pr107582-1.c: New test.
            * gcc.dg/analyzer/feasibility-pr107582-2.c: New test.

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

[Bug analyzer/107582] - -Wanalyzer-use-of-uninitialized-value false positive with while loop in pthread_cleanup_push

[dmalcolm at gcc dot gnu.org]

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

--- Comment #7 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Fixed on trunk for GCC 13 by the above commit.

I hope to backport this to GCC 12; keeping this open to track that.

[Bug analyzer/107582] - -Wanalyzer-use-of-uninitialized-value false positive with while loop in pthread_cleanup_push

[dmalcolm at gcc dot gnu.org]

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

--- Comment #8 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
(In reply to David Malcolm from comment #7)
> I hope to backport this to GCC 12; keeping this open to track that.

I believe the buggy implementation of dynamic_call_info_t::update_model was
introduced in r12-3002-gaef703cf982072, so GCC 12 is probably the earlier
branch to backport to.

[Bug analyzer/107582] - -Wanalyzer-use-of-uninitialized-value false positive with while loop in pthread_cleanup_push

[dmalcolm at gcc dot gnu.org]

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

--- Comment #9 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
s/earlier/earliest/

[Bug analyzer/107582] - -Wanalyzer-use-of-uninitialized-value false positive with while loop in pthread_cleanup_push

[dimitri at ouroboros dot rocks]

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

--- Comment #10 from dimitri at ouroboros dot rocks ---
thanks for the analysis and the fix!

[Bug analyzer/107582] - -Wanalyzer-use-of-uninitialized-value false positive with while loop in pthread_cleanup_push

[cvs-commit at gcc dot gnu.org]

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

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

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

commit r12-9358-ge7f7483d50069fede8450091449714d122c58fca
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Wed Mar 29 14:16:47 2023 -0400

    analyzer: fix feasibility false +ve on jumps through function ptrs
[PR107582]

    PR analyzer/107582 reports a false +ve from
    -Wanalyzer-use-of-uninitialized-value where
    the analyzer's feasibility checker erroneously decides
    that point (B) in the code below is reachable, with "x" being
    uninitialized there:

        pthread_cleanup_push(func, NULL);

        while (ret != ETIMEDOUT)
            ret = rand() % 1000;

        /* (A): after the while loop  */

        if (ret != ETIMEDOUT)
          x = &z;

        pthread_cleanup_pop(1);

        if (ret == ETIMEDOUT)
          return 0;

        /* (B): after not bailing out  */

    due to these contradictionary conditions somehow both holding:
      * (ret == ETIMEDOUT), at (A) (skipping the initialization of x), and
      * (ret != ETIMEDOUT), at (B)

    The root cause is that after the while loop, state merger puts ret in
    the exploded graph in an UNKNOWN state, and saves the diagnostic at (B).

    Later, as we explore the feasibilty of reaching the enode for (B),
    dynamic_call_info_t::update_model is called to push/pop the
    frames for handling the call to "func" in pthread_cleanup_pop.
    The "ret" at these nodes in the feasible_graph has a conjured_svalue for
    "ret", and a constraint on it being either == *or* != ETIMEDOUT.

    However dynamic_call_info_t::update_model blithely clobbers the
    model with a copy from the exploded_graph, in which "ret" is UNKNOWN.

    This patch fixes dynamic_call_info_t::update_model so that it
    simulates pushing/popping a frame on the model we're working with,
    preserving knowledge of the constraint on "ret", and enabling the
    analyzer to "know" that the bail-out must happen.

    Doing so fixes the false positive.

    Cherrypicked from r13-4158-ga7aef0a5a2b7e2.

    gcc/analyzer/ChangeLog:
            PR analyzer/107582
            * engine.cc (dynamic_call_info_t::update_model): Update the model
            by pushing or pop a frame, rather than by clobbering it with the
            model from the exploded_node's state.

    gcc/testsuite/ChangeLog:
            PR analyzer/107582
            * gcc.dg/analyzer/feasibility-4.c: New test.
            * gcc.dg/analyzer/feasibility-pr107582-1.c: New test.
            * gcc.dg/analyzer/feasibility-pr107582-2.c: New test.

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

[Bug analyzer/107582] - -Wanalyzer-use-of-uninitialized-value false positive with while loop in pthread_cleanup_push

[dmalcolm at gcc dot gnu.org]

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

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

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

--- Comment #12 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Should be fixed on gcc 12 branch by the above (for the eventual gcc 12.3
release); marking as resolved.