diff mbox series

[committed,2/7] analyzer: fix wording of 'number of bad bytes' note [PR106626]

Message ID 20221201024200.3722982-2-dmalcolm@redhat.com
State New
Headers show
Series [committed,1/7] analyzer: move bounds checking to a new bounds-checking.cc | expand

Commit Message

David Malcolm Dec. 1, 2022, 2:41 a.m. UTC 
Consider -fanalyzer on:

#include <stdint.h>

int32_t arr[10];

void int_arr_write_element_after_end_far(int32_t x)
{
  arr[100] = x;
}

Trunk x86_64: https://godbolt.org/z/7GqEcYGq6

Currently we emit:

<source>: In function 'int_arr_write_element_after_end_far':
<source>:7:12: warning: buffer overflow [CWE-787] [-Wanalyzer-out-of-bounds]
    7 |   arr[100] = x;
      |   ~~~~~~~~~^~~
  event 1
    |
    |    3 | int32_t arr[10];
    |      |         ^~~
    |      |         |
    |      |         (1) capacity is 40 bytes
    |
    +--> 'int_arr_write_element_after_end_far': events 2-3
           |
           |    5 | void int_arr_write_element_after_end_far(int32_t x)
           |      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |      |
           |      |      (2) entry to 'int_arr_write_element_after_end_far'
           |    6 | {
           |    7 |   arr[100] = x;
           |      |   ~~~~~~~~~~~~
           |      |            |
           |      |            (3) out-of-bounds write from byte 400 till byte 403 but 'arr' ends at byte 40
           |
<source>:7:12: note: write is 4 bytes past the end of 'arr'
    7 |   arr[100] = x;
      |   ~~~~~~~~~^~~

The wording of the final note:
  "write is 4 bytes past the end of 'arr'"
reads to me as if the "4 bytes past" is describing where the access
occurs, which seems wrong, as the write is far beyond the end of the
array.  Looking at the implementation, it's actually describing the
number of bytes within the access that are beyond the bounds of the
buffer.

This patch updates the wording so that the final note reads
  "write of 4 bytes to beyond the end of 'arr'"
which more clearly expresses that it's the size of the access
being described.

The patch also uses inform_n to avoid emitting "1 bytes".

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Pushed to trunk as r13-4426-gd69a95c12cc91e.

gcc/analyzer/ChangeLog:
	PR analyzer/106626
	* bounds-checking.cc (buffer_overflow::emit): Use inform_n.
	Update wording to clarify that we're talking about the size of
	the bad access, rather than its position.
	(buffer_overread::emit): Likewise.

gcc/testsuite/ChangeLog:
	PR analyzer/106626
	* gcc.dg/analyzer/out-of-bounds-read-char-arr.c: Update for
	changes to expected wording.
	* gcc.dg/analyzer/out-of-bounds-read-int-arr.c: Likewise.
	* gcc.dg/analyzer/out-of-bounds-write-char-arr.c: Likewise.
	* gcc.dg/analyzer/out-of-bounds-write-int-arr.c: Likewise.

Signed-off-by: David Malcolm <dmalcolm@redhat.com>
---
 gcc/analyzer/bounds-checking.cc               | 66 ++++++++++++-------
 .../analyzer/out-of-bounds-read-char-arr.c    | 11 +---
 .../analyzer/out-of-bounds-read-int-arr.c     |  8 +--
 .../analyzer/out-of-bounds-write-char-arr.c   | 11 +---
 .../analyzer/out-of-bounds-write-int-arr.c    |  8 +--
 5 files changed, 56 insertions(+), 48 deletions(-)
diff mbox series

Patch

diff --git a/gcc/analyzer/bounds-checking.cc b/gcc/analyzer/bounds-checking.cc
index 19aaa51e6a8..ad7f431ea2f 100644
--- a/gcc/analyzer/bounds-checking.cc
+++ b/gcc/analyzer/bounds-checking.cc
@@ -143,17 +143,28 @@  public:
 
     if (warned)
       {
-	char num_bytes_past_buf[WIDE_INT_PRINT_BUFFER_SIZE];
-	print_dec (m_out_of_bounds_range.m_size_in_bytes,
-		   num_bytes_past_buf, UNSIGNED);
-	if (m_diag_arg)
-	  inform (rich_loc->get_loc (), "write is %s bytes past the end"
-					" of %qE", num_bytes_past_buf,
-						   m_diag_arg);
-	else
-	  inform (rich_loc->get_loc (), "write is %s bytes past the end"
-					"of the region",
-					num_bytes_past_buf);
+	if (wi::fits_uhwi_p (m_out_of_bounds_range.m_size_in_bytes))
+	  {
+	    unsigned HOST_WIDE_INT num_bad_bytes
+	      = m_out_of_bounds_range.m_size_in_bytes.to_uhwi ();
+	    if (m_diag_arg)
+	      inform_n (rich_loc->get_loc (),
+			num_bad_bytes,
+			"write of %wu byte to beyond the end of %qE",
+			"write of %wu bytes to beyond the end of %qE",
+			num_bad_bytes,
+			m_diag_arg);
+	    else
+	      inform_n (rich_loc->get_loc (),
+			num_bad_bytes,
+			"write of %wu byte to beyond the end of the region",
+			"write of %wu bytes to beyond the end of the region",
+			num_bad_bytes);
+	  }
+	else if (m_diag_arg)
+	  inform (rich_loc->get_loc (),
+		  "write to beyond the end of %qE",
+		  m_diag_arg);
       }
 
     return warned;
@@ -212,17 +223,28 @@  public:
 
     if (warned)
       {
-	char num_bytes_past_buf[WIDE_INT_PRINT_BUFFER_SIZE];
-	print_dec (m_out_of_bounds_range.m_size_in_bytes,
-		   num_bytes_past_buf, UNSIGNED);
-	if (m_diag_arg)
-	  inform (rich_loc->get_loc (), "read is %s bytes past the end"
-					" of %qE", num_bytes_past_buf,
-						    m_diag_arg);
-	else
-	  inform (rich_loc->get_loc (), "read is %s bytes past the end"
-					"of the region",
-					num_bytes_past_buf);
+	if (wi::fits_uhwi_p (m_out_of_bounds_range.m_size_in_bytes))
+	  {
+	    unsigned HOST_WIDE_INT num_bad_bytes
+	      = m_out_of_bounds_range.m_size_in_bytes.to_uhwi ();
+	    if (m_diag_arg)
+	      inform_n (rich_loc->get_loc (),
+			num_bad_bytes,
+			"read of %wu byte from after the end of %qE",
+			"read of %wu bytes from after the end of %qE",
+			num_bad_bytes,
+			m_diag_arg);
+	    else
+	      inform_n (rich_loc->get_loc (),
+			num_bad_bytes,
+			"read of %wu byte from after the end of the region",
+			"read of %wu bytes from after the end of the region",
+			num_bad_bytes);
+	  }
+	else if (m_diag_arg)
+	  inform (rich_loc->get_loc (),
+		  "read from after the end of %qE",
+		  m_diag_arg);
       }
 
     return warned;
diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-read-char-arr.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-read-char-arr.c
index 61cbfc75c11..2b43000f833 100644
--- a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-read-char-arr.c
+++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-read-char-arr.c
@@ -32,24 +32,19 @@  char int_arr_read_element_after_end_off_by_one(void)
 {
   return arr[10]; /* { dg-warning "buffer overread" "warning" } */
   /* { dg-message "out-of-bounds read at byte 10 but 'arr' ends at byte 10" "final event" { target *-*-* } .-1 } */
-  /* { dg-message "read is 1 bytes past the end of 'arr'" "note" { target *-*-* } .-2 } */
-  // FIXME(PR 106626): "1 bytes"
+  /* { dg-message "read of 1 byte from after the end of 'arr'" "num bad bytes note" { target *-*-* } .-2 } */
 }
 
 char int_arr_read_element_after_end_near(void)
 {
   return arr[11]; /* { dg-warning "buffer overread" "warning" } */
   /* { dg-message "out-of-bounds read at byte 11 but 'arr' ends at byte 10" "final event" { target *-*-* } .-1 } */
-  /* { dg-message "read is 1 bytes past the end of 'arr'" "note" { target *-*-* } .-2 } */
-  // FIXME(PR 106626): is the note correct?
-  // FIXME(PR 106626): "1 bytes"
+  /* { dg-message "read of 1 byte from after the end of 'arr'" "num bad bytes note" { target *-*-* } .-2 } */
 }
 
 char int_arr_read_element_after_end_far(void)
 {
   return arr[100]; /* { dg-warning "buffer overread" "warning" } */
   /* { dg-message "out-of-bounds read at byte 100 but 'arr' ends at byte 10" "final event" { target *-*-* } .-1 } */
-  /* { dg-message "read is 1 bytes past the end of 'arr'" "note" { target *-*-* } .-2 } */
-  // FIXME(PR 106626): the note seems incorrect (size of access is 1 byte, but magnitude beyond boundary is 90)
-  // FIXME(PR 106626): "1 bytes"
+  /* { dg-message "read of 1 byte from after the end of 'arr'" "num bad bytes note" { target *-*-* } .-2 } */
 }
diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-read-int-arr.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-read-int-arr.c
index 0bb30d24e9f..0dc95563620 100644
--- a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-read-int-arr.c
+++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-read-int-arr.c
@@ -34,21 +34,19 @@  int32_t int_arr_read_element_after_end_off_by_one(void)
 {
   return arr[10]; /* { dg-warning "buffer overread" "warning" } */
   /* { dg-message "out-of-bounds read from byte 40 till byte 43 but 'arr' ends at byte 40" "final event" { target *-*-* } .-1 } */
-  /* { dg-message "read is 4 bytes past the end of 'arr'" "note" { target *-*-* } .-2 } */
+  /* { dg-message "read of 4 bytes from after the end of 'arr'" "num bad bytes note" { target *-*-* } .-2 } */
 }
 
 int32_t int_arr_read_element_after_end_near(void)
 {
   return arr[11]; /* { dg-warning "buffer overread" "warning" } */
   /* { dg-message "out-of-bounds read from byte 44 till byte 47 but 'arr' ends at byte 40" "final event" { target *-*-* } .-1 } */
-  /* { dg-message "read is 4 bytes past the end of 'arr'" "note" { target *-*-* } .-2 } */
-  // FIXME(PR 106626): is the note correct?
+  /* { dg-message "read of 4 bytes from after the end of 'arr'" "num bad bytes note" { target *-*-* } .-2 } */
 }
 
 int32_t int_arr_read_element_after_end_far(void)
 {
   return arr[100]; /* { dg-warning "buffer overread" "warning" } */
   /* { dg-message "out-of-bounds read from byte 400 till byte 403 but 'arr' ends at byte 40" "final event" { target *-*-* } .-1 } */
-  /* { dg-message "read is 4 bytes past the end of 'arr'" "note" { target *-*-* } .-2 } */
-  // FIXME(PR 106626): the note seems incorrect (size of access is 4 bytes, but magnitude beyond boundary is 390-393)
+  /* { dg-message "read of 4 bytes from after the end of 'arr'" "num bad bytes note" { target *-*-* } .-2 } */
 }
diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-write-char-arr.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-write-char-arr.c
index 47fbc5207ee..3564476c322 100644
--- a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-write-char-arr.c
+++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-write-char-arr.c
@@ -32,24 +32,19 @@  void int_arr_write_element_after_end_off_by_one(char x)
 {
   arr[10] = x; /* { dg-warning "buffer overflow" "warning" } */
   /* { dg-message "out-of-bounds write at byte 10 but 'arr' ends at byte 10" "final event" { target *-*-* } .-1 } */
-  /* { dg-message "write is 1 bytes past the end of 'arr'" "note" { target *-*-* } .-2 } */
-  // FIXME(PR 106626): "1 bytes"
+  /* { dg-message "write of 1 byte to beyond the end of 'arr'" "num bad bytes note" { target *-*-* } .-2 } */
 }
 
 void int_arr_write_element_after_end_near(char x)
 {
   arr[11] = x; /* { dg-warning "buffer overflow" "warning" } */
   /* { dg-message "out-of-bounds write at byte 11 but 'arr' ends at byte 10" "final event" { target *-*-* } .-1 } */
-  /* { dg-message "write is 1 bytes past the end of 'arr'" "note" { target *-*-* } .-2 } */
-  // FIXME(PR 106626): is the note correct?
-  // FIXME(PR 106626): "1 bytes"
+  /* { dg-message "write of 1 byte to beyond the end of 'arr'" "num bad bytes note" { target *-*-* } .-2 } */
 }
 
 void int_arr_write_element_after_end_far(char x)
 {
   arr[100] = x; /* { dg-warning "buffer overflow" "warning" } */
   /* { dg-message "out-of-bounds write at byte 100 but 'arr' ends at byte 10" "final event" { target *-*-* } .-1 } */
-  /* { dg-message "write is 1 bytes past the end of 'arr'" "note" { target *-*-* } .-2 } */
-  // FIXME(PR 106626): the note seems incorrect (size of access is 1 byte, but magnitude beyond boundary is 90)
-  // FIXME(PR 106626): "1 bytes"
+  /* { dg-message "write of 1 byte to beyond the end of 'arr'" "num bad bytes note" { target *-*-* } .-2 } */
 }
diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-write-int-arr.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-write-int-arr.c
index bf9760ee978..24a9a6bfa18 100644
--- a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-write-int-arr.c
+++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-write-int-arr.c
@@ -34,21 +34,19 @@  void int_arr_write_element_after_end_off_by_one(int32_t x)
 {
   arr[10] = x; /* { dg-warning "buffer overflow" "warning" } */
   /* { dg-message "out-of-bounds write from byte 40 till byte 43 but 'arr' ends at byte 40" "final event" { target *-*-* } .-1 } */
-  /* { dg-message "write is 4 bytes past the end of 'arr'" "note" { target *-*-* } .-2 } */
+  /* { dg-message "write of 4 bytes to beyond the end of 'arr'" "num bad bytes note" { target *-*-* } .-2 } */
 }
 
 void int_arr_write_element_after_end_near(int32_t x)
 {
   arr[11] = x; /* { dg-warning "buffer overflow" "warning" } */
   /* { dg-message "out-of-bounds write from byte 44 till byte 47 but 'arr' ends at byte 40" "final event" { target *-*-* } .-1 } */
-  /* { dg-message "write is 4 bytes past the end of 'arr'" "note" { target *-*-* } .-2 } */
-  // FIXME(PR 106626): is the note correct?
+  /* { dg-message "write of 4 bytes to beyond the end of 'arr'" "num bad bytes note" { target *-*-* } .-2 } */
 }
 
 void int_arr_write_element_after_end_far(int32_t x)
 {
   arr[100] = x; /* { dg-warning "buffer overflow" "warning" } */
   /* { dg-message "out-of-bounds write from byte 400 till byte 403 but 'arr' ends at byte 40" "final event" { target *-*-* } .-1 } */
-  /* { dg-message "write is 4 bytes past the end of 'arr'" "note" { target *-*-* } .-2 } */
-  // FIXME(PR 106626): the note seems incorrect (size of access is 4 bytes, but magnitude beyond boundary is 390-393)
+  /* { dg-message "write of 4 bytes to beyond the end of 'arr'" "num bad bytes note" { target *-*-* } .-2 } */
 }