public inbox for gcc-cvs@sourceware.org
help / color / mirror / Atom feed
From: Jakub Jelinek <jakub@gcc.gnu.org>
To: gcc-cvs@gcc.gnu.org
Subject: [gcc r13-756] asan: Fix up instrumentation of assignments which are both loads and stores [PR105714]
Date: Wed, 25 May 2022 10:08:50 +0000 (GMT)	[thread overview]
Message-ID: <20220525100850.E09EF3856DDC@sourceware.org> (raw)

https://gcc.gnu.org/g:af02daff557a0abbf5521c143f1cdda406848a9b

commit r13-756-gaf02daff557a0abbf5521c143f1cdda406848a9b
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed May 25 12:05:08 2022 +0200

    asan: Fix up instrumentation of assignments which are both loads and stores [PR105714]
    
    On the following testcase with -Os asan pass sees:
      <bb 6> [local count: 354334800]:
      # h_21 = PHI <h_15(6), 0(5)>
      *c.3_5 = *d.2_4;
      h_15 = h_21 + 1;
      if (h_15 != 3)
        goto <bb 6>; [75.00%]
      else
        goto <bb 7>; [25.00%]
    
      <bb 7> [local count: 118111600]:
      *c.3_5 = MEM[(struct a *)&b + 12B];
      _13 = c.3_5->x;
      return _13;
    It instruments the
      *c.3_5 = *d.2_4;
    assignment by adding
      .ASAN_CHECK (7, c.3_5, 4, 4);
      .ASAN_CHECK (6, d.2_4, 4, 4);
    before it (which later lowers to checking the corresponding shadow
    memory).  But when considering instrumentation of
      *c.3_5 = MEM[(struct a *)&b + 12B];
    it doesn't instrument anything, because it sees that *c.3_5 store is
    already instrumented in a dominating block and so there is no need
    to instrument *c.3_5 store again (i.e. add another
      .ASAN_CHECK (7, c.3_5, 4, 4);
    ).  That is true, but misses the fact that we still want to
    instrument the MEM[(struct a *)&b + 12B] load.
    
    The following patch fixes that by changing has_stmt_been_instrumented_p
    to consider both store and load in the assignment if it does both
    (returning true iff both have been instrumented).
    That matches how we handle e.g. builtin calls, where we also perform AND
    of all the memory locs involved in the call.
    
    I've verified that we still don't add the redundant
      .ASAN_CHECK (7, c.3_5, 4, 4);
    call but just add
      _18 = &MEM[(struct a *)&b + 12B];
      .ASAN_CHECK (6, _18, 4, 4);
    to instrument the load.
    
    2022-05-25  Jakub Jelinek  <jakub@redhat.com>
    
            PR sanitizer/105714
            * asan.cc (has_stmt_been_instrumented_p): For assignments which
            are both stores and loads, return true only if both destination
            and source have been instrumented.
    
            * gcc.dg/asan/pr105714.c: New test.

Diff:
---
 gcc/asan.cc                          | 15 ++++++++++++++-
 gcc/testsuite/gcc.dg/asan/pr105714.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/gcc/asan.cc b/gcc/asan.cc
index 4b583e54efd..15b2cf8a94e 100644
--- a/gcc/asan.cc
+++ b/gcc/asan.cc
@@ -1285,7 +1285,20 @@ has_stmt_been_instrumented_p (gimple *stmt)
 
       if (get_mem_ref_of_assignment (as_a <gassign *> (stmt), &r,
 				     &r_is_store))
-	return has_mem_ref_been_instrumented (&r);
+	{
+	  if (!has_mem_ref_been_instrumented (&r))
+	    return false;
+	  if (r_is_store && gimple_assign_load_p (stmt))
+	    {
+	      asan_mem_ref src;
+	      asan_mem_ref_init (&src, NULL, 1);
+	      src.start = gimple_assign_rhs1 (stmt);
+	      src.access_size = int_size_in_bytes (TREE_TYPE (src.start));
+	      if (!has_mem_ref_been_instrumented (&src))
+		return false;
+	    }
+	  return true;
+	}
     }
   else if (gimple_call_builtin_p (stmt, BUILT_IN_NORMAL))
     {
diff --git a/gcc/testsuite/gcc.dg/asan/pr105714.c b/gcc/testsuite/gcc.dg/asan/pr105714.c
new file mode 100644
index 00000000000..d378b8ab0c4
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/asan/pr105714.c
@@ -0,0 +1,33 @@
+/* PR sanitizer/105714 */
+/* { dg-do run } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-Os" } } */
+/* { dg-shouldfail "asan" } */
+
+struct A { int x; };
+struct A b[2];
+struct A *c = b, *d = b;
+int e;
+
+int
+foo ()
+{
+  for (e = 0; e < 1; e++)
+    {
+      int i[1];
+      i;
+    }
+  for (int h = 0; h < 3; h++)
+    *c = *d;
+  *c = *(b + 3);
+  return c->x;
+}
+
+int
+main ()
+{
+  foo ();
+  return 0;
+}
+
+/* { dg-output "ERROR: AddressSanitizer: global-buffer-overflow on address.*(\n|\r\n|\r)" } */
+/* { dg-output "READ of size.*" } */


                 reply	other threads:[~2022-05-25 10:08 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220525100850.E09EF3856DDC@sourceware.org \
    --to=jakub@gcc.gnu.org \
    --cc=gcc-cvs@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).