analyzer: add notes to write-to-const/string from access attr [PR104793]

The previous patch extended
  -Wanalyzer-write-to-const
  -Wanalyzer-write-to-string-literal
to make use of __attribute__ ((access, ....), but the results could be
inscrutable.

This patch adds notes to such diagnostics to give the user a reason for
why the analyzer is complaining.

Example output:

test.c: In function 'main':
test.c:15:13: warning: write to string literal [-Wanalyzer-write-to-string-literal]
   15 |         if (getrandom((char *)test, sizeof(buf), GRND_RANDOM))
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  'main': event 1
    |
    |   15 |         if (getrandom((char *)test, sizeof(buf), GRND_RANDOM))
    |      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |             |
    |      |             (1) write to string literal here
    |
test.c:3:5: note: parameter 1 of 'getrandom' marked with attribute 'access (write_only, 1, 2)'
    3 | int getrandom (void *__buffer, size_t __length,
      |     ^~~~~~~~~

Unfortunately we don't have location information for the attributes
themselves, just the function declaration, and there doesn't seem to be
a good way of getting at the location of the individual parameters from
the middle end (the C and C++ FEs both have get_fndecl_argument_location,
but the implementations are different).

gcc/analyzer/ChangeLog:
	PR analyzer/104793
	* analyzer.h (class pending_note): New forward decl.
	* diagnostic-manager.cc (saved_diagnostic::saved_diagnostic):
	Initialize m_notes.
	(saved_diagnostic::operator==): Compare m_notes.
	(saved_diagnostic::add_note): New.
	(saved_diagnostic::emit_any_notes): New.
	(diagnostic_manager::add_note): New.
	(diagnostic_manager::emit_saved_diagnostic): Call emit_any_notes
	after emitting the warning.
	* diagnostic-manager.h (saved_diagnostic::add_note): New decl.
	(saved_diagnostic::emit_any_notes): New decl.
	(saved_diagnostic::m_notes): New field.
	(diagnostic_manager::add_note): New decl.
	* engine.cc (impl_region_model_context::add_note): New.
	* exploded-graph.h (impl_region_model_context::add_note): New
	decl.
	* pending-diagnostic.h (class pending_note): New.
	(class pending_note_subclass): New template.
	* region-model.cc (class reason_attr_access): New.
	(check_external_function_for_access_attr): Add class
	annotating_ctxt and use it when checking region.
	(noop_region_model_context::add_note): New.
	* region-model.h (region_model_context::add_note): New vfunc.
	(noop_region_model_context::add_note): New decl.
	(class region_model_context_decorator): New.
	(class note_adding_context): New.

gcc/testsuite/ChangeLog:
	PR analyzer/104793
	* gcc.dg/analyzer/write-to-const-2.c: Add dg-message directives
	for expected notes.
	* gcc.dg/analyzer/write-to-function-1.c: Likewise.
	* gcc.dg/analyzer/write-to-string-literal-2.c: Likewise.
	* gcc.dg/analyzer/write-to-string-literal-3.c: Likewise.
	* gcc.dg/analyzer/write-to-string-literal-4.c: Likewise.
	* gcc.dg/analyzer/write-to-string-literal-5.c: New test.

Signed-off-by: David Malcolm <dmalcolm@redhat.com>
This commit is contained in:
David Malcolm 2022-03-10 09:09:40 -05:00
parent b6eaf90c64
commit c65d3c7f9d
14 changed files with 362 additions and 15 deletions

View File

@ -85,6 +85,7 @@ class bounded_ranges;
class bounded_ranges_manager;
class pending_diagnostic;
class pending_note;
class state_change_event;
class checker_path;
class extrinsic_state;

View File

@ -629,7 +629,8 @@ saved_diagnostic::saved_diagnostic (const state_machine *sm,
m_var (var), m_sval (sval), m_state (state),
m_d (d), m_trailing_eedge (NULL),
m_idx (idx),
m_best_epath (NULL), m_problem (NULL)
m_best_epath (NULL), m_problem (NULL),
m_notes ()
{
gcc_assert (m_stmt || m_stmt_finder);
@ -651,6 +652,11 @@ saved_diagnostic::~saved_diagnostic ()
bool
saved_diagnostic::operator== (const saved_diagnostic &other) const
{
if (m_notes.length () != other.m_notes.length ())
return false;
for (unsigned i = 0; i < m_notes.length (); i++)
if (!m_notes[i]->equal_p (*other.m_notes[i]))
return false;
return (m_sm == other.m_sm
/* We don't compare m_enode. */
&& m_snode == other.m_snode
@ -662,6 +668,15 @@ saved_diagnostic::operator== (const saved_diagnostic &other) const
&& m_trailing_eedge == other.m_trailing_eedge);
}
/* Add PN to this diagnostic, taking ownership of it. */
void
saved_diagnostic::add_note (pending_note *pn)
{
gcc_assert (pn);
m_notes.safe_push (pn);
}
/* Return a new json::object of the form
{"sm": optional str,
"enode": int,
@ -697,6 +712,7 @@ saved_diagnostic::to_json () const
exploded_edge *m_trailing_eedge;
enum status m_status;
feasibility_problem *m_problem;
auto_delete_vec <pending_note> m_notes;
*/
return sd_obj;
@ -769,6 +785,15 @@ saved_diagnostic::supercedes_p (const saved_diagnostic &other) const
return m_d->supercedes_p (*other.m_d);
}
/* Emit any pending notes owned by this diagnostic. */
void
saved_diagnostic::emit_any_notes () const
{
for (auto pn : m_notes)
pn->emit ();
}
/* State for building a checker_path from a particular exploded_path.
In particular, this precomputes reachability information: the set of
source enodes for which a path be found to the diagnostic enode. */
@ -875,6 +900,20 @@ diagnostic_manager::add_diagnostic (exploded_node *enode,
add_diagnostic (NULL, enode, snode, stmt, finder, NULL_TREE, NULL, 0, d);
}
/* Add PN to the most recent saved_diagnostic. */
void
diagnostic_manager::add_note (pending_note *pn)
{
LOG_FUNC (get_logger ());
gcc_assert (pn);
/* Get most recent saved_diagnostic. */
gcc_assert (m_saved_diagnostics.length () > 0);
saved_diagnostic *sd = m_saved_diagnostics[m_saved_diagnostics.length () - 1];
sd->add_note (pn);
}
/* Return a new json::object of the form
{"diagnostics" : [obj for saved_diagnostic]}. */
@ -1240,6 +1279,8 @@ diagnostic_manager::emit_saved_diagnostic (const exploded_graph &eg,
auto_cfun sentinel (sd.m_snode->m_fun);
if (sd.m_d->emit (&rich_loc))
{
sd.emit_any_notes ();
unsigned num_dupes = sd.get_num_dupes ();
if (flag_analyzer_show_duplicate_count && num_dupes > 0)
inform_n (loc, num_dupes,

View File

@ -42,6 +42,8 @@ public:
bool operator== (const saved_diagnostic &other) const;
void add_note (pending_note *pn);
json::object *to_json () const;
const feasibility_problem *get_feasibility_problem () const
@ -60,6 +62,8 @@ public:
bool supercedes_p (const saved_diagnostic &other) const;
void emit_any_notes () const;
//private:
const state_machine *m_sm;
const exploded_node *m_enode;
@ -80,6 +84,7 @@ private:
feasibility_problem *m_problem; // owned
auto_vec<const saved_diagnostic *> m_duplicates;
auto_delete_vec <pending_note> m_notes;
};
class path_builder;
@ -116,6 +121,8 @@ public:
stmt_finder *finder,
pending_diagnostic *d);
void add_note (pending_note *pn);
void emit_saved_diagnostics (const exploded_graph &eg);
void emit_saved_diagnostic (const exploded_graph &eg,

View File

@ -142,6 +142,16 @@ impl_region_model_context::warn (pending_diagnostic *d)
}
}
void
impl_region_model_context::add_note (pending_note *pn)
{
LOG_FUNC (get_logger ());
if (m_eg)
m_eg->get_diagnostic_manager ().add_note (pn);
else
delete pn;
}
void
impl_region_model_context::on_svalue_leak (const svalue *sval)

View File

@ -48,6 +48,7 @@ class impl_region_model_context : public region_model_context
logger *logger = NULL);
bool warn (pending_diagnostic *d) FINAL OVERRIDE;
void add_note (pending_note *pn) FINAL OVERRIDE;
void on_svalue_leak (const svalue *) OVERRIDE;
void on_liveness_change (const svalue_set &live_svalues,
const region_model *model) FINAL OVERRIDE;

View File

@ -329,6 +329,49 @@ class pending_diagnostic_subclass : public pending_diagnostic
}
};
/* An abstract base class for capturing additional notes that are to be
emitted with a diagnostic. */
class pending_note
{
public:
virtual ~pending_note () {}
/* Hand-coded RTTI: get an ID for the subclass. */
virtual const char *get_kind () const = 0;
/* Vfunc for emitting the note. */
virtual void emit () const = 0;
bool equal_p (const pending_note &other) const
{
/* Check for pointer equality on the IDs from get_kind. */
if (get_kind () != other.get_kind ())
return false;
/* Call vfunc now we know they have the same ID: */
return subclass_equal_p (other);
}
/* A vfunc for testing for equality, where we've already
checked they have the same ID. See pending_note_subclass
below for a convenience subclass for implementing this. */
virtual bool subclass_equal_p (const pending_note &other) const = 0;
};
/* Analogous to pending_diagnostic_subclass, but for pending_note. */
template <class Subclass>
class pending_note_subclass : public pending_note
{
public:
bool subclass_equal_p (const pending_note &base_other) const
FINAL OVERRIDE
{
const Subclass &other = (const Subclass &)base_other;
return *(const Subclass*)this == other;
}
};
} // namespace ana
#endif /* GCC_ANALYZER_PENDING_DIAGNOSTIC_H */

