Bug 99671 - RFE: analyzer could complain about ptr derefs that occur before the ptr is checked
Summary: RFE: analyzer could complain about ptr derefs that occur before the ptr is ch...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: analyzer (show other bugs)
Version: 11.0
: P3 normal
Target Milestone: 13.0
Assignee: David Malcolm
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-03-19 15:03 UTC by David Malcolm
Modified: 2024-01-01 02:54 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2022-11-09 00:00:00


Attachments
Implementation of this (not yet ported to Sphinx) (11.68 KB, patch)
2022-11-09 22:26 UTC, David Malcolm
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Malcolm 2021-03-19 15:03:17 UTC
See e.g. bug 80049, where we have code of the form


  expr = p->field;
  [...]
  if (p) ...

We could complain that 'p' has already been dereferenced before the check.
This might be implementable using sm-malloc: the "p->field" dereference transitions "p" to non-null, and we could thus complain at the "if (p)" on non-null if the transition was due to such a transition (rather than being known to be non-null) - or perhaps a new state "assumed-non-null"?

PVS Studio complains about this:
  https://www.viva64.com/en/w/v595/
  https://www.viva64.com/en/examples/v595/
using CWE 476:
  https://cwe.mitre.org/data/definitions/476.html
Comment 1 David Malcolm 2022-11-09 14:00:02 UTC
I have a mostly-working -fanalyzer implementation of this.

I now see that PR 77432 tracks implementing this, albeit for the middle-end.
Comment 2 David Malcolm 2022-11-09 22:26:13 UTC
Created attachment 53863 [details]
Implementation of this (not yet ported to Sphinx)

This patch implements the new warning; still uses texinfo rather than sphinx.  Not yet posted to mailing list; am waiting on dust to settle from the texinfo->sphinx transition on trunk.
Comment 3 GCC Commits 2022-11-10 18:31:06 UTC
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:5c6546ca7d8cab1f1c129f5b55f709e2ceee0f94

