Re: [PATCH 2/2] analyzer: out-of-bounds checker [PR106000]

[David Malcolm <dmalcolm@redhat.com>]

On Tue, 2022-08-09 at 23:19 +0200, Tim Lange wrote:
> This patch adds an experimental out-of-bounds checker to the
> analyzer.
> 
> The checker was tested on coreutils, curl, httpd and openssh. It is
> mostly
> accurate but does produce false-positives on yacc-generated files and
> sometimes when the analyzer misses an invariant. These cases will be
> documented in bugzilla.
> (Regrtests still running with the latest changes, will report back
> later.)

Hi Tim, thanks for the patch, and for all the testing you've done on
it.

We've already had several rounds of review of this off-list, and this
patch looks very close to ready.

Some nits below...

> diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt
> index 5021376b6fb..8e73af60ceb 100644
> --- a/gcc/analyzer/analyzer.opt
> +++ b/gcc/analyzer/analyzer.opt
> @@ -158,6 +158,10 @@ Wanalyzer-tainted-size
>  Common Var(warn_analyzer_tainted_size) Init(1) Warning
>  Warn about code paths in which an unsanitized value is used as a
> size.
>  
> +Wanalyzer-out-of-bounds
> +Common Var(warn_analyzer_out_of_bounds) Init(1) Warning
> +Warn about code paths in which a write or read to a buffer is out-
> of-bounds.
> +

Please keep the list alphabetized; I think this needs to be between
  Wanalyzer-mismatching-deallocation 
and 
  Wanalyzer-possible-null-argument

>  Wanalyzer-use-after-free
>  Common Var(warn_analyzer_use_after_free) Init(1) Warning
>  Warn about code paths in which a freed value is used.
> diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-
> model.cc
> index f7df2fca245..2f9382ed96c 100644
> --- a/gcc/analyzer/region-model.cc
> +++ b/gcc/analyzer/region-model.cc
> @@ -1268,6 +1268,402 @@ region_model::on_stmt_pre (const gimple
> *stmt,
>      }
>  }
>  
> +/* Abstract base class for all out-of-bounds warnings.  */
> +
> +class out_of_bounds : public
> pending_diagnostic_subclass<out_of_bounds>
> +{
> +public:
> +  out_of_bounds (const region *reg, tree diag_arg, byte_range range)
> +  : m_reg (reg), m_diag_arg (diag_arg), m_range (range)
> +  {}
> +
> +  const char *get_kind () const final override
> +  {
> +    return "out_of_bounds_diagnostic";
> +  }
> +
> +  bool operator== (const out_of_bounds &other) const
> +  {
> +    return m_reg == other.m_reg
> +          && m_range == other.m_range
> +          && pending_diagnostic::same_tree_p (m_diag_arg,
> other.m_diag_arg);
> +  }
> +
> +  int get_controlling_option () const final override
> +  {
> +    return OPT_Wanalyzer_out_of_bounds;
> +  }
> +
> +  void mark_interesting_stuff (interesting_t *interest) final
> override
> +  {
> +    interest->add_region_creation (m_reg);
> +  }
> +
> +protected:
> +  const region *m_reg;
> +  tree m_diag_arg;
> +  byte_range m_range;

Please add a comment clarifying what the meaning of m_range is here. 
Is it
(a) the range of all bytes that are accessed,
(b) the range of bytes that are accessed out-of-bounds,
(c) etc?

From my reading of the patch I think it's (b).


> +};
> +
> +/* Abstract subclass to complaing about out-of-bounds
> +   past the end of the buffer.  */
> +
> +class past_the_end : public out_of_bounds
> +{
> +public:
> +  past_the_end (const region *reg, tree diag_arg, byte_range range,
> +               tree byte_bound)
> +  : out_of_bounds (reg, diag_arg, range), m_byte_bound (byte_bound)
> +  {}
> +
> +  bool operator== (const past_the_end &other) const
> +  {
> +    return m_reg == other.m_reg
> +          && m_range == other.m_range
> +          && pending_diagnostic::same_tree_p (m_diag_arg,
> other.m_diag_arg)

Is it possible to call
  out_of_bounds::operator== 
for the first three fields, rather than a copy-and-paste of the logic?

> +          && pending_diagnostic::same_tree_p (m_byte_bound,
> +                                              other.m_byte_bound);
> +  }
> +
> +  label_text
> +  describe_region_creation_event (const evdesc::region_creation &ev)
> final
> +  override
> +  {
> +    if (m_byte_bound && TREE_CODE (m_byte_bound) == INTEGER_CST)
> +      return ev.formatted_print ("capacity is %E bytes",
> m_byte_bound);
> +
> +    return label_text ();
> +  }
> +
> +protected:
> +  tree m_byte_bound;
> +};

