diff mbox

Fix for invalid sanitization of trailing byte in __builtin_strlen

Message ID 53A411A8.3000907@partner.samsung.com
State New
Headers show

Commit Message

max June 20, 2014, 10:49 a.m. UTC 
This patch fixes invalid sanitization of trailing byte in 
__builtin_strlen ( https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61547).

Tested on x86_64-unknown-linux-gnu, no regressions.

Ok to commit?

-Maxim

Comments

Jakub Jelinek June 20, 2014, 10:57 a.m. UTC | #1 
On Fri, Jun 20, 2014 at 02:49:12PM +0400, Maxim Ostapenko wrote:
> This patch fixes invalid sanitization of trailing byte in __builtin_strlen (
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61547).
> 
> Tested on x86_64-unknown-linux-gnu, no regressions.
> +
> +#include <sanitizer/asan_interface.h>
> +
> +char a[2] = "0";
> +
> +#ifdef __cplusplus
> +extern "C"
> +#endif
> +__SIZE_TYPE__ strlen (const char *p) {
> +  /* Simulate error */
> +  if (p == a)
> +    return 1;

Why this?  Can't you instead just use __attribute__((no_sanitize_address, noinline))
on it instead?

Otherwise looks good to me

	Jakub
Yuri Gribov June 20, 2014, 11:39 a.m. UTC | #2 
On Fri, Jun 20, 2014 at 2:57 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Jun 20, 2014 at 02:49:12PM +0400, Maxim Ostapenko wrote:
>> This patch fixes invalid sanitization of trailing byte in __builtin_strlen (
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61547).
>>
>> Tested on x86_64-unknown-linux-gnu, no regressions.

What about bootstrap though?

>> +__SIZE_TYPE__ strlen (const char *p) {
>> +  /* Simulate error */
>> +  if (p == a)
>> +    return 1;
>
> Why this?  Can't you instead just use __attribute__((no_sanitize_address, noinline))
> on it instead?

Yeah, good point.

-Y
diff mbox

Patch

gcc/ChangeLog:

2014-06-20  Yury Gribov  <y.gribov@samsung.com>
	    Max Ostapenko  <m.ostapenko@partner.samsung.com>

	PR sanitizer/61547
	* asan.c (instrument_strlen_call): Fixed instrumentation of
	trailing byte.

gcc/testsuite/ChangeLog:

2014-06-20  Yury Gribov  <y.gribov@samsung.com>
	    Max Ostapenko  <m.ostapenko@partner.samsung.com>

	PR sanitizer/61547
	* c-c++-common/asan/strlen-overflow-1.c: New test.


diff --git a/gcc/asan.c b/gcc/asan.c
index 281a795..71c063b 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -2036,19 +2036,19 @@  instrument_strlen_call (gimple_stmt_iterator *iter)
 
   build_check_stmt (loc, gimple_assign_lhs (str_arg_ssa), NULL_TREE, 1, iter,
 		    /*non_zero_len_p*/true, /*before_p=*/true,
-		    /*is_store=*/false, /*is_scalar_access*/false, /*align*/0);
+		    /*is_store=*/false, /*is_scalar_access*/true, /*align*/0);
 
-  gimple stmt =
-    gimple_build_assign_with_ops (PLUS_EXPR,
-				  make_ssa_name (TREE_TYPE (len), NULL),
-				  len,
-				  build_int_cst (TREE_TYPE (len), 1));
-  gimple_set_location (stmt, loc);
-  gsi_insert_after (iter, stmt, GSI_NEW_STMT);
+  gimple g =
+    gimple_build_assign_with_ops (POINTER_PLUS_EXPR,
+				  make_ssa_name (cptr_type, NULL),
+				  gimple_assign_lhs (str_arg_ssa),
+				  len);
+  gimple_set_location (g, loc);
+  gsi_insert_after (iter, g, GSI_NEW_STMT);
 
-  build_check_stmt (loc, gimple_assign_lhs (stmt), len, 1, iter,
+  build_check_stmt (loc, gimple_assign_lhs (g), NULL_TREE, 1, iter,
 		    /*non_zero_len_p*/true, /*before_p=*/false,
-		    /*is_store=*/false, /*is_scalar_access*/false, /*align*/0);
+		    /*is_store=*/false, /*is_scalar_access*/true, /*align*/0);
 
   return true;
 }
diff --git a/gcc/testsuite/c-c++-common/asan/strlen-overflow-1.c b/gcc/testsuite/c-c++-common/asan/strlen-overflow-1.c
new file mode 100644
index 0000000..4c4585e
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/strlen-overflow-1.c
@@ -0,0 +1,31 @@ 
+/* { dg-do run } */
+/* { dg-skip-if "" { *-*-* } { "-flto" } { "" } } */
+/* { dg-shouldfail "asan" } */
+
+#include <sanitizer/asan_interface.h>
+
+char a[2] = "0";
+
+#ifdef __cplusplus
+extern "C"
+#endif
+__SIZE_TYPE__ strlen (const char *p) {
+  /* Simulate error */
+  if (p == a)
+    return 1;
+
+  __SIZE_TYPE__ n = 0;
+  for (; *p; ++n, ++p);
+  return n;
+}
+
+int main () {
+  char *p = &a[0];
+  asm ("" : "+r"(p));
+  __asan_poison_memory_region ((char *)&a[1], 1);
+  return __builtin_strlen (a);
+}
+
+/* { dg-output "READ of size 1 at 0x\[0-9a-f\]+ thread T0.*(\n|\r\n|\r)" } */
+/* { dg-output "    #0 0x\[0-9a-f\]+ (in _*main (\[^\n\r]*strlen-overflow-1.c:26|\[^\n\r]*:0)|\[(\]).*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*0x\[0-9a-f\]+ is located 1 bytes inside of global variable" } */