View File

@ -1583,6 +1583,41 @@ region_model::purge_state_involving (const svalue *sval,
ctxt->purge_state_involving (sval);
}
/* A pending_note subclass for adding a note about an
__attribute__((access, ...)) to a diagnostic. */
class reason_attr_access : public pending_note_subclass<reason_attr_access>
{
public:
reason_attr_access (tree callee_fndecl, const attr_access &access)
: m_callee_fndecl (callee_fndecl),
m_ptr_argno (access.ptrarg),
m_access_str (TREE_STRING_POINTER (access.to_external_string ()))
{
}
const char *get_kind () const FINAL OVERRIDE { return "reason_attr_access"; }
void emit () const
{
inform (DECL_SOURCE_LOCATION (m_callee_fndecl),
"parameter %i of %qD marked with attribute %qs",
m_ptr_argno + 1, m_callee_fndecl, m_access_str);
}
bool operator== (const reason_attr_access &other) const
{
return (m_callee_fndecl == other.m_callee_fndecl
&& m_ptr_argno == other.m_ptr_argno
&& !strcmp (m_access_str, other.m_access_str));
}
private:
tree m_callee_fndecl;
unsigned m_ptr_argno;
const char *m_access_str;
};
/* Check CALL a call to external function CALLEE_FNDECL based on
any __attribute__ ((access, ....) on the latter, complaining to
CTXT about any issues.
@ -1629,10 +1664,36 @@ check_external_function_for_access_attr (const gcall *call,
if (access->mode == access_write_only
|| access->mode == access_read_write)
{
/* Subclass of decorated_region_model_context that
adds a note about the attr access to any saved diagnostics. */
class annotating_ctxt : public note_adding_context
{
public:
annotating_ctxt (tree callee_fndecl,
const attr_access &access,
region_model_context *ctxt)
: note_adding_context (ctxt),
m_callee_fndecl (callee_fndecl),
m_access (access)
{
}
pending_note *make_note () FINAL OVERRIDE
{
return new reason_attr_access (m_callee_fndecl, m_access);
}
private:
tree m_callee_fndecl;
const attr_access &m_access;
};
/* Use this ctxt below so that any diagnostics get the
note added to them. */
annotating_ctxt my_ctxt (callee_fndecl, *access, ctxt);
tree ptr_tree = gimple_call_arg (call, access->ptrarg);
const svalue *ptr_sval = get_rvalue (ptr_tree, ctxt);
const region *reg = deref_rvalue (ptr_sval, ptr_tree, ctxt);
check_region_for_write (reg, ctxt);
const svalue *ptr_sval = get_rvalue (ptr_tree, &my_ctxt);
const region *reg = deref_rvalue (ptr_sval, ptr_tree, &my_ctxt);
check_region_for_write (reg, &my_ctxt);
/* We don't use the size arg for now. */
}
}
@ -4148,6 +4209,12 @@ region_model::unset_dynamic_extents (const region *reg)
/* class noop_region_model_context : public region_model_context. */
void
noop_region_model_context::add_note (pending_note *pn)
{
delete pn;
}
void
noop_region_model_context::bifurcate (custom_edge_info *info)
{

View File

@ -881,6 +881,10 @@ class region_model_context
Return true if the diagnostic was stored, or false if it was deleted. */
virtual bool warn (pending_diagnostic *d) = 0;
/* Hook for clients to add a note to the last previously stored pending diagnostic.
Takes ownership of the pending_node (or deletes it). */
virtual void add_note (pending_note *pn) = 0;
/* Hook for clients to be notified when an SVAL that was reachable
in a previous state is no longer live, so that clients can emit warnings
about leaks. */
@ -954,6 +958,7 @@ class noop_region_model_context : public region_model_context
{
public:
bool warn (pending_diagnostic *) OVERRIDE { return false; }
void add_note (pending_note *pn) OVERRIDE;
void on_svalue_leak (const svalue *) OVERRIDE {}
void on_liveness_change (const svalue_set &,
const region_model *) OVERRIDE {}
@ -1020,6 +1025,147 @@ private:
int m_num_unexpected_codes;
};
/* Subclass of region_model_context that wraps another context, allowing
for extra code to be added to the various hooks. */
class region_model_context_decorator : public region_model_context
{
public:
bool warn (pending_diagnostic *d) OVERRIDE
{
return m_inner->warn (d);
}
void add_note (pending_note *pn) OVERRIDE
{
m_inner->add_note (pn);
}
void on_svalue_leak (const svalue *sval) OVERRIDE
{
m_inner->on_svalue_leak (sval);
}
void on_liveness_change (const svalue_set &live_svalues,
const region_model *model) OVERRIDE
{
m_inner->on_liveness_change (live_svalues, model);
}
logger *get_logger () OVERRIDE
{
return m_inner->get_logger ();
}
void on_condition (const svalue *lhs,
enum tree_code op,
const svalue *rhs) OVERRIDE
{
m_inner->on_condition (lhs, op, rhs);
}
void on_unknown_change (const svalue *sval, bool is_mutable) OVERRIDE
{
m_inner->on_unknown_change (sval, is_mutable);
}
void on_phi (const gphi *phi, tree rhs) OVERRIDE
{
m_inner->on_phi (phi, rhs);
}
void on_unexpected_tree_code (tree t,
const dump_location_t &loc) OVERRIDE
{
m_inner->on_unexpected_tree_code (t, loc);
}
void on_escaped_function (tree fndecl) OVERRIDE
{
m_inner->on_escaped_function (fndecl);
}
uncertainty_t *get_uncertainty () OVERRIDE
{
return m_inner->get_uncertainty ();
}
void purge_state_involving (const svalue *sval) OVERRIDE
{
m_inner->purge_state_involving (sval);
}
void bifurcate (custom_edge_info *info) OVERRIDE
{
m_inner->bifurcate (info);
}
void terminate_path () OVERRIDE
{
m_inner->terminate_path ();
}
const extrinsic_state *get_ext_state () const OVERRIDE
{
return m_inner->get_ext_state ();
}
bool get_malloc_map (sm_state_map **out_smap,
const state_machine **out_sm,
unsigned *out_sm_idx) OVERRIDE
{
return m_inner->get_malloc_map (out_smap, out_sm, out_sm_idx);
}
bool get_taint_map (sm_state_map **out_smap,
const state_machine **out_sm,
unsigned *out_sm_idx) OVERRIDE
{
return m_inner->get_taint_map (out_smap, out_sm, out_sm_idx);
}
const gimple *get_stmt () const OVERRIDE
{
return m_inner->get_stmt ();
}
protected:
region_model_context_decorator (region_model_context *inner)
: m_inner (inner)
{
gcc_assert (m_inner);
}
region_model_context *m_inner;
};
/* Subclass of region_model_context_decorator that adds a note
when saving diagnostics. */
class note_adding_context : public region_model_context_decorator
{
public:
bool warn (pending_diagnostic *d) OVERRIDE
{
if (m_inner->warn (d))
{
add_note (make_note ());
return true;
}
else
return false;
}
/* Hook to make the new note. */
virtual pending_note *make_note () = 0;
protected:
note_adding_context (region_model_context *inner)
: region_model_context_decorator (inner)
{
}
};
/* A bundle of data for use when attempting to merge two region_model
instances to make a third. */

View File

@ -2,17 +2,17 @@ typedef __SIZE_TYPE__ size_t;
void read_only (void *)
__attribute__ ((access (read_only, 1)));
void write_only (void *)
void write_only (void *) /* { dg-message "parameter 1 of 'write_only' marked with attribute 'access \\(write_only, 1\\)'" } */
__attribute__ ((access (write_only, 1)));
void read_write (void *)
void read_write (void *) /* { dg-message "parameter 1 of 'read_write' marked with attribute 'access \\(read_write, 1\\)'" } */
__attribute__ ((access (read_write, 1)));
void none (void *)
__attribute__ ((access (none, 1)));
void read_only_with_size (void *, size_t)
__attribute__ ((access (read_only, 1, 2)));
void write_only_with_size (void *, size_t)
void write_only_with_size (void *, size_t) /* { dg-message "parameter 1 of 'write_only_with_size' marked with attribute 'access \\(write_only, 1, 2\\)'" } */
__attribute__ ((access (write_only, 1, 2)));
void read_write_with_size (void *, size_t)
void read_write_with_size (void *, size_t) /* { dg-message "parameter 1 of 'read_write_with_size' marked with attribute 'access \\(read_write, 1, 2\\)'" } */
__attribute__ ((access (read_write, 1, 2)));
void none_with_size (void *, size_t)
__attribute__ ((access (none, 1, 2)));

View File

@ -1,6 +1,6 @@
typedef __SIZE_TYPE__ size_t;
int getrandom (void *__buffer, size_t __length,
int getrandom (void *__buffer, size_t __length, /* { dg-message "parameter 1 of 'getrandom' marked with attribute 'access \\(write_only, 1, 2\\)'" } */
unsigned int __flags)
__attribute__ ((access (__write_only__, 1, 2)));

View File

@ -1,6 +1,6 @@
typedef __SIZE_TYPE__ size_t;
int getrandom (void *__buffer, size_t __length,
int getrandom (void *__buffer, size_t __length, /* { dg-message "parameter 1 of 'getrandom' marked with attribute 'access \\(write_only, 1, 2\\)'" } */
unsigned int __flags)
__attribute__ ((access (__write_only__, 1, 2)));

View File

@ -2,17 +2,17 @@ typedef __SIZE_TYPE__ size_t;
void read_only (void *)
__attribute__ ((access (read_only, 1)));
void write_only (void *)
void write_only (void *) /* { dg-message "parameter 1 of 'write_only' marked with attribute 'access \\(write_only, 1\\)'" } */
__attribute__ ((access (write_only, 1)));
void read_write (void *)
void read_write (void *) /* { dg-message "parameter 1 of 'read_write' marked with attribute 'access \\(read_write, 1\\)'" } */
__attribute__ ((access (read_write, 1)));
void none (void *)
__attribute__ ((access (none, 1)));
void read_only_with_size (void *, size_t)
__attribute__ ((access (read_only, 1, 2)));
void write_only_with_size (void *, size_t)
void write_only_with_size (void *, size_t) /* { dg-message "parameter 1 of 'write_only_with_size' marked with attribute 'access \\(write_only, 1, 2\\)'" } */
__attribute__ ((access (write_only, 1, 2)));
void read_write_with_size (void *, size_t)
void read_write_with_size (void *, size_t) /* { dg-message "parameter 1 of 'read_write_with_size' marked with attribute 'access \\(read_write, 1, 2\\)'" } */
__attribute__ ((access (read_write, 1, 2)));
void none_with_size (void *, size_t)
__attribute__ ((access (none, 1, 2)));

View File

@ -1,6 +1,6 @@
typedef __SIZE_TYPE__ size_t;
int getrandom (void *__buffer, size_t __length,
int getrandom (void *__buffer, size_t __length, /* { dg-message "parameter 1 of 'getrandom' marked with attribute 'access \\(write_only, 1, 2\\)'" } */
unsigned int __flags)
__attribute__ ((access (__write_only__, 1, 2)));

View File

@ -0,0 +1,31 @@
/* Verify that deduplication of -Wanalyzer-write-to-string-literal (and their
notes) works. */
/* { dg-additional-options "-fanalyzer-show-duplicate-count" } */
#include "analyzer-decls.h"
typedef __SIZE_TYPE__ size_t;
int getrandom (void *__buffer, size_t __length, /* { dg-message "parameter 1 of 'getrandom' marked with attribute 'access \\(write_only, 1, 2\\)'" } */
unsigned int __flags)
__attribute__ ((access (__write_only__, 1, 2)));
#define GRND_RANDOM 0x02
void *test (int flag)
{
char *ptr;
if (flag)
ptr = __builtin_malloc (1024);
else
ptr = __builtin_alloca (1024);
__analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed enodes" } */
if (getrandom((char *)"foo", 3, GRND_RANDOM)) /* { dg-warning "write to string literal" "warning" } */
/* { dg-message "1 duplicate" "dup" { target *-*-* } .-1 } */
__builtin_printf("ok\n");
return ptr;
}