[...snip the concrete subclasses...]

We went through several rounds of review off-list, and I have lots of
ideas for wording tweaks to the patch, but rather than me be a
"backseat driver" (or bikeshedding), I think that that aspect of the
patch is good enough as-is, and I'll make the wording changes myself
once the patch is in trunk.


[...snip...]

> +
> +    if (warned)
> +      {
> +       char num_bytes_past_buf[WIDE_INT_PRINT_BUFFER_SIZE];
> +       print_dec (m_range.m_size_in_bytes, num_bytes_past_buf,
> UNSIGNED);

I think we can use %wu for this, but I can fix this up in a followup.


[...snip...]

> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index fa23fbeaaaa..5ab834af780 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -459,6 +459,7 @@ Objective-C and Objective-C++ Dialects}.
>  -Wno-analyzer-null-dereference @gol
>  -Wno-analyzer-possible-null-argument @gol
>  -Wno-analyzer-possible-null-dereference @gol
> +-Wno-analyzer-out-of-bounds @gol

Please move between
  -Wno-analyzer-null-dereference @gol
and
  -Wno-analyzer-possible-null-argument @gol 
for alphabetization.

>  -Wno-analyzer-shift-count-negative @gol
>  -Wno-analyzer-shift-count-overflow @gol
>  -Wno-analyzer-stale-setjmp-buffer @gol
> @@ -9991,6 +9992,17 @@ This warning requires @option{-fanalyzer},
> which enables it; use
>  This diagnostic warns for paths through the code in which a
>  value known to be NULL is dereferenced.
>  
> +@item -Wno-analyzer-out-of-bounds
> +@opindex Wanalyzer-out-of-bounds
> +@opindex Wno-analyzer-out-of-bounds
> +This warning requires @option{-fanalyzer} to enable it; use
> +@option{-Wno-analyzer-out-of-bounds} to disable it.
> +
> +This diagnostic warns for path through the code in which a buffer is
> +accessed or written out-of-bounds.

Would be good to clarify the limitations: as I understand it:

"The diagnostic only applies for cases where the analyzer is able to
determine a constant size for the buffer.  It warns when any part of a
read or write is definitely before the start of the buffer, or
definitely after the end."

...or somesuch wording.
> +
> +See @url{https://cwe.mitre.org/data/definitions/119.html, CWE-119:
> Improper Restriction of Operations within the Bounds of a Memory
> Buffer}.

Also, please move the new entry to position to keep things
alphabetized.

> +
>  @item -Wno-analyzer-shift-count-negative
>  @opindex Wanalyzer-shift-count-negative
>  @opindex Wno-analyzer-shift-count-negative

[...snip...]

> diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-1.c
> b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-1.c
> new file mode 100644
> index 00000000000..715c8b7460f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-1.c
> @@ -0,0 +1,119 @@
> +#include <stdlib.h>
> +#include <string.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +
> +/* Wanalyzer-out-of-bounds tests for buffer overflows.  */
> +
> +/* Avoid folding of memcpy.  */
> +typedef void * (*memcpy_t) (void *dst, const void *src, size_t n);
> +
> +static memcpy_t __attribute__((noinline))
> +get_memcpy (void)
> +{
> +  return memcpy;
> +}
> +
> +
> +/* Taken from CWE-787.  */
> +void test1 (void)
> +{
> +  int id_sequence[3];
> +
> +  id_sequence[0] = 123;
> +  id_sequence[1] = 234;
> +  id_sequence[2] = 345;
> +  id_sequence[3] = 456; /* { dg-line test1 } */
> +
> +  /* { dg-warning "overflow" "warning" { target *-*-* } test1 } */
> +  /* { dg-message "" "note" { target *-*-* } test1 } */

I see that you've left the regexes mostly blank in the various DejaGnu
directives in these new tests.  Normally I'd want these to be less
vague, but given that I plan to change the wordings in a followup
anyway, this is OK.

[...snip lots of great testcases...]

With the above nits fixed, the patch is OK for trunk (assuming that
your testing doesn't show any problems).

Thanks again for the patch; this feels like a major new feature.
Dave