errno can't alias locals (PR 92412)
diff mbox series

Message ID 5311b2b4-3822-1996-5f80-e4afa2ea51f7@gmail.com
State New
Headers show
Series
  • errno can't alias locals (PR 92412)
Related show

Commit Message

Martin Sebor Nov. 11, 2019, 10:38 p.m. UTC
The conditional in default_ref_may_alias_errno has the function
return true even for local variables, implying that locals must
be assumed not to have been changed across calls to errno-setting
functions like malloc.  This leads to both worse code and also
false negatives in the strlen pass' detection of buffer overflow
across such calls.

The attached patch constrains the conditional to only consider
external declarations.

Tested on x86_64-linux.

Martin

Comments

Richard Biener Nov. 12, 2019, 8:04 a.m. UTC | #1
On Mon, Nov 11, 2019 at 11:38 PM Martin Sebor <msebor@gmail.com> wrote:
>
> The conditional in default_ref_may_alias_errno has the function
> return true even for local variables, implying that locals must
> be assumed not to have been changed across calls to errno-setting
> functions like malloc.  This leads to both worse code and also
> false negatives in the strlen pass' detection of buffer overflow
> across such calls.
>
> The attached patch constrains the conditional to only consider
> external declarations.
>
> Tested on x86_64-linux.

OK.

This means a tentative definition for 'errno' is non-conforming?
(besides not working well in practice, of course)

Thanks,
Richard.

>
> Martin
Andreas Schwab Nov. 12, 2019, 10:13 a.m. UTC | #2
On Nov 12 2019, Richard Biener wrote:

> This means a tentative definition for 'errno' is non-conforming?

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html

 If a macro definition is suppressed in order to access an actual
 object, or a program defines an identifier with the name errno, the
 behavior is undefined.

Andreas.
Richard Biener Nov. 13, 2019, 2:49 p.m. UTC | #3
On Tue, Nov 12, 2019 at 11:13 AM Andreas Schwab <schwab@suse.de> wrote:
>
> On Nov 12 2019, Richard Biener wrote:
>
> > This means a tentative definition for 'errno' is non-conforming?
>
> https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html
>
>  If a macro definition is suppressed in order to access an actual
>  object, or a program defines an identifier with the name errno, the
>  behavior is undefined.

To quote the relevant parts before this:

The <errno.h> header shall provide a declaration or definition for
errno. The symbol errno shall expand to a modifiable lvalue of type
int. It is unspecified whether errno is a macro or an identifier
declared with external linkage.

