Re: [RFC] analyzer: allocation size warning

[David Malcolm <dmalcolm@redhat.com>]

On Wed, 2022-06-22 at 16:57 +0200, Tim Lange wrote:
> The checker reaches region-model.cc#3083 in my patch with the
>   impl_region_model_context
> on the 'after' node of create_buffer() but then discards the warning
> inside
> impl_region_model_context::warn because m_stmt is null. Even if m_stmt
> were
> not be NULL at the 'after' node, my warning would be emitted before the
> return edge was taken and thus be wrongly indented like shown below:
> /path/to/.c:10:16: warning: Allocated buffer size is not a multiple of
> the pointee's size [CWE-131] [-Wanalyzer-allocation-size]
>    10 |     int *buf = create_buffer(42);
>       |                ^~~~~~~~~~~~~~~~~
>   ‘main’: events 1-2
>     |
>     |    9 |   int main (int argc, char **argv) {
>     |      |       ^~~~
>     |      |       |
>     |      |       (1) entry to ‘main’
>     |   10 |     int *buf = create_buffer(42);
>     |      |                ~~~~~~~~~~~~~~~~~
>     |      |                |
>     |      |                (2) calling ‘create_buffer’ from ‘main’
>     |
>     +--> ‘create_buffer’: events 3-4
>            |
>            |    4 |   void *create_buffer(int n)
>            |      |         ^~~~~~~~~~~~~
>            |      |         |
>            |      |         (3) entry to ‘create_buffer’
>            |    5 |   {
>            |    6 |     return malloc(n);
>            |      |            ~~~~~~~~~
>            |      |            |
>            |      |            (4) allocated 42 bytes here
>            |
>          ‘main’: event 5
>            |
>            |   10 |     int *buf = create_buffer(42);
>            |      |                ^~~~~~~~~~~~~~~~~
>            |      |                |
>            |      |                (5) Assigned to ‘int *’ here
>            |
>            
> The correct warning should be:
> /path/to/.c:10:16: warning: Allocated buffer size is not a multiple of
> the pointee's size [CWE-131] [-Wanalyzer-allocation-size]
>    10 |     int *buf = create_buffer(42);
>       |                ^~~~~~~~~~~~~~~~~
>   ‘main’: events 1-2
>     |
>     |    9 |   int main (int argc, char **argv) {
>     |      |       ^~~~
>     |      |       |
>     |      |       (1) entry to ‘main’
>     |   10 |     int *buf = create_buffer(42);
>     |      |                ~~~~~~~~~~~~~~~~~
>     |      |                |
>     |      |                (2) calling ‘create_buffer’ from ‘main’
>     |
>     +------------> ‘create_buffer’: events 3-4
>                    |
>                    |    4 |   void *create_buffer(int n)
>                    |      |         ^~~~~~~~~~~~~
>                    |      |         |
>                    |      |         (3) entry to ‘create_buffer’
>                    |    5 |   {
>                    |    6 |     return malloc(n);
>                    |      |            ~~~~~~~~~
>                    |      |            |
>                    |      |            (4) allocated 42 bytes here
>                    |
> ‘main’: event 5 <--+
>    |
>    |   10 |     int *buf = create_buffer(42);
>    |      |                ^~~~~~~~~~~~~~~~~
>    |      |                |
>    |      |                (5) Assigned to ‘int *’ here
>    |
> For that, the return edge has to be visited to be part of the
> emission_path.
> This is currently not the case as the assignment of the <return_value>
> to
> the caller lhs is handled inside pop_frame, which is transitively
> called
> from program_state::on_edge of the 'after' node of the callee.
> I tried to defer the set_value(caller lhs, <return_value>) call to the
> 'before' node after the return edge but failed to do elegantly. My last
> try
> is in the patch commented out with // FIXME.
> My main problem is that I can not pop the frame and later get the
> return value easily. Deferring the whole pop_frame to the before node
> breaks the assumptions inside exploded_graph::get_or_create_node.
> 
> I don't know what's the best/elegant way of solving this. Is a solution
> to
> attach the return svalue to the return edge and then use it later in
> the
> PK_BEFORE_SUPERNODE?

The ctxt is created here:

#5  0x00000000012f5856 in ana::program_state::on_edge (this=this@entry=0x7fffffffc8c0, eg=..., enode=enode@entry=0x2d8d970, 
    succ=succ@entry=0x2d0e590, uncertainty=uncertainty@entry=0x7fffffffc990) at ../../src/gcc/analyzer/program-state.cc:1035
1035	  if (!m_region_model->maybe_update_for_edge (*succ,
(gdb) list
1030	  impl_region_model_context ctxt (eg, enode,
1031					  &enode->get_state (),
1032					  this,
1033					  uncertainty, NULL,
1034					  last_stmt);
1035	  if (!m_region_model->maybe_update_for_edge (*succ,
1036						      last_stmt,
1037						      &ctxt, NULL))

I tried another approach: to provide a custom stmt_finder for this
ctxt, which uses the "returning call" stmt for the destination
supernode:

diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc
index 7ad581c7fbd..11554de9484 100644
--- a/gcc/analyzer/program-state.cc
+++ b/gcc/analyzer/program-state.cc
@@ -996,6 +996,29 @@ program_state::get_current_function () const
   return m_region_model->get_current_function ();
 }
 
+// FIXME
+
+class returning_call_stmt_finder : public stmt_finder
+{
+public:
+  returning_call_stmt_finder (const superedge *succ): m_succ (succ) {}
+
+  stmt_finder *clone () const final override
+  {
+    return new returning_call_stmt_finder (m_succ);
+  }
+  const gimple *find_stmt (const exploded_path &) final override
+  {
+    if (m_succ->m_dest)
+      if (m_succ->m_dest->get_returning_call ())
+       return m_succ->m_dest->get_returning_call ();
+    return NULL;
+  }
+
+private:
+  const superedge *m_succ;
+};
+
 /* Determine if following edge SUCC from ENODE is valid within the graph EG
    and update this state accordingly in-place.
 
@@ -1018,6 +1041,8 @@ program_state::on_edge (exploded_graph &eg,
   const program_point &point = enode->get_point ();
   const gimple *last_stmt = point.get_supernode ()->get_last_stmt ();
 
+  returning_call_stmt_finder stmt_finder (succ);
+
   /* For conditionals and switch statements, add the
      relevant conditions (for the specific edge) to new_state;
      skip edges for which the resulting constraints
@@ -1031,7 +1056,7 @@ program_state::on_edge (exploded_graph &eg,
                                  &enode->get_state (),
                                  this,
                                  uncertainty, NULL,
-                                 last_stmt);
+                                 last_stmt, &stmt_finder);
   if (!m_region_model->maybe_update_for_edge (*succ,
                                              last_stmt,
                                              &ctxt, NULL))

Doing so leads to this output:

../../src/gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c: In function ‘create_buffer’:
../../src/gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c:15:14: warning: Allocated buffer size is not a multiple of the pointee's size [CWE-131] [-Wanalyzer-allocation-size]
   15 |   int *buf = create_buffer(42); /* { dg-warning "" "" { xfail *-*-* } } */
      |              ^~~~~~~~~~~~~~~~~
  ‘test_1’: events 1-2
    |
    |   12 | void test_1(void)
    |      |      ^~~~~~
    |      |      |
    |      |      (1) entry to ‘test_1’
    |......
    |   15 |   int *buf = create_buffer(42); /* { dg-warning "" "" { xfail *-*-* } } */
    |      |              ~~~~~~~~~~~~~~~~~
    |      |              |
    |      |              (2) calling ‘create_buffer’ from ‘test_1’
    |
    +--> ‘create_buffer’: events 3-4
           |
           |    7 | void *create_buffer(int n)
           |      |       ^~~~~~~~~~~~~
           |      |       |
           |      |       (3) entry to ‘create_buffer’
           |    8 | {
           |    9 |   return malloc(n);
           |      |          ~~~~~~~~~
           |      |          |
           |      |          (4) allocated (long unsigned int)42 here
           |
         ‘test_1’: event 5
           |
           |   15 |   int *buf = create_buffer(42); /* { dg-warning "" "" { xfail *-*-* } } */
           |      |              ^~~~~~~~~~~~~~~~~
           |      |              |
           |      |              (5) Assigned to ‘int *’ here
           |

which fixes the stmt and the enclosing function decl for event 5 (the
assignment to the "int *buf"), but annoyingly the stack depth
information is wrong; I think the saved diagnostic is being associated
with the existing exploded_node (in create_buffer), whereas I want it
to use the supernode for test_1, which doesn't yet have an exploded
node when pop_frame is called.  I have various ideas for tackling this:
- have two contexts for pop_frame: one in the old frame, the other in
the new frame (for the caller)
- generalize stmt_finder so it can also update the supernode to use
- rework pop_frame (I've had to do this before, I've run into issues
like this before).

I think it's best to keep this issue as an expected failure, and file a
bug about it, so that we can tackle it by itself, and not block you
from making further progress on this patch.

Various review comments inline below...


Signed-off-by: Tim Lange <mail@tim-lange.me>
---
 gcc/analyzer/analyzer.opt                     |   4 +
 gcc/analyzer/checker-path.cc                  |  12 +-
 gcc/analyzer/checker-path.h                   |   2 +-
 gcc/analyzer/engine.cc                        |  12 +
 gcc/analyzer/pending-diagnostic.h             |  21 ++
 gcc/analyzer/region-model.cc                  | 322 ++++++++++++++++++
 gcc/analyzer/region-model.h                   |   4 +
 .../gcc.dg/analyzer/allocation-size-1.c       |  63 ++++
 .../gcc.dg/analyzer/allocation-size-2.c       |  44 +++
 .../gcc.dg/analyzer/allocation-size-3.c       |  48 +++
 .../gcc.dg/analyzer/allocation-size-4.c       |  92 +++++
 gcc/testsuite/gcc.dg/analyzer/attr-malloc-6.c |   2 +
 gcc/testsuite/gcc.dg/analyzer/malloc-4.c      |   2 +-
 gcc/testsuite/gcc.dg/analyzer/pr96639.c       |   2 +
 14 files changed, 627 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
 create mode 100644 gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c
 create mode 100644 gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c
 create mode 100644 gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c

diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt
index 4aea52d3a87..f213989e0bb 100644
--- a/gcc/analyzer/analyzer.opt
+++ b/gcc/analyzer/analyzer.opt
@@ -78,6 +78,10 @@ Wanalyzer-malloc-leak
 Common Var(warn_analyzer_malloc_leak) Init(1) Warning
 Warn about code paths in which a heap-allocated pointer leaks.
 
+Wanalyzer-allocation-size
+Common Var(warn_analyzer_allocation_size) Init(1) Warning
+Warn about code paths in which a buffer is assigned to a incompatible
type.
+


Any time we add a new option to analyzer.opt we're going to need to add
corresponding documentation to gcc/doc/invoke.texi.  Grep for some of
the existing analyzer warnings to see examples.


 Wanalyzer-mismatching-deallocation
 Common Var(warn_analyzer_mismatching_deallocation) Init(1) Warning
 Warn about code paths in which the wrong deallocation function is
called.


[...snip...]

diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-
model.cc
index 6b49719d521..acb8bd1bfca 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -73,6 +73,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-ssa-operands.h"
 #include "ssa-iterators.h"
 #include "calls.h"
+#include "print-tree.h"
 
 #if ENABLE_ANALYZER
 
@@ -653,6 +654,66 @@ private:
   tree m_count_cst;
 };
 
+/* Concrete subclass for casts of pointers that lead to trailing
bytes.  */
+
+class dubious_allocation_size
+: public pending_diagnostic_subclass<dubious_allocation_size>
+{
+public:
+  dubious_allocation_size (const region *lhs, const region *rhs, 
+                           const svalue *capacity)
+  : m_lhs(lhs), m_rhs(rhs), m_capacity(capacity) {}
+
+  const char *get_kind () const final override 
+  { 
+    return "dubious_allocation_size"; 
+  }
+
+  bool operator== (const dubious_allocation_size &other) const
+  {
+    return m_lhs == other.m_lhs && m_rhs == other.m_rhs;;
+  }
+
+  int get_controlling_option () const final override
+  {
+    return OPT_Wanalyzer_allocation_size;
+  }
+
+  bool emit (rich_location *rich_loc) final override
+  {
+    diagnostic_metadata m;
+    m.add_cwe (131);
+    return warning_meta (rich_loc, m, get_controlling_option (),
+              "Allocated buffer size is not a multiple of the
pointee's size");

Style nit: our diagnostic messages don't start with a capital letter.

I think this would benefit from a note, via "inform", saying the
sizeof() the pointee; something like:

  bool warned = warning_meta (rich_loc, m, get_controlling_option (),
                              "allocated buffer size is not a"
                              " multiple of the pointee's size");
  if (warned)
    inform (rich_loc->get_location, "%<sizeof(%E)%> is %qE",
                                    etc, etc);
  return warned;

or somesuch.


+  }
+
+  label_text 
+  describe_region_creation_event (const evdesc::region_creation &ev)
final override
+  {
+    // TODO: better way to print the capacity
+    return ev.formatted_print ("allocated %s here", 

Maybe: "allocated here (%s bytes)" ?

+                                          m_capacity-
>get_desc(true).m_buffer);

Annoyingly, label_text doesn't have an automatically working
destructor, due to us (until recently) only being able to use C++98.  
So as written, this leaks memory.  Now that we can use C++11, maybe we
should fix label_text to have a dtor, move assignment, etc, but it's
probably simpler in the short term to simply fix the leak.

+  }
+
+  label_text describe_final_event (const evdesc::final_event &ev)
final override
+  {
+    return ev.formatted_print ("Assigned to %qT here", m_lhs->get_type
());

Style nit: make initial letter of message lower-case.

+  }
+
+  void mark_interesting_stuff (interesting_t *interest) final override
+  {
+    if (m_lhs)
+      interest->add_region_creation (m_lhs);
+    if (m_rhs)
+      interest->add_region_creation (m_rhs);
+  }
+
+private:
+  const region *m_lhs;
+  const region *m_rhs;
+  const svalue *m_capacity;
+};
+
 /* If ASSIGN is a stmt that can be modelled via
      set_value (lhs_reg, SVALUE, CTXT)
    for some SVALUE, get the SVALUE.
@@ -2799,6 +2860,241 @@ region_model::check_region_for_read (const
region *src_reg,
   check_region_access (src_reg, DIR_READ, ctxt);
 }
 
+/* Returns the trailing bytes on dubious allocation sizes.  */
+
+static unsigned HOST_WIDE_INT 
+capacity_compatible_with_type (tree cst, tree pointee_size_tree)
+{
+  unsigned HOST_WIDE_INT pointee_size = TREE_INT_CST_LOW
(pointee_size_tree);
+  if (pointee_size == 0)
+    return 0;
+  unsigned HOST_WIDE_INT alloc_size = TREE_INT_CST_LOW (cst);
+
+  return alloc_size % pointee_size;
+}
+
+/* Visits svalues and checks whether the 
+   size_cst is a operand of the svalue.  */
+
+class size_visitor : public visitor
+{
+public:
+  size_visitor(tree size_cst, const svalue *sval, constraint_manager
*cm) 
+  : m_size_cst(size_cst), m_sval(sval), m_cm(cm)
+  {
+    sval->accept(this);
+  }
+
+  bool get_result()
+  {
+    /* The result_set gradually builts from atomtic nodes upwards. If
a node is

Typo: atomtic -> atomic

+       in the result_set, itself or one/all of its children have an
operand that
+       is a multiple of the size_cst. If the root is inside, the given
sval 
+       is valid aka a multiple of the size_cst.*/
+    return result_set.contains(m_sval);
+  }
+
+  void 
+  visit_constant_svalue (const constant_svalue *sval) final override
+  {
+    unsigned HOST_WIDE_INT sval_int
+         = TREE_INT_CST_LOW (sval->get_constant ());
+    unsigned HOST_WIDE_INT size_cst_int = TREE_INT_CST_LOW
(m_size_cst);
+    if (size_cst_int == 0 || sval_int % size_cst_int == 0)
+      result_set.add (sval);
+  }
+
+  void 
+  visit_unknown_svalue (const unknown_svalue *sval ATTRIBUTE_UNUSED)
+    final override
+  {
+    result_set.add (sval);
+  }
+
+  void 
+  visit_poisoned_svalue (const poisoned_svalue *sval ATTRIBUTE_UNUSED)
+    final override
+  {
+    result_set.add (sval);
+  }
+  
+  void visit_unaryop_svalue (const unaryop_svalue *sval) 
+  {
+    const svalue *arg = sval->get_arg ();
+    arg->accept (this);
+    if (result_set.contains (arg))
+       result_set.add (sval);
+  }
+
+  void visit_binop_svalue (const binop_svalue *sval) final override
+  {
+    const svalue *arg0 = sval->get_arg0 ();
+    const svalue *arg1 = sval->get_arg1 ();
+
+    arg0->accept (this);
+    arg1->accept (this);
+    if (sval->get_op () == MULT_EXPR)
+      {
+       if (result_set.contains (arg0) || result_set.contains (arg1))
+         result_set.add (sval);
+      }
+    else
+      {
+       if (result_set.contains (arg0) && result_set.contains (arg1))
+         result_set.add (sval);
+      }
+  }
+
+  void visit_repeated_svalue (const repeated_svalue *sval) 
+  {
+    sval->get_inner_svalue ()->accept(this);
+    if (result_set.contains (sval->get_inner_svalue ()))
+      result_set.add (sval);
+  }
+
+  void visit_unmergeable_svalue (const unmergeable_svalue *sval) final
override
+  {
+    sval->get_arg ()->accept (this);
+    if (result_set.contains (sval->get_arg ()))
+      result_set.add (sval);
+  }
+
+  void visit_widening_svalue (const widening_svalue *sval) final
override
+  {
+    const svalue *base = sval->get_base_svalue ();
+    const svalue *iter = sval->get_iter_svalue ();
+
+    base->accept(this);
+    iter->accept(this);
+    if (result_set.contains (base) && result_set.contains (iter))
+      result_set.add (sval);
+  }
+
+  void visit_conjured_svalue (const conjured_svalue *sval
ATTRIBUTE_UNUSED) 
+    final override
+  {
+    if (m_cm->get_equiv_class_by_svalue (sval, NULL))
+      result_set.add (sval);
+  }
+
+  void visit_asm_output_svalue (const asm_output_svalue *sval
ATTRIBUTE_UNUSED) 
+    final override
+  {
+    // TODO: Should we do something else than assume it could be
correct
+    result_set.add (sval);

I think we just have to assume it is.

+  }
+
+  void visit_const_fn_result_svalue (const const_fn_result_svalue 
+                                     *sval ATTRIBUTE_UNUSED) final
override
+  {
+    // TODO: Should we do something else than assume it could be
correct
+    result_set.add (sval);

Probably here as well.

+  }
+
+private:
+  tree m_size_cst;
+  const svalue *m_sval;
+  constraint_manager *m_cm;
+  svalue_set result_set; /* Used as a mapping of svalue*->bool.  */
+};
+
+/* Returns true if there is a constant tree with 
+   the same constant value inside the sval.  */
+
+static bool
+const_operand_in_sval_p (tree type_size_cst, const svalue *sval,
+                         constraint_manager *cm)
+{
+  size_visitor v(type_size_cst, sval, cm);
+  // sval->accept(&v);
+  return v.get_result ();
+}
+
+/* Special handling for structs with "inheritance" or that hold an
unbounded 
+     type. Those will be skipped to prevent false positives.  */
+
+static bool
+struct_or_union_with_inheritance_p (tree maybe_struct)
+{
+  if (RECORD_OR_UNION_TYPE_P (maybe_struct))
+    {
+      tree iter = TYPE_FIELDS (maybe_struct);
+      if (iter == NULL_TREE)
+        return false;
+      if (RECORD_OR_UNION_TYPE_P (TREE_TYPE (iter)))
+        return true;
+
+      tree last_field;
+      while (iter != NULL_TREE)
+        {
+          last_field = iter;
+          iter = DECL_CHAIN (iter);
+        }
+
+      if (last_field != NULL_TREE 
+          && COMPLETE_OR_UNBOUND_ARRAY_TYPE_P (TREE_TYPE
(last_field)))
+        return true;
+    }
+  return false;
+}
+
+void
+region_model::check_region_size (const region *lhs_reg, const svalue
*rhs_sval,
+                                                  region_model_context
*ctxt) const
+{

Please add a comment immediately before this function summarizing its
intent.

+  if (!ctxt)
+    return;
+  
+  const region_svalue *reg_sval = dyn_cast <const region_svalue *>
(rhs_sval);
+  if (!reg_sval)
+    return;
+
+  tree pointer_type = lhs_reg->get_type ();
+  if (pointer_type == NULL_TREE || !POINTER_TYPE_P (pointer_type))
+    return;
+
+  tree pointee_type = TREE_TYPE (pointer_type);
+  /* void * is always compatible and make sure that the pointee_type
actually
+     has a size, or else size_in_bytes might fail.  */
+  if (pointee_type == NULL_TREE || VOID_TYPE_P (pointee_type) 
+      || TYPE_SIZE_UNIT (pointee_type) == NULL_TREE)
+    return;
+  if (struct_or_union_with_inheritance_p (pointee_type))
+    return;
+
+  tree pointee_size_tree = size_in_bytes(pointee_type);
+  /* The size might be unknown e.g. being a array with n elements
+     or casting to char * never has any trailing bytes.  */
+  if (TREE_CODE (pointee_size_tree) != INTEGER_CST
+      || TREE_INT_CST_LOW (pointee_size_tree) == 1)
+    return;
+
+  const svalue *capacity = get_capacity (reg_sval->get_pointee ());
+  switch (capacity->get_kind ())
+    {
+    case svalue_kind::SK_CONSTANT:
+      {
+       const constant_svalue *cap_sval = capacity-
>dyn_cast_constant_svalue ();

You can use:
  as_a <const constant_svalue *> (capacity)
here since we're within the SK_CONSTANT case, avoiding an extra vfunc
call.


+       tree cap = cap_sval->get_constant ();
+       unsigned HOST_WIDE_INT size_diff
+         = capacity_compatible_with_type (cap, pointee_size_tree);
+       if (size_diff != 0)
+         {
+           ctxt->warn (new dubious_allocation_size (lhs_reg, reg_sval-
>get_pointee (), capacity));

Nit: some overlong lines here; please wrap to avoid going over 80
columns.

+         }
+      }
+      break;
+    default:
+      {
+       if (!const_operand_in_sval_p (pointee_size_tree, capacity,
m_constraints))
+         {
+           ctxt->warn (new dubious_allocation_size (lhs_reg, reg_sval-
>get_pointee (), capacity));

Another overlong line.

+         }
+      }
+      break;
+    }
+}
+
 /* Set the value of the region given by LHS_REG to the value given
    by RHS_SVAL.
    Use CTXT to report any warnings associated with writing to
LHS_REG.  */
@@ -2810,6 +3106,8 @@ region_model::set_value (const region *lhs_reg,
const svalue *rhs_sval,
   gcc_assert (lhs_reg);
   gcc_assert (rhs_sval);
 
+  check_region_size(lhs_reg, rhs_sval, ctxt);
+
   check_region_for_write (lhs_reg, ctxt);
 
   m_store.set_value (m_mgr->get_store_manager(), lhs_reg, rhs_sval,

[...snip...]

diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
new file mode 100644
index 00000000000..cb3df5516e7
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c
@@ -0,0 +1,63 @@
+#include <stdlib.h>
+
+/* Tests with constant buffer sizes.  */
+
+void test_1 (void)
+{
+  short *ptr = malloc (21 * sizeof(short));
+  free (ptr);
+}
+
+void test_2 (void)
+{
+  int *ptr = malloc (21 * sizeof (short)); /* { dg-line malloc2 } */
+  free (ptr);
+
+  /* { dg-warning "" "" { target *-*-* } malloc2 } */
+  /* { dg-message "" "" { target *-*-* } malloc2 } */
+}
+
+void test_3 (void)
+{
+  void *ptr = malloc (21 * sizeof (short));
+  short *sptr = (short *)ptr;
+  free (sptr);
+}
+
+void test_4 (void)
+{
+  void *ptr = malloc (21 * sizeof (short)); /* { dg-message } */
+  int *iptr = (int *)ptr; /* { dg-line assign } */
+  free (iptr);
+
+  /* { dg-warning "" "" { target *-*-* } assign } */
+  /* { dg-message "" "" { target *-*-* } assign } */
+}
+
+struct s {
+  int i;
+};
+
+void test_5 (void)
+{
+  struct s *ptr = malloc (5 * sizeof (struct s));
+  free (ptr);
+}
+
+void test_6 (void)
+{
+  long *ptr = malloc (5 * sizeof (struct s));  /* { dg-line malloc6 }
*/
+  free (ptr);
+
+  /* { dg-warning "" "" { target *-*-* } malloc6 } */
+  /* { dg-message "" "" { target *-*-* } malloc6 } */
+}
+
+void test_7 (void)
+{
+  char buf[2];
+  int *ptr = (int *)buf; /* { dg-line malloc7 } */
+
+  /* { dg-warning "" "" { target *-*-* } malloc7 } */
+  /* { dg-message "" "" { target *-*-* } malloc7 } */
+}
\ No newline at end of file
diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c
b/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c
new file mode 100644
index 00000000000..a619a786a4e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c
@@ -0,0 +1,44 @@
+#include <stdlib.h>
+#include <stdio.h>
+
+/* Tests with symbolic buffer sizes.  */
+
+void test_1 (void)
+{
+  int n;
+  scanf("%i", &n);
+  short *ptr = malloc (n * sizeof(short));
+  free (ptr);
+}
+
+void test_2 (void)
+{
+  int n;
+  scanf("%i", &n);
+  int *ptr = malloc (n * sizeof (short)); /* { dg-line malloc } */
+  free (ptr);
+
+  /* { dg-warning "" "" { target *-*-* } malloc } */
+  /* { dg-message "" "" { target *-*-* } malloc } */
+}
+
+void test_3 (void)
+{
+  int n;
+  scanf("%i", &n);
+  void *ptr = malloc (n * sizeof (short));
+  short *sptr = (short *)ptr;
+  free (sptr);
+}
+
+void test_4 (void)
+{
+  int n;
+  scanf("%i", &n);
+  void *ptr = malloc (n * sizeof (short)); /* { dg-message } */
+  int *iptr = (int *)ptr; /* { dg-line assign } */
+  free (iptr);
+
+  /* { dg-warning "" "" { target *-*-* } assign } */
+  /* { dg-message "" "" { target *-*-* } assign } */
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c
b/gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c
new file mode 100644
index 00000000000..dafc0e73c63
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c
@@ -0,0 +1,48 @@
+#include <stdlib.h>
+#include <stdio.h>
+
+/* CWE-131 example 5 */
+void test_1(void) 
+{
+  int *id_sequence = (int *) malloc (3); /* { dg-line malloc1 } */
+  if (id_sequence == NULL) exit (1);
+
+  id_sequence[0] = 13579;
+  id_sequence[1] = 24680;
+  id_sequence[2] = 97531;
+
+  free (id_sequence);
+
+  /* { dg-warning "" "" { target *-*-* } malloc1 } */
+  /* { dg-message "" "" { target *-*-* } malloc1 } */
+}
+
+void test_2(void)
+{
+  int *ptr = malloc (10 + sizeof(int)); /* { dg-line malloc2 } */
+  free (ptr);
+
+  /* { dg-warning "" "" { target *-*-* } malloc2 } */
+  /* { dg-message "" "" { target *-*-* } malloc2 } */
+}
+
+void test_3(void)
+{
+  int n;
+  scanf("%i", &n);
+  int *ptr = malloc (n + sizeof (int)); /* { dg-line malloc3 } */
+  free (ptr);
+
+  /* { dg-warning "" "" { target *-*-* } malloc3 } */
+  /* { dg-message "" "" { target *-*-* } malloc3 } */
+}
+
+void test_4(void)
+{
+  int n;
+  scanf("%i", &n);
+  int m;
+  scanf("%i", &m);
+  int *ptr = malloc ((n + m) * sizeof (int));
+  free (ptr);
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c
b/gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c
new file mode 100644
index 00000000000..32e14bad6ec
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c
@@ -0,0 +1,92 @@
+#include <stddef.h>
+#include <stdlib.h>
+#include <stdio.h>
+
+/* Flow warnings */
+
+void *create_buffer(int n)
+{
+  return malloc(n);
+}
+
+void test_1(void) 
+{
+  // FIXME
+  int *buf = create_buffer(42); /* { dg-warning "" "" { xfail *-*-* }
} */
+  free (buf);
+}
+
+void test_2(void) 
+{
+  void *buf = create_buffer(42); /* { dg-message } */
+  int *ibuf = buf; /* { dg-line assign2 } */
+  free (ibuf);
+
+  /* { dg-warning "" "" { target *-*-* } assign2 } */
+  /* { dg-message "" "" { target *-*-* } assign2 } */
+}
+
+void test_3(void)
+{
+  void *buf = malloc(42); /* { dg-message } */
+  if (buf != NULL) /* { dg-message } */
+    {
+      int *ibuf = buf; /* { dg-line assign3 } */
+      free (ibuf);
+    }
+
+  /* { dg-warning "" "" { target *-*-* } assign3 } */
+  /* { dg-message "" "" { target *-*-* } assign3 } */
+}
+
+void test_4(void)
+{
+  int n;
+  scanf("%i", &n);
+
+  int size;
+  if (n == 0)
+    size = 1;
+  else if (n == 1)
+    size = 10;
+  else
+    size = 20;
+
+  int *buf = malloc(size); // Size should be 'unknown' at this point
+  free (buf);
+}
+
+void test_5(void)
+{
+  int n;
+  scanf("%i", &n);
+
+  int size;
+  if (n == 0)
+    size = 2;
+  else
+    size = 10;
+
+  short *buf = malloc(size); // Size should be widened to 2 and 10,
both fit
+  free (buf);
+}
+
+
+void test_6(void)
+{
+  int n;
+  scanf("%i", &n);
+
+  int size;
+  if (n == 0)
+    size = 1;
+  else
+    size = 10;
+
+  short *buf = malloc(size); /* { dg-line malloc6 } */
+  free (buf);
+  
+
+  /* { dg-warning "" "" { target *-*-* } malloc6 } */
+  /* { dg-message "" "" { target *-*-* } malloc6 } */
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/attr-malloc-6.c
b/gcc/testsuite/gcc.dg/analyzer/attr-malloc-6.c
index bd28107d0d7..8fa6a6eb570 100644
--- a/gcc/testsuite/gcc.dg/analyzer/attr-malloc-6.c
+++ b/gcc/testsuite/gcc.dg/analyzer/attr-malloc-6.c
@@ -1,7 +1,9 @@
+/* { dg-additional-options -Wno-analyzer-allocation-size } */
 /* Adapted from gcc.dg/Wmismatched-dealloc.c.  */
 
 #define A(...) __attribute__ ((malloc (__VA_ARGS__)))
 
+struct FILE;
 typedef struct FILE   FILE;
 typedef __SIZE_TYPE__ size_t;
 

Hopefully this change isn't needed anymore.

diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-4.c
b/gcc/testsuite/gcc.dg/analyzer/malloc-4.c
index 908bb28ee50..f9a73c79403 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-4.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-4.c
@@ -1,4 +1,4 @@
-/* { dg-additional-options "-Wno-incompatible-pointer-types" } */
+/* { dg-additional-options "-Wno-incompatible-pointer-types -Wno-
analyzer-allocation-size" } */
 
 #include <stdlib.h>

Why is this change needed?  Is this another left-over change from
fixing that stray error?

 
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr96639.c
b/gcc/testsuite/gcc.dg/analyzer/pr96639.c
index 02ca3f084a2..6f365c3cb5d 100644
--- a/gcc/testsuite/gcc.dg/analyzer/pr96639.c
+++ b/gcc/testsuite/gcc.dg/analyzer/pr96639.c
@@ -1,3 +1,5 @@
+/* { dg-additional-options -Wno-analyzer-allocation-size } */
+
 void *calloc (__SIZE_TYPE__, __SIZE_TYPE__);


I added this testcase in 42c5ae5d7f0ad89b75d93c497fe44b6c66da7e76 to
fix a crash due to a NULL type.

Rather than add -Wno-analyzer-allocation-size, please fix the size
passed to the calloc call.

Thanks; hope this is constructive
Dave