commit r13-3884-g5c6546ca7d8cab1f1c129f5b55f709e2ceee0f94
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Thu Nov 10 13:23:56 2022 -0500

    analyzer: new warning: -Wanalyzer-deref-before-check [PR99671]
    
    This patch implements a new -Wanalyzer-deref-before-check within
    -fanalyzer.  It complains about code paths in which a pointer is checked
    for NULL after it has already been dereferenced.
    
    For example, for the testcase in PR 77432 the diagnostic emits:
    deref-before-check-1.c: In function 'test_from_pr77432':
    deref-before-check-1.c:6:8: warning: check of 'a' for NULL after already dereferencing it [-Wanalyzer-deref-before-check]
        6 |     if (a)
          |        ^
      'test_from_pr77432': events 1-2
        |
        |    5 |     int b = *a;
        |      |         ^
        |      |         |
        |      |         (1) pointer 'a' is dereferenced here
        |    6 |     if (a)
        |      |        ~
        |      |        |
        |      |        (2) pointer 'a' is checked for NULL here but it was already dereferenced at (1)
        |
    
    and in PR 77425 we had an instance of this hidden behind a
    macro, which the diagnostic complains about as follows:
    
    deref-before-check-pr77425.c: In function 'get_odr_type':
    deref-before-check-pr77425.c:35:10: warning: check of 'odr_types_ptr' for NULL after already dereferencing it [-Wanalyzer-deref-before-check]
       35 |       if (odr_types_ptr)
          |          ^
      'get_odr_type': events 1-3
        |
        |   27 |   if (cond)
        |      |      ^
        |      |      |
        |      |      (1) following 'false' branch...
        |......
        |   31 |   else if (other_cond)
        |      |           ~~~~~~~~~~~
        |      |           ||
        |      |           |(2) ...to here
        |      |           (3) following 'true' branch...
        |
      'get_odr_type': event 4
        |
        |   11 | #define odr_types (*odr_types_ptr)
        |      |                   ~^~~~~~~~~~~~~~~
        |      |                    |
        |      |                    (4) ...to here
    deref-before-check-pr77425.c:33:7: note: in expansion of macro 'odr_types'
        |   33 |       odr_types[val->id] = 0;
        |      |       ^~~~~~~~~
        |
      'get_odr_type': event 5
        |
        |   11 | #define odr_types (*odr_types_ptr)
        |      |                   ~^~~~~~~~~~~~~~~
        |      |                    |
        |      |                    (5) pointer 'odr_types_ptr' is dereferenced here
    deref-before-check-pr77425.c:33:7: note: in expansion of macro 'odr_types'
        |   33 |       odr_types[val->id] = 0;
        |      |       ^~~~~~~~~
        |
      'get_odr_type': event 6
        |
        |   35 |       if (odr_types_ptr)
        |      |          ^
        |      |          |
        |      |          (6) pointer 'odr_types_ptr' is checked for NULL here but it was already dereferenced at (5)
        |
    
    gcc/analyzer/ChangeLog:
            PR analyzer/99671
            * analyzer.opt (Wanalyzer-deref-before-check): New warning.
            * diagnostic-manager.cc
            (null_assignment_sm_context::set_next_state): Only add state
            change events for transition to "null" state.
            (null_assignment_sm_context::is_transition_to_null): New.
            * engine.cc (impl_region_model_context::on_pop_frame): New.
            * exploded-graph.h (impl_region_model_context::on_pop_frame): New
            decl.
            * program-state.cc (sm_state_map::clear_any_state): New.
            (sm_state_map::can_merge_with_p): New.
            (program_state::can_merge_with_p): Replace requirement that
            sm-states be equal in favor of an attempt to merge them.
            * program-state.h (sm_state_map::clear_any_state): New decl.
            (sm_state_map::can_merge_with_p): New decl.
            * region-model.cc (region_model::eval_condition): Make const.
            (region_model::pop_frame): Call ctxt->on_pop_frame.
            * region-model.h (region_model::eval_condition): Make const.
            (region_model_context::on_pop_frame): New vfunc.
            (noop_region_model_context::on_pop_frame): New.
            (region_model_context_decorator::on_pop_frame): New.
            * sm-malloc.cc (enum resource_state): Add RS_ASSUMED_NON_NULL.
            (allocation_state::dump_to_pp): Drop "final".
            (struct assumed_non_null_state): New subclass.
            (malloc_state_machine::m_assumed_non_null): New.
            (assumed_non_null_p): New.
            (class deref_before_check): New.
            (assumed_non_null_state::dump_to_pp): New.
            (malloc_state_machine::get_or_create_assumed_non_null_state_for_frame):
            New.
            (malloc_state_machine::maybe_assume_non_null): New.
            (malloc_state_machine::on_stmt): Transition from start state to
            "assumed-non-null" state for pointers passed to
            __attribute__((nonnull)) arguments, and for pointers explicitly
            dereferenced.  Call maybe_complain_about_deref_before_check for
            pointers explicitly compared against NULL.
            (malloc_state_machine::maybe_complain_about_deref_before_check):
            New.
            (malloc_state_machine::on_deallocator_call): Also transition
            "assumed-non-null" states to "freed".
            (malloc_state_machine::on_pop_frame): New.
            (malloc_state_machine::maybe_get_merged_states_nonequal): New.
            * sm-malloc.dot: Update for changes to sm-malloc.cc.
            * sm.h (state_machine::on_pop_frame): New.
            (state_machine::maybe_get_merged_state): New.
            (state_machine::maybe_get_merged_states_nonequal): New.
    
    gcc/ChangeLog:
            * doc/gcc/gcc-command-options/options-that-control-static-analysis.rst:
            Add -Wanalyzer-deref-before-check.
    
    gcc/testsuite/ChangeLog:
            * gcc.dg/analyzer/deref-before-check-1.c: New test.
            * gcc.dg/analyzer/deref-before-check-2.c: New test.
            * gcc.dg/analyzer/deref-before-check-pr77425.c: New test.
            * gcc.dg/analyzer/malloc-1.c (test_51): New test.
    
    gcc/ChangeLog:
            PR analyzer/99671
            * tristate.h (tristate::is_unknown): New.
    
    Signed-off-by: David Malcolm <dmalcolm@redhat.com>
Comment 4 David Malcolm 2022-11-10 18:49:36 UTC
Implemented for GCC 13 by the above patch; closing this RFE.