The first sentence suggests errno.h can provide a tentative definition
(since that's also a definition?).  The third sentence gives choice only
between a macro and a declaration with external linkage (that's clearly
_not_ a definition).

So ...

Richard.

> Andreas.
>
> --
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."
Martin Sebor Nov. 13, 2019, 4:44 p.m. UTC | #4
On 11/12/19 1:04 AM, Richard Biener wrote:
> On Mon, Nov 11, 2019 at 11:38 PM Martin Sebor <msebor@gmail.com> wrote:
>>
>> The conditional in default_ref_may_alias_errno has the function
>> return true even for local variables, implying that locals must
>> be assumed not to have been changed across calls to errno-setting
>> functions like malloc.  This leads to both worse code and also
>> false negatives in the strlen pass' detection of buffer overflow
>> across such calls.
>>
>> The attached patch constrains the conditional to only consider
>> external declarations.
>>
>> Tested on x86_64-linux.
> 
> OK.
> 
> This means a tentative definition for 'errno' is non-conforming?
> (besides not working well in practice, of course)

Errno is a reserved name (just like all external identifiers)
so only an implementation can declare it at file scope.

But if you are asking if the patch changes the assumption GCC
makes about errno aliasing tentative definitions, I don't think
it does.  AFAICT, GCC has been making the assumption since 4.5.
Here's a slightly different test case than the one in pr92412
that makes it clearer:

   int errno;

   void* f (void)
   {
     int e = errno;
     void *p = __builtin_malloc (1);
     if (e != errno)   // eliminated
       __builtin_abort ();
     return p;
   }

But to be clear, my goal with the change is to let GCC (strlen
specifically) assume that errno doesn't alias local variables.

Martin

Patch
diff mbox series

PR tree-optimization/92412 - excessive errno aliasing assumption defeats optimization

gcc/ChangeLog:

	PR tree-optimization/92412
	* targhooks.c (default_ref_may_alias_errno): Errono can only alias
	extern variables.

gcc/testsuite/ChangeLog:

	PR tree-optimization/92412
	* gcc.dg/strlenopt-91.c: New test.

Index: gcc/targhooks.c
===================================================================
--- gcc/targhooks.c	(revision 278066)
+++ gcc/targhooks.c	(working copy)
@@ -1415,9 +1415,11 @@  default_ref_may_alias_errno (ao_ref *ref)
   if (TYPE_UNSIGNED (TREE_TYPE (base))
       || TYPE_MODE (TREE_TYPE (base)) != TYPE_MODE (integer_type_node))
     return false;
-  /* The default implementation assumes an errno location
-     declaration is never defined in the current compilation unit.  */
+  /* The default implementation assumes an errno location declaration
+     is never defined in the current compilation unit and may not be
+     aliased by a local variable.  */
   if (DECL_P (base)
+      && DECL_EXTERNAL (base)
       && !TREE_STATIC (base))
     return true;
   else if (TREE_CODE (base) == MEM_REF
Index: gcc/testsuite/gcc.dg/strlenopt-91.c
===================================================================
--- gcc/testsuite/gcc.dg/strlenopt-91.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/strlenopt-91.c	(working copy)
@@ -0,0 +1,124 @@ 
+/* PR tree-optimization/92412 - excessive errno aliasing assumption defeats
+   optimization
+   { dg-do compile }
+   { dg-options "-O1 -Wall -fdump-tree-optimized" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+extern void* alloca (size_t);
+extern void* calloc (size_t, size_t);
+extern void* malloc (size_t);
+
+extern const char exta[4];
+static char stata[] = "123";
+
+void sink (const void*, ...);
+
+#define T(ptr, alloc) do {						\
+    const char *p = ptr;						\
+    if (p[0] != '1' || p[1] != '2' || p[2] != '3' || p[3] != '\0'	\
+	|| __builtin_strlen (p) != 3)					\
+      return;								\
+									\
+    void *q = alloc;							\
+    __builtin_strcpy (q, p);						\
+									\
+    if (p[0] != '1' || p[1] != '2' || p[2] != '3' || p[3] != '\0'	\
+	|| __builtin_strlen (p) != 3					\
+	|| __builtin_strlen (q) != 3)					\
+      __builtin_abort ();						\
+									\
+    sink (p, q);							\
+  } while (0)
+
+
+void alloca_test_local (unsigned n)
+{
+  char loca[] = "123";
+  T (loca, alloca (n));
+}
+
+void alloca_test_extern_const (unsigned n)
+{
+  T (exta, alloca (n));
+}
+
+void alloca_test_static (unsigned n)
+{
+  T (stata, alloca (n));
+}
+
+
+// Verify fix for PR tree-optimization/92412.
+void calloc_test_local (unsigned m, unsigned n)
+{
+  char loca[] = "123";
+  T (loca, calloc (m, n));
+}
+
+void calloc_test_extern_const (unsigned m, unsigned n)
+{
+  T (exta, calloc (m, n));
+}
+
+void calloc_test_static (unsigned m, unsigned n)
+{
+  T (stata, calloc (m, n));
+}
+
+
+// Verify fix for PR tree-optimization/92412.
+void malloc_test_local (unsigned n)
+{
+  char loca[] = "123";
+  T (loca, malloc (n));
+}
+
+void malloc_test_extern_const (unsigned n)
+{
+  T (exta, malloc (n));
+}
+
+void malloc_test_static (unsigned n)
+{
+  T (stata, malloc (n));
+}
+
+
+#undef T
+#define T(ptr, n) do {							\
+    const char *p = ptr;						\
+    if (p[0] != '1' || p[1] != '2' || p[2] != '3' || p[3] != '\0'	\
+	|| __builtin_strlen (p) != 3)					\
+      return;								\
+									\
+    char vla[n];							\
+    char *q = vla;							\
+    __builtin_strcpy (q, p);						\
+									\
+    if (p[0] != '1' || p[1] != '2' || p[2] != '3' || p[3] != '\0'	\
+	|| __builtin_strlen (p) != 3					\
+	|| __builtin_strlen (q) != 3)					\
+      __builtin_abort ();						\
+									\
+    sink (p, vla);							\
+  } while (0)
+
+
+void vla_test_local (unsigned n)
+{
+  char loca[] = "123";
+  T (loca, n);
+}
+
+void vla_test_extern_const (unsigned n)
+{
+  T (exta, n);
+}
+
+void vla_test_static (unsigned n)
+{
+  T (stata, n);
+}
+
+/* { dg-final { scan-tree-dump-not "abort" "optimized" } } */