@@ -58,6 +58,10 @@ Wanalyzer-allocation-size
Common Var(warn_analyzer_allocation_size) Init(1) Warning
Warn about code paths in which a pointer to a buffer is assigned to an incompatible type.
+Wanalyzer-deref-before-check
+Common Var(warn_analyzer_deref_before_check) Init(1) Warning
+Warn about code paths in which a pointer is checked for NULL after it has already been dereferenced.
+
Wanalyzer-double-fclose
Common Var(warn_analyzer_double_fclose) Init(1) Warning
Warn about code paths in which a stdio FILE can be closed more than once.
@@ -1739,9 +1739,12 @@ struct null_assignment_sm_context : public sm_context
state_machine::state_t from = get_state (stmt, var);
if (from != m_sm.get_start_state ())
return;
+ if (!is_transition_to_null (to))
+ return;
const svalue *var_new_sval
= m_new_state->m_region_model->get_rvalue (var, NULL);
+
const supernode *supernode = m_point->get_supernode ();
int stack_depth = m_point->get_stack_depth ();
@@ -1764,6 +1767,8 @@ struct null_assignment_sm_context : public sm_context
state_machine::state_t from = get_state (stmt, sval);
if (from != m_sm.get_start_state ())
return;
+ if (!is_transition_to_null (to))
+ return;
const supernode *supernode = m_point->get_supernode ();
int stack_depth = m_point->get_stack_depth ();
@@ -1834,6 +1839,13 @@ struct null_assignment_sm_context : public sm_context
return m_new_state;
}
+ /* We only care about transitions to the "null" state
+ within sm-malloc. Special-case this. */
+ static bool is_transition_to_null (state_machine::state_t s)
+ {
+ return !strcmp (s->get_name (), "null");
+ }
+
const program_state *m_old_state;
const program_state *m_new_state;
const gimple *m_stmt;
@@ -921,6 +921,22 @@ impl_region_model_context::on_bounded_ranges (const svalue &sval,
}
}
+/* Implementation of region_model_context::on_pop_frame vfunc.
+ Notify all state machines about the frame being popped, which
+ could lead to states being discarded. */
+
+void
+impl_region_model_context::on_pop_frame (const frame_region *frame_reg)
+{
+ int sm_idx;
+ sm_state_map *smap;
+ FOR_EACH_VEC_ELT (m_new_state->m_checker_states, sm_idx, smap)
+ {
+ const state_machine &sm = m_ext_state.get_sm (sm_idx);
+ sm.on_pop_frame (smap, frame_reg);
+ }
+}
+
/* Implementation of region_model_context::on_phi vfunc.
Notify all state machines about the phi, which could lead to
state transitions. */
@@ -77,6 +77,8 @@ class impl_region_model_context : public region_model_context
void on_bounded_ranges (const svalue &sval,
const bounded_ranges &ranges) final override;
+ void on_pop_frame (const frame_region *frame_reg) final override;
+
void on_unknown_change (const svalue *sval, bool is_mutable) final override;
void on_phi (const gphi *phi, tree rhs) final override;
@@ -518,6 +518,14 @@ sm_state_map::impl_set_state (const svalue *sval,
return true;
}
+/* Clear any state for SVAL from this state map. */
+
+void
+sm_state_map::clear_any_state (const svalue *sval)
+{
+ m_map.remove (sval);
+}
+
/* Set the "global" state within this state map to STATE. */
void
@@ -719,6 +727,67 @@ sm_state_map::canonicalize_svalue (const svalue *sval,
return sval;
}
+/* Attempt to merge this state map with OTHER, writing the result
+ into *OUT.
+ Return true if the merger was possible, false otherwise.
+
+ Normally, only identical state maps can be merged, so that
+ differences between state maps lead to different enodes
+
+ However some state machines may support merging states to
+ allow for discarding of less important states, and thus avoid
+ blow-up of the exploded graph. */
+
+bool
+sm_state_map::can_merge_with_p (const sm_state_map &other,
+ const state_machine &sm,
+ const extrinsic_state &ext_state,
+ sm_state_map **out) const
+{
+ /* If identical, then they merge trivially, with a copy. */
+ if (*this == other)
+ {
+ delete *out;
+ *out = clone ();
+ return true;
+ }
+
+ delete *out;
+ *out = new sm_state_map (sm);
+
+ /* Otherwise, attempt to merge element by element. */
+
+ /* Try to merge global state. */
+ if (state_machine::state_t merged_global_state
+ = sm.maybe_get_merged_state (get_global_state (),
+ other.get_global_state ()))
+ (*out)->set_global_state (merged_global_state);
+ else
+ return false;
+
+ /* Try to merge state each svalue's state (for the union
+ of svalues represented by each smap).
+ Ignore the origin information. */
+ hash_set<const svalue *> svals;
+ for (auto kv : *this)
+ svals.add (kv.first);
+ for (auto kv : other)
+ svals.add (kv.first);
+ for (auto sval : svals)
+ {
+ state_machine::state_t this_state = get_state (sval, ext_state);
+ state_machine::state_t other_state = other.get_state (sval, ext_state);
+ if (state_machine::state_t merged_state
+ = sm.maybe_get_merged_state (this_state, other_state))
+ (*out)->impl_set_state (sval, merged_state, NULL, ext_state);
+ else
+ return false;
+ }
+
+ /* Successfully merged all elements. */
+ return true;
+}
+
/* class program_state. */
/* program_state's ctor. */
@@ -1283,11 +1352,14 @@ program_state::can_merge_with_p (const program_state &other,
gcc_assert (out);
gcc_assert (m_region_model);
- /* Early reject if there are sm-differences between the states. */
+ /* Attempt to merge the sm-states. */
int i;
sm_state_map *smap;
FOR_EACH_VEC_ELT (out->m_checker_states, i, smap)
- if (*m_checker_states[i] != *other.m_checker_states[i])
+ if (!m_checker_states[i]->can_merge_with_p (*other.m_checker_states[i],
+ ext_state.get_sm (i),
+ ext_state,
+ &out->m_checker_states[i]))
return false;
/* Attempt to merge the region_models. */
@@ -1298,13 +1370,6 @@ program_state::can_merge_with_p (const program_state &other,
this, &other))
return false;
- /* Copy m_checker_states to OUT. */
- FOR_EACH_VEC_ELT (out->m_checker_states, i, smap)
- {
- delete smap;
- out->m_checker_states[i] = m_checker_states[i]->clone ();
- }
-
out->m_region_model->canonicalize ();
return true;
@@ -145,6 +145,7 @@ public:
state_machine::state_t state,
const svalue *origin,
const extrinsic_state &ext_state);
+ void clear_any_state (const svalue *sval);
void set_global_state (state_machine::state_t state);
state_machine::state_t get_global_state () const;
@@ -174,6 +175,11 @@ public:
bool replay_call_summary (call_summary_replay &r,
const sm_state_map &summary);
+ bool can_merge_with_p (const sm_state_map &other,
+ const state_machine &sm,
+ const extrinsic_state &ext_state,
+ sm_state_map **out) const;
+
private:
const state_machine &m_sm;
map_t m_map;
@@ -4690,7 +4690,7 @@ tristate
region_model::eval_condition (tree lhs,
enum tree_code op,
tree rhs,
- region_model_context *ctxt)
+ region_model_context *ctxt) const
{
/* For now, make no attempt to model constraints on floating-point
values. */
@@ -5415,8 +5415,13 @@ region_model::pop_frame (tree result_lvalue,
{
gcc_assert (m_current_frame);
- /* Evaluate the result, within the callee frame. */
const frame_region *frame_reg = m_current_frame;
+
+ /* Notify state machines. */
+ if (ctxt)
+ ctxt->on_pop_frame (frame_reg);
+
+ /* Evaluate the result, within the callee frame. */
tree fndecl = m_current_frame->get_function ()->decl;
tree result = DECL_RESULT (fndecl);
const svalue *retval = NULL;
@@ -458,7 +458,7 @@ class region_model
tristate eval_condition (tree lhs,
enum tree_code op,
tree rhs,
- region_model_context *ctxt);
+ region_model_context *ctxt) const;
bool add_constraint (tree lhs, enum tree_code op, tree rhs,
region_model_context *ctxt);
bool add_constraint (tree lhs, enum tree_code op, tree rhs,
@@ -708,6 +708,9 @@ class region_model_context
virtual void on_bounded_ranges (const svalue &sval,
const bounded_ranges &ranges) = 0;
+ /* Hook for clients to be notified when a frame is popped from the stack. */
+ virtual void on_pop_frame (const frame_region *) = 0;
+
/* Hooks for clients to be notified when an unknown change happens
to SVAL (in response to a call to an unknown function). */
virtual void on_unknown_change (const svalue *sval, bool is_mutable) = 0;
@@ -789,6 +792,7 @@ public:
const bounded_ranges &) override
{
}
+ void on_pop_frame (const frame_region *) override {}
void on_unknown_change (const svalue *sval ATTRIBUTE_UNUSED,
bool is_mutable ATTRIBUTE_UNUSED) override
{
@@ -886,6 +890,11 @@ class region_model_context_decorator : public region_model_context
m_inner->on_bounded_ranges (sval, ranges);
}
+ void on_pop_frame (const frame_region *frame_reg) override
+ {
+ m_inner->on_pop_frame (frame_reg);
+ }
+
void on_unknown_change (const svalue *sval, bool is_mutable) override
{
m_inner->on_unknown_change (sval, is_mutable);
@@ -87,6 +87,9 @@ enum resource_state
/* The start state. */
RS_START,
+ /* State for a pointer that's been unconditionally dereferenced. */
+ RS_ASSUMED_NON_NULL,
+
/* State for a pointer that's known to be NULL. */
RS_NULL,
@@ -126,7 +129,7 @@ struct allocation_state : public state_machine::state
m_deallocator (deallocator)
{}
- void dump_to_pp (pretty_printer *pp) const final override;
+ void dump_to_pp (pretty_printer *pp) const override;
const allocation_state *get_nonnull () const;
@@ -135,6 +138,25 @@ struct allocation_state : public state_machine::state
const deallocator *m_deallocator;
};
+/* Custom state subclass, for the "assumed-non-null" state
+ where the assumption happens in a particular frame. */
+
+struct assumed_non_null_state : public allocation_state
+{
+ assumed_non_null_state (const char *name, unsigned id,
+ const frame_region *frame)
+ : allocation_state (name, id, RS_ASSUMED_NON_NULL,
+ NULL, NULL),
+ m_frame (frame)
+ {
+ gcc_assert (m_frame);
+ }
+
+ void dump_to_pp (pretty_printer *pp) const final override;
+
+ const frame_region *m_frame;
+};
+
/* An enum for choosing which wording to use in various diagnostics
when describing deallocations. */
@@ -384,14 +406,25 @@ public:
enum tree_code op,
const svalue *rhs) const final override;
+ void on_pop_frame (sm_state_map *smap,
+ const frame_region *) const final override;
+
bool can_purge_p (state_t s) const final override;
std::unique_ptr<pending_diagnostic> on_leak (tree var) const final override;
bool reset_when_passed_to_unknown_fn_p (state_t s,
bool is_mutable) const final override;
+ state_t
+ maybe_get_merged_states_nonequal (state_t state_a,
+ state_t state_b) const final override;
+
static bool unaffected_by_call_p (tree fndecl);
+ void maybe_assume_non_null (sm_context *sm_ctxt,
+ tree ptr,
+ const gimple *stmt) const;
+
void on_realloc_with_move (region_model *model,
sm_state_map *smap,
const svalue *old_ptr_sval,
@@ -406,6 +439,10 @@ public:
/* States that are independent of api. */
+ /* States for a pointer that's been unconditionally dereferenced
+ in a particular stack frame. */
+ hash_map<const frame_region *, state_t> m_assumed_non_null;
+
/* State for a pointer that's known to be NULL. */
state_t m_null;
@@ -425,6 +462,16 @@ private:
const deallocator *
get_or_create_deallocator (tree deallocator_fndecl);
+ state_t
+ get_or_create_assumed_non_null_state_for_frame (const frame_region *frame);
+
+ void
+ maybe_complain_about_deref_before_check (sm_context *sm_ctxt,
+ const supernode *node,
+ const gimple *stmt,
+ const assumed_non_null_state *,
+ tree ptr) const;
+
void on_allocator_call (sm_context *sm_ctxt,
const gcall *call,
const deallocator_set *deallocators,
@@ -678,6 +725,14 @@ freed_p (state_machine::state_t state)
return get_rs (state) == RS_FREED;
}
+/* Return true if STATE is a value that has been assumed to be non-NULL. */
+
+static bool
+assumed_non_null_p (state_machine::state_t state)
+{
+ return get_rs (state) == RS_ASSUMED_NON_NULL;
+}
+
/* Class for diagnostics relating to malloc_state_machine. */
class malloc_diagnostic : public pending_diagnostic
@@ -1428,6 +1483,80 @@ private:
const char *m_funcname;
};
+/* Concrete pending_diagnostic subclass for -Wanalyzer-deref-before-check. */
+
+class deref_before_check : public malloc_diagnostic
+{
+public:
+ deref_before_check (const malloc_state_machine &sm, tree arg)
+ : malloc_diagnostic (sm, arg)
+ {}
+
+ const char *get_kind () const final override { return "deref_before_check"; }
+
+ int get_controlling_option () const final override
+ {
+ return OPT_Wanalyzer_deref_before_check;
+ }
+
+ bool emit (rich_location *rich_loc) final override
+ {
+ if (m_arg)
+ return warning_at (rich_loc, get_controlling_option (),
+ "check of %qE for NULL after already"
+ " dereferencing it",
+ m_arg);
+ else
+ return warning_at (rich_loc, get_controlling_option (),
+ "check of pointer for NULL after already"
+ " dereferencing it");
+ }
+
+ label_text describe_state_change (const evdesc::state_change &change)
+ final override
+ {
+ if (change.m_old_state == m_sm.get_start_state ()
+ && assumed_non_null_p (change.m_new_state))
+ {
+ m_first_deref_event = change.m_event_id;
+ if (m_arg)
+ return change.formatted_print ("pointer %qE is dereferenced here",
+ m_arg);
+ else
+ return label_text::borrow ("pointer is dereferenced here");
+ }
+ return malloc_diagnostic::describe_state_change (change);
+ }
+
+ label_text describe_final_event (const evdesc::final_event &ev) final override
+ {
+ if (m_first_deref_event.known_p ())
+ {
+ if (m_arg)
+ return ev.formatted_print ("pointer %qE is checked for NULL here but"
+ " it was already dereferenced at %@",
+ m_arg, &m_first_deref_event);
+ else
+ return ev.formatted_print ("pointer is checked for NULL here but"
+ " it was already dereferenced at %@",
+ &m_first_deref_event);
+ }
+ else
+ {
+ if (m_arg)
+ return ev.formatted_print ("pointer %qE is checked for NULL here but"
+ " it was already dereferenced",
+ m_arg);
+ else
+ return ev.formatted_print ("pointer is checked for NULL here but"
+ " it was already dereferenced");
+ }
+ }
+
+private:
+ diagnostic_event_id_t m_first_deref_event;
+};
+
/* struct allocation_state : public state_machine::state. */
/* Implementation of state_machine::state::dump_to_pp vfunc
@@ -1456,6 +1585,17 @@ allocation_state::get_nonnull () const
return as_a_allocation_state (m_deallocators->m_nonnull);
}
+/* struct assumed_non_null_state : public allocation_state. */
+
+void
+assumed_non_null_state::dump_to_pp (pretty_printer *pp) const
+{
+ allocation_state::dump_to_pp (pp);
+ pp_string (pp, " (in ");
+ m_frame->dump_to_pp (pp, true);
+ pp_character (pp, ')');
+}
+
/* malloc_state_machine's ctor. */
malloc_state_machine::malloc_state_machine (logger *logger)
@@ -1597,6 +1737,22 @@ malloc_state_machine::get_or_create_deallocator (tree deallocator_fndecl)
return d;
}
+/* Get the "assumed-non-null" state for assumptions made within FRAME,
+ creating it if necessary. */
+
+state_machine::state_t
+malloc_state_machine::
+get_or_create_assumed_non_null_state_for_frame (const frame_region *frame)
+{
+ if (state_t *slot = m_assumed_non_null.get (frame))
+ return *slot;
+ state_machine::state *new_state
+ = new assumed_non_null_state ("assumed-non-null", alloc_state_id (), frame);
+ add_custom_state (new_state);
+ m_assumed_non_null.put (frame, new_state);
+ return new_state;
+}
+
/* Try to identify the function declaration either by name or as a known malloc
builtin. */
@@ -1629,6 +1785,33 @@ known_allocator_p (const_tree fndecl, const gcall *call)
return false;
}
+/* If PTR's nullness is not known, transition it to the "assumed-non-null"
+ state for the current frame. */
+
+void
+malloc_state_machine::maybe_assume_non_null (sm_context *sm_ctxt,
+ tree ptr,
+ const gimple *stmt) const
+{
+ const region_model *old_model = sm_ctxt->get_old_region_model ();
+ if (!old_model)
+ return;
+
+ tree null_ptr_cst = build_int_cst (TREE_TYPE (ptr), 0);
+ tristate known_non_null
+ = old_model->eval_condition (ptr, NE_EXPR, null_ptr_cst, NULL);
+ if (known_non_null.is_unknown ())
+ {
+ /* Cast away const-ness for cache-like operations. */
+ malloc_state_machine *mut_this
+ = const_cast <malloc_state_machine *> (this);
+ state_t next_state
+ = mut_this->get_or_create_assumed_non_null_state_for_frame
+ (old_model->get_current_frame ());
+ sm_ctxt->set_next_state (stmt, ptr, next_state);
+ }
+}
+
/* Implementation of state_machine::on_stmt vfunc for malloc_state_machine. */
bool
@@ -1743,6 +1926,8 @@ malloc_state_machine::on_stmt (sm_context *sm_ctxt,
(*this, diag_arg, callee_fndecl, i));
sm_ctxt->set_next_state (stmt, arg, m_stop);
}
+ else if (state == m_start)
+ maybe_assume_non_null (sm_ctxt, arg, stmt);
}
}
BITMAP_FREE (nonnull_args);
@@ -1760,6 +1945,36 @@ malloc_state_machine::on_stmt (sm_context *sm_ctxt,
}
}
+ /* Look for pointers explicitly being compared against zero
+ that are in state assumed_non_null i.e. we already defererenced
+ them.
+ We have to do this check here, rather than in on_condition
+ because we add a constraint that the pointer is non-null when
+ dereferencing it, and this makes the apply_constraints_for_gcond
+ find known-true and known-false conditions; on_condition is only
+ called when adding new constraints. */
+ if (const gcond *cond_stmt = dyn_cast <const gcond *> (stmt))
+ {
+ enum tree_code op = gimple_cond_code (cond_stmt);
+ if (op == EQ_EXPR || op == NE_EXPR)
+ {
+ tree lhs = gimple_cond_lhs (cond_stmt);
+ tree rhs = gimple_cond_rhs (cond_stmt);
+ if (any_pointer_p (lhs)
+ && any_pointer_p (rhs)
+ && zerop (rhs))
+ {
+ state_t state = sm_ctxt->get_state (stmt, lhs);
+ if (assumed_non_null_p (state))
+ maybe_complain_about_deref_before_check
+ (sm_ctxt, node,
+ stmt,
+ (const assumed_non_null_state *)state,
+ lhs);
+ }
+ }
+ }
+
if (tree lhs = sm_ctxt->is_zero_assignment (stmt))
if (any_pointer_p (lhs))
on_zero_assignment (sm_ctxt, stmt,lhs);
@@ -1778,7 +1993,9 @@ malloc_state_machine::on_stmt (sm_context *sm_ctxt,
tree arg = TREE_OPERAND (op, 0);
state_t state = sm_ctxt->get_state (stmt, arg);
- if (unchecked_p (state))
+ if (state == m_start)
+ maybe_assume_non_null (sm_ctxt, arg, stmt);
+ else if (unchecked_p (state))
{
tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
sm_ctxt->warn (node, stmt, arg,
@@ -1808,6 +2025,53 @@ malloc_state_machine::on_stmt (sm_context *sm_ctxt,
return false;
}
+/* Given a check against null of PTR in assumed-non-null state STATE,
+ potentially add a deref_before_check warning to SM_CTXT. */
+
+void
+malloc_state_machine::
+maybe_complain_about_deref_before_check (sm_context *sm_ctxt,
+ const supernode *node,
+ const gimple *stmt,
+ const assumed_non_null_state *state,
+ tree ptr) const
+{
+ const region_model *model = sm_ctxt->get_old_region_model ();
+ if (!model)
+ return;
+
+ /* Don't complain if the current frame (where the check is occurring) is
+ deeper than the frame in which the "not null" assumption was made.
+ This suppress false positives for cases like:
+
+ void foo (struct s *p)
+ {
+ int val = s->some_field; // deref here
+ shared_helper (p);
+ }
+
+ where "shared_helper" has:
+
+ void shared_helper (struct s *p)
+ {
+ if (!p) // check here
+ return;
+ // etc
+ }
+
+ since the check in "shared_helper" is OK. */
+ const frame_region *checked_in_frame = model->get_current_frame ();
+ const frame_region *assumed_nonnull_in_frame = state->m_frame;
+ if (checked_in_frame->get_index () > assumed_nonnull_in_frame->get_index ())
+ return;
+
+ tree diag_ptr = sm_ctxt->get_diagnostic_tree (ptr);
+ sm_ctxt->warn
+ (node, stmt, ptr,
+ make_unique<deref_before_check> (*this, diag_ptr));
+ sm_ctxt->set_next_state (stmt, ptr, m_stop);
+}
+
/* Handle a call to an allocator.
RETURNS_NONNULL is true if CALL is to a fndecl known to have
__attribute__((returns_nonnull)). */
@@ -1870,8 +2134,8 @@ malloc_state_machine::on_deallocator_call (sm_context *sm_ctxt,
state_t state = sm_ctxt->get_state (call, arg);
- /* start/unchecked/nonnull -> freed. */
- if (state == m_start)
+ /* start/assumed_non_null/unchecked/nonnull -> freed. */
+ if (state == m_start || assumed_non_null_p (state))
sm_ctxt->set_next_state (call, arg, d->m_freed);
else if (unchecked_p (state) || nonnull_p (state))
{
@@ -2016,6 +2280,31 @@ malloc_state_machine::on_condition (sm_context *sm_ctxt,
}
}
+/* Implementation of state_machine::on_pop_frame vfunc for malloc_state_machine.
+ Clear any "assumed-non-null" state where the assumption happened in
+ FRAME_REG. */
+
+void
+malloc_state_machine::on_pop_frame (sm_state_map *smap,
+ const frame_region *frame_reg) const
+{
+ hash_set<const svalue *> svals_to_clear;
+ for (auto kv : *smap)
+ {
+ const svalue *sval = kv.first;
+ state_t state = kv.second.m_state;
+ if (assumed_non_null_p (state))
+ {
+ const assumed_non_null_state *assumed_state
+ = (const assumed_non_null_state *)state;
+ if (frame_reg == assumed_state->m_frame)
+ svals_to_clear.add (sval);
+ }
+ }
+ for (auto sval : svals_to_clear)
+ smap->clear_any_state (sval);
+}
+
/* Implementation of state_machine::can_purge_p vfunc for malloc_state_machine.
Don't allow purging of pointers in state 'unchecked' or 'nonnull'
(to avoid false leak reports). */
@@ -2053,6 +2342,23 @@ malloc_state_machine::reset_when_passed_to_unknown_fn_p (state_t s,
return is_mutable;
}
+/* Implementation of state_machine::maybe_get_merged_states_nonequal vfunc
+ for malloc_state_machine.
+
+ Support discarding "assumed-non-null" states when merging with
+ start state. */
+
+state_machine::state_t
+malloc_state_machine::maybe_get_merged_states_nonequal (state_t state_a,
+ state_t state_b) const
+{
+ if (assumed_non_null_p (state_a) && state_b == m_start)
+ return m_start;
+ if (state_a == m_start && assumed_non_null_p (state_b))
+ return m_start;
+ return NULL;
+}
+
/* Return true if calls to FNDECL are known to not affect this sm-state. */
bool
@@ -32,6 +32,9 @@ digraph "malloc" {
It could be a pointer to heap-allocated memory, or could be NULL. */
unchecked;
+ /* State for a pointer that's been unconditionally dereferenced. */
+ assumed_non_null;
+
/* State for a pointer that's known to be NULL. */
null;
@@ -58,6 +61,7 @@ digraph "malloc" {
/* On "free". */
start -> freed [label="on 'free(X);'"];
+ assumed_non_null -> freed [label="on 'free(X);'"];
unchecked -> freed [label="on 'free(X);'"];
nonnull -> freed [label="on 'free(X);'"];
freed -> stop [label="on 'free(X);':\n Warn('double-free')"];
@@ -66,6 +70,7 @@ digraph "malloc" {
/* Handle "__attribute__((nonnull))". */
unchecked -> nonnull [label="on 'FN(X)' with __attribute__((nonnull)):\nWarn('possible NULL arg')"];
null -> stop [label="on 'FN(X)' with __attribute__((nonnull)):\nWarn('NULL arg')"];
+ start -> assumed_non_null [label="on 'FN(X)' with __attribute__((nonnull))"];
/* is_zero_assignment. */
start -> null [label="on 'X = 0;'"];
@@ -76,6 +81,7 @@ digraph "malloc" {
start -> non_heap [label="on 'X = &EXPR;'"];
/* Handle dereferences. */
+ start -> assumed_non_null [label="on '*X'"];
unchecked -> nonnull [label="on '*X':\nWarn('possible NULL deref')"];
null -> stop [label="on '*X':\nWarn('NULL deref')"];
freed -> stop [label="on '*X':\nWarn('use after free')"];
@@ -83,6 +89,7 @@ digraph "malloc" {
/* on_condition. */
unchecked -> nonnull [label="on 'X != 0'"];
unchecked -> null [label="on 'X == 0'"];
+ assumed_non_null -> stop [label="on 'if (X)':\nWarn('deref-before-check')"];
unchecked -> stop [label="on leak:\nWarn('leak')"];
nonnull -> stop [label="on leak:\nWarn('leak')"];
@@ -117,6 +117,12 @@ public:
{
}
+ virtual void
+ on_pop_frame (sm_state_map *smap ATTRIBUTE_UNUSED,
+ const frame_region *frame_reg ATTRIBUTE_UNUSED) const
+ {
+ }
+
/* Return true if it safe to discard the given state (to help
when simplifying state objects).
States that need leak detection should return false. */
@@ -139,6 +145,31 @@ public:
return is_mutable;
}
+ /* Attempt to get a state for the merger of STATE_A and STATE_B,
+ or return NULL if merging shouldn't occur, so that differences
+ between sm-state will lead to separate exploded nodes.
+
+ Most state machines will only merge equal states, but can
+ override maybe_get_merged_states_nonequal to support mergers
+ of certain non-equal states. */
+ state_t maybe_get_merged_state (state_t state_a,
+ state_t state_b) const
+ {
+ if (state_a == state_b)
+ return state_a;
+ return maybe_get_merged_states_nonequal (state_a, state_b);
+ }
+
+ /* Base implementation of hook for maybe_get_merged_state on non-equal
+ states. */
+ virtual state_t
+ maybe_get_merged_states_nonequal (state_t state_a ATTRIBUTE_UNUSED,
+ state_t state_b ATTRIBUTE_UNUSED) const
+ {
+ /* By default, non-equal sm states should inhibit merger of enodes. */
+ return NULL;
+ }
+
void validate (state_t s) const;
void dump_to_pp (pretty_printer *pp) const;
@@ -19,6 +19,7 @@ Options That Control Static Analysis
Enabling this option effectively enables the following warnings:
:option:`-Wanalyzer-allocation-size` |gol|
+ :option:`-Wanalyzer-deref-before-check` |gol|
:option:`-Wanalyzer-double-fclose` |gol|
:option:`-Wanalyzer-double-free` |gol|
:option:`-Wanalyzer-exposure-through-output-file` |gol|
@@ -88,6 +89,33 @@ Options That Control Static Analysis
Default setting; overrides :option:`-Wno-analyzer-allocation-size`.
+.. option:: -Wno-analyzer-deref-before-check
+
+ This warning requires :option:`-fanalyzer`, which enables it; use
+ :option:`-Wno-analyzer-deref-before-check`
+ to disable it.
+
+ This diagnostic warns for paths through the code in which a pointer
+ is checked for ``NULL`` *after* it has already been
+ dereferenced, suggesting that the pointer could have been NULL.
+ Such cases suggest that the check for NULL is either redundant,
+ or that it needs to be moved to before the pointer is dereferenced.
+
+ This diagnostic also considers values passed to a function argument
+ marked with ``__attribute__((nonnull))`` as requiring a non-NULL
+ value, and thus will complain if such values are checked for ``NULL``
+ after returning from such a function call.
+
+ This diagnostic is unlikely to be reported when any level of optimization
+ is enabled, as GCC's optimization logic will typically consider such
+ checks for NULL as being redundant, and optimize them away before the
+ analyzer "sees" them. Hence optimization should be disabled when
+ attempting to trigger this diagnostic.
+
+.. option:: -Wanalyzer-deref-before-check
+
+ Default setting; overrides :option:`-Wno-analyzer-deref-before-check`.
+
.. option:: -Wno-analyzer-double-fclose
This warning requires :option:`-fanalyzer`, which enables it; use
@@ -825,6 +853,7 @@ The following options control the analyzer.
Currently, :option:`-fanalyzer-checker=taint` disables the
following warnings from :option:`-fanalyzer` :
+ :option:`-Wanalyzer-deref-before-check` |gol|
:option:`-Wanalyzer-double-fclose` |gol|
:option:`-Wanalyzer-double-free` |gol|
:option:`-Wanalyzer-exposure-through-output-file` |gol|
new file mode 100644
@@ -0,0 +1,169 @@
+#define NULL ((void *)0)
+
+int test_from_pr77432 (int *a)
+{
+ int b = *a; /* { dg-message "pointer 'a' is dereferenced here" } */
+ if (a) /* { dg-warning "check of 'a' for NULL after already dereferencing it \\\[-Wanalyzer-deref-before-check\\\]" "warning" } */
+ /* { dg-message "pointer 'a' is checked for NULL here but it was already dereferenced at \\(1\\)" "final event" { target *-*-* } .-1 } */
+ return b;
+ return 0;
+}
+
+int test_1a (int *p, int x)
+{
+ *p = x; /* { dg-message "pointer 'p' is dereferenced here" } */
+
+ if (p) /* { dg-warning "check of 'p' for NULL after already dereferencing it \\\[-Wanalyzer-deref-before-check\\\]" "warning" } */
+ /* { dg-message "pointer 'p' is checked for NULL here but it was already dereferenced at \\(1\\)" "final event" { target *-*-* } .-1 } */
+ return 1;
+ else
+ return 0;
+}
+
+int test_1b (int *p, int *q)
+{
+ *q = *p; /* { dg-message "8: pointer 'p' is dereferenced here" } */
+
+ if (p) /* { dg-warning "check of 'p' for NULL after already dereferencing it \\\[-Wanalyzer-deref-before-check\\\]" "warning" } */
+ /* { dg-message "pointer 'p' is checked for NULL here but it was already dereferenced at \\(1\\)" "final event" { target *-*-* } .-1 } */
+ return 1;
+ else
+ return 0;
+}
+
+struct s2
+{
+ int x;
+ int y;
+};
+
+int test_2a (struct s2 *p)
+{
+ int sum = p->x + p->y; /* { dg-message "pointer 'p' is dereferenced here" } */
+ if (!p) /* { dg-warning "check of 'p' for NULL after already dereferencing it" } */
+ __builtin_abort ();
+ return sum;
+}
+
+int test_2b (struct s2 *p)
+{
+ if (!p)
+ __builtin_abort ();
+ int sum = p->x + p->y;
+ return sum;
+}
+
+struct s3
+{
+ int flag;
+};
+
+extern void err (const char *);
+
+void test_3 (struct s3 *p)
+{
+ if (p->flag) /* { dg-message "pointer 'p' is dereferenced here" } */
+ err ("p->flag");
+ if (!p) /* { dg-warning "check of 'p' for NULL after already dereferencing it" } */
+ err ("p was NULL");
+}
+
+struct s4
+{
+ struct s4 *m_next;
+ int m_val;
+};
+
+int test_4 (struct s4 *p)
+{
+ if (p->m_next->m_val > 0) /* { dg-message "pointer '\\*p.m_next' is dereferenced here" } */
+ return -1;
+ if (!p->m_next) /* { dg-warning "check of '\\*p.m_next' for NULL after already dereferencing it" } */
+ return -2;
+ return p->m_next->m_val;
+}
+
+struct s5
+{
+ const char *str;
+ int val;
+};
+
+int test_5 (struct s5 *p)
+{
+ __builtin_printf ("%s: %i\n", p->str, p->val); /* { dg-message "pointer 'p' is dereferenced here" } */
+ if (p != NULL) /* { dg-warning "check of 'p' for NULL after already dereferencing it" } */
+ return p->val;
+ return -1;
+}
+
+static int __attribute__((noinline))
+__analyzer_check_ptr (int *p)
+{
+ if (p)
+ return *p;
+ else
+ return 42;
+}
+
+int test_calling_check_ptr_after_deref_1 (int *q)
+{
+ int v = *q; /* { dg-bogus "dereferenced here" } */
+ v += __analyzer_check_ptr (q);
+ return v;
+}
+
+int test_calling_check_ptr_after_deref_2 (int *q)
+{
+ int v = *q; /* { dg-bogus "dereferenced here" } */
+ v += __analyzer_check_ptr (q);
+ *q = 17;
+ return v;
+}
+
+int test_calling_check_ptr_after_deref_3 (int *q)
+{
+ int v = *q; /* { dg-message "pointer 'q' is dereferenced here" } */
+ v += __analyzer_check_ptr (q);
+ if (q) /* { dg-warning "check of 'q' for NULL after already dereferencing it" } */
+ *q = 17;
+ return v;
+}
+
+static int __attribute__((noinline))
+__analyzer_deref_ptr (int *p)
+{
+ return *p;
+}
+
+int test_calling_check_ptr_after_calling_deref_1 (int *q)
+{
+ int v = __analyzer_deref_ptr (q);
+ v += __analyzer_check_ptr (q);
+ return v;
+}
+
+int test_calling_check_ptr_after_calling_deref_2 (int *q)
+{
+ int v = __analyzer_deref_ptr (q);
+ v += __analyzer_check_ptr (q);
+ *q = 17;
+ return v;
+}
+
+int test_calling_check_ptr_after_calling_deref_3 (int *q)
+{
+ int v = __analyzer_deref_ptr (q);
+ v += __analyzer_check_ptr (q);
+ if (q)
+ *q = 17;
+ return v;
+}
+
+int test_checking_ptr_after_calling_deref (int *q)
+{
+ int v = __analyzer_deref_ptr (q);
+ if (q)
+ return 0;
+ return v;
+}
new file mode 100644
@@ -0,0 +1,130 @@
+#include <stdio.h>
+
+struct st
+{
+ char *str;
+ int i;
+};
+
+int test_1 (struct st *p)
+{
+ fprintf (stderr, "str: %s\n", p->str); /* { dg-message "pointer 'p' is dereferenced here" } */
+ if (!p) /* { dg-warning "check of 'p' for NULL after already dereferencing it" } */
+ return -1;
+ return p->i;
+}
+
+int test_2 (int flag_a, int flag_b, struct st *p)
+{
+ if (flag_a)
+ {
+ int j = p->i; /* { dg-message "pointer 'p' is dereferenced here" } */
+ if (flag_b && p) /* { dg-warning "check of 'p' for NULL after already dereferencing it" } */
+ return 1;
+ return j;
+ }
+ return 2;
+}
+
+int test_3 (struct st *a, struct st *b)
+{
+ if (!a)
+ return b->i;
+ if (!b)
+ return a->i;
+ return 0;
+}
+
+int test_4 (struct st *p)
+{
+ int *q = &p->i;
+ if (!p)
+ return -1;
+ return *q;
+}
+
+void test_check_after_strlen (const char *str)
+{
+ size_t len_a = __builtin_strlen (str); /* { dg-message "pointer 'str' is dereferenced here" } */
+ size_t len_b = str ? __builtin_strlen (str) : 0; /* { dg-warning "check of 'str' for NULL after already dereferencing it" } */
+}
+
+void test_6 (struct st *a, struct st *b)
+{
+ int diff = a->i - b->i; /* { dg-message "pointer 'b' is dereferenced here" } */
+
+ /* ... */
+
+ if (b) /* { dg-warning "check of 'b' for NULL after already dereferencing it" } */
+ fprintf (stderr, "str: %s\n", b->str);
+}
+
+void test_check_after_strcmp (const char *s1, const char *s2)
+{
+ if (!__builtin_strcmp (s1, s2)) /* { dg-message "pointer 's1' is dereferenced here" } */
+ return;
+
+ /* ... */
+
+ if (s1) /* { dg-warning "check of 's1' for NULL after already dereferencing it" } */
+ return;
+}
+
+void test_more_than_one_deref (struct st *p)
+{
+ char *str = p->str; /* { dg-message "pointer 'p' is dereferenced here" } */
+ int i = p->i;
+
+ /* ... */
+
+ if (p) /* { dg-warning "check of 'p' for NULL after already dereferencing it" } */
+ return;
+
+ /* ... */
+}
+
+void test_deref_under_another_name (struct st *p)
+{
+ struct st *q = p;
+ int i = q->i; /* { dg-message "pointer 'p' is dereferenced here" } */
+
+ /* ... */
+
+ if (p) /* { dg-warning "check of 'p' for NULL after already dereferencing it" } */
+ return;
+
+ /* ... */
+}
+
+void test_check_after_memcpy_src (struct st *dst, struct st *src)
+{
+ __builtin_memcpy (dst, src, sizeof (struct st)); /* { dg-message "pointer 'src' is dereferenced here" } */
+
+ /* ... */
+
+ if (!src) /* { dg-warning "check of 'src' for NULL after already dereferencing it" } */
+ return;
+
+ /* ... */
+}
+
+void test_check_after_memcpy_dst (struct st *dst, struct st *src)
+{
+ __builtin_memcpy (dst, src, sizeof (struct st)); /* { dg-message "pointer 'dst' is dereferenced here" } */
+
+ /* ... */
+
+ if (!dst) /* { dg-warning "check of 'dst' for NULL after already dereferencing it" } */
+ return;
+
+ /* ... */
+}
+
+void test_merger (int *p, int flag)
+{
+ int x = *p;
+ if (flag)
+ __builtin_free (p);
+ if (!flag)
+ __builtin_free (p); /* { dg-bogus "double-'free'" } */
+}
new file mode 100644
@@ -0,0 +1,43 @@
+/* Fixed in r7-2945-g61f46d0e6dd568.
+ Simplified from gcc/ipa-devirt.c. */
+
+#define NULL ((void *)0)
+typedef struct odr_type_d {
+ /* .... */
+ int id;
+ /* .... */
+} *odr_type;
+static odr_type **odr_types_ptr;
+#define odr_types (*odr_types_ptr) /* { dg-message "pointer 'odr_types_ptr' is dereferenced here" } */
+
+int cond, other_cond;
+
+odr_type some_logic ();
+
+odr_type
+get_odr_type (/* ... */)
+{
+ /* .... */
+ odr_type val = NULL;
+ /* .... */
+
+ val = some_logic ();
+
+ /* .... */
+ if (cond)
+ {
+ /* .... */
+ }
+ else if (other_cond)
+ {
+ odr_types[val->id] = 0; /* { dg-message "in expansion of macro 'odr_types'" } */
+ /* .... */
+ if (odr_types_ptr) /* { dg-warning "check of 'odr_types_ptr' for NULL after already dereferencing it" } */
+ {
+ /* .... */
+ val->id = 42;
+ }
+ /* .... */
+ }
+ return val;
+}
@@ -625,5 +625,14 @@ void test_50c (void)
free (&&my_label); /* { dg-warning "'free' of '&my_label' which points to memory not on the heap \\\[CWE-590\\\]" } */
}
+/* Double free after unconditional dereference. */
+
+int test_51 (int *p)
+{
+ int result = *p;
+ free (p); /* { dg-message "first 'free' here" } */
+ free (p); /* { dg-warning "double-'free' of 'p'" } */
+ return result;
+}
/* { dg-prune-output "\\\[-Wfree-nonheap-object" } */
@@ -38,6 +38,7 @@ class tristate {
const char *as_string () const;
bool is_known () const { return m_value != TS_UNKNOWN; }
+ bool is_unknown () const { return m_value == TS_UNKNOWN; }
bool is_true () const { return m_value == TS_TRUE; }
bool is_false () const { return m_value == TS_FALSE; }
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) | Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. Pushed to trunk as r13-3884-g5c6546ca7d8cab. 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> --- gcc/analyzer/analyzer.opt | 4 + gcc/analyzer/diagnostic-manager.cc | 12 + gcc/analyzer/engine.cc | 16 + gcc/analyzer/exploded-graph.h | 2 + gcc/analyzer/program-state.cc | 83 ++++- gcc/analyzer/program-state.h | 6 + gcc/analyzer/region-model.cc | 9 +- gcc/analyzer/region-model.h | 11 +- gcc/analyzer/sm-malloc.cc | 314 +++++++++++++++++- gcc/analyzer/sm-malloc.dot | 7 + gcc/analyzer/sm.h | 31 ++ .../options-that-control-static-analysis.rst | 29 ++ .../gcc.dg/analyzer/deref-before-check-1.c | 169 ++++++++++ .../gcc.dg/analyzer/deref-before-check-2.c | 130 ++++++++ .../analyzer/deref-before-check-pr77425.c | 43 +++ gcc/testsuite/gcc.dg/analyzer/malloc-1.c | 9 + gcc/tristate.h | 1 + 17 files changed, 860 insertions(+), 16 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/deref-before-check-1.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/deref-before-check-2.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/deref-before-check-pr77425.c