[PATCH] Forbid section anchors for ASan build (PR sanitizer/81697)

[PATCH] Forbid section anchors for ASan build (PR sanitizer/81697)

[Slava Barinov]

       gcc/
       * varasm.c (use_object_blocks_p): Forbid section anchors for ASan

       gcc/testsuite/
       * g++.dg/asan/global-alignment.cc: New test to test global
       variables alignment.

Signed-off-by: Slava Barinov <v.barinov@samsung.com>
---
 gcc/ChangeLog                                 |  6 ++++++
 gcc/testsuite/ChangeLog                       |  3 +++
 gcc/testsuite/g++.dg/asan/global-alignment.cc | 17 +++++++++++++++++
 gcc/varasm.c                                  |  3 ++-
 4 files changed, 28 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/asan/global-alignment.cc

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index dde91ceea5b..d840825e7c8 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,9 @@
+2017-08-08  Vyacheslav Barinov  <v.barinov@samsung.com>
+
+	PR sanitizer/81697
+	* varasm.c (use_object_blocks_p): Forbid section anchors for ASan
+	build.
+
 2017-08-08  Martin Liska  <mliska@suse.cz>
 
 	* asan.c: Include header files.
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 315af8361df..0a0a5850c74 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,5 +1,8 @@
 2017-08-08  Vyacheslav Barinov  <v.barinov@samsung.com>
 
+	PR sanitizer/81697
+	* g++.dg/asan/global-alignment.cc: New test to test global
+	variables alignment.
 	* g++.dg/asan/asan.exp: Switch on *.cc tests.
 
 2017-08-07  Michael Meissner  <meissner@linux.vnet.ibm.com>
diff --git a/gcc/testsuite/g++.dg/asan/global-alignment.cc b/gcc/testsuite/g++.dg/asan/global-alignment.cc
new file mode 100644
index 00000000000..c011c703ea6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/asan/global-alignment.cc
@@ -0,0 +1,17 @@
+/* { dg-options "-fmerge-all-constants" } */
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-O0" } { "" } } */
+
+#include <string>
+#include <map>
+
+const char kRecoveryInstallString[] = "NEW";
+const char kRecoveryUpdateString[] = "UPDATE";
+const char kRecoveryUninstallationString[] = "UNINSTALL";
+
+const std::map<std::string, int> kStringToRequestMap = {
+  {kRecoveryInstallString, 0},
+  {kRecoveryUpdateString, 0},
+  {kRecoveryUninstallationString, 0},
+};
+/* { dg-final { scan-assembler-times {\.section\s+\.rodata\n(?:(?!\.section).)*\.(string|ascii|asciz)\s+"NEW} 1 } } */
diff --git a/gcc/varasm.c b/gcc/varasm.c
index e0834a1ff3b..dbeb8d9331e 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -346,7 +346,8 @@ get_section (const char *name, unsigned int flags, tree decl)
 static bool
 use_object_blocks_p (void)
 {
-  return flag_section_anchors;
+  return (flag_section_anchors
+	  && !(flag_sanitize & SANITIZE_ADDRESS));
 }
 
 /* Return the object_block structure for section SECT.  Create a new
-- 
2.13.3

Re: [PATCH] Forbid section anchors for ASan build (PR sanitizer/81697)

[Andrew Pinski]

On Mon, Aug 7, 2017 at 11:15 PM, Slava Barinov <v.barinov@samsung.com> wrote:
>        gcc/
>        * varasm.c (use_object_blocks_p): Forbid section anchors for ASan
>
>        gcc/testsuite/
>        * g++.dg/asan/global-alignment.cc: New test to test global
>        variables alignment.


Can you describe this a little bit more?  What is going wrong here?
Is it because there is no red zone between the variables?
Also I noticed you are using .cc as the testcase file name, why don't
you use .C instead and then you won't need the other patch which you
just posted.

Thanks,
Andrew Pinski

>
> Signed-off-by: Slava Barinov <v.barinov@samsung.com>
> ---
>  gcc/ChangeLog                                 |  6 ++++++
>  gcc/testsuite/ChangeLog                       |  3 +++
>  gcc/testsuite/g++.dg/asan/global-alignment.cc | 17 +++++++++++++++++
>  gcc/varasm.c                                  |  3 ++-
>  4 files changed, 28 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/g++.dg/asan/global-alignment.cc
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index dde91ceea5b..d840825e7c8 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,9 @@
> +2017-08-08  Vyacheslav Barinov  <v.barinov@samsung.com>
> +
> +       PR sanitizer/81697
> +       * varasm.c (use_object_blocks_p): Forbid section anchors for ASan
> +       build.
> +
>  2017-08-08  Martin Liska  <mliska@suse.cz>
>
>         * asan.c: Include header files.
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index 315af8361df..0a0a5850c74 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,5 +1,8 @@
>  2017-08-08  Vyacheslav Barinov  <v.barinov@samsung.com>
>
> +       PR sanitizer/81697
> +       * g++.dg/asan/global-alignment.cc: New test to test global
> +       variables alignment.
>         * g++.dg/asan/asan.exp: Switch on *.cc tests.
>
>  2017-08-07  Michael Meissner  <meissner@linux.vnet.ibm.com>
> diff --git a/gcc/testsuite/g++.dg/asan/global-alignment.cc b/gcc/testsuite/g++.dg/asan/global-alignment.cc
> new file mode 100644
> index 00000000000..c011c703ea6
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/asan/global-alignment.cc
> @@ -0,0 +1,17 @@
> +/* { dg-options "-fmerge-all-constants" } */
> +/* { dg-do compile } */
> +/* { dg-skip-if "" { *-*-* } { "-O0" } { "" } } */
> +
> +#include <string>
> +#include <map>
> +
> +const char kRecoveryInstallString[] = "NEW";
> +const char kRecoveryUpdateString[] = "UPDATE";
> +const char kRecoveryUninstallationString[] = "UNINSTALL";
> +
> +const std::map<std::string, int> kStringToRequestMap = {
> +  {kRecoveryInstallString, 0},
> +  {kRecoveryUpdateString, 0},
> +  {kRecoveryUninstallationString, 0},
> +};
> +/* { dg-final { scan-assembler-times {\.section\s+\.rodata\n(?:(?!\.section).)*\.(string|ascii|asciz)\s+"NEW} 1 } } */
> diff --git a/gcc/varasm.c b/gcc/varasm.c
> index e0834a1ff3b..dbeb8d9331e 100644
> --- a/gcc/varasm.c
> +++ b/gcc/varasm.c
> @@ -346,7 +346,8 @@ get_section (const char *name, unsigned int flags, tree decl)
>  static bool
>  use_object_blocks_p (void)
>  {
> -  return flag_section_anchors;
> +  return (flag_section_anchors
> +         && !(flag_sanitize & SANITIZE_ADDRESS));
>  }
>
>  /* Return the object_block structure for section SECT.  Create a new
> --
> 2.13.3
>

Re: [PATCH] Forbid section anchors for ASan build (PR sanitizer/81697)

[Vyacheslav Barinov]

[-- Attachment #1: Type: text/plain, Size: 1860 bytes --]

Hello,

Andrew Pinski <pinskia@gmail.com> writes:

> On Mon, Aug 7, 2017 at 11:15 PM, Slava Barinov <v.barinov@samsung.com> wrote:
>>        gcc/
>>        * varasm.c (use_object_blocks_p): Forbid section anchors for ASan
>>
>>        gcc/testsuite/
>>        * g++.dg/asan/global-alignment.cc: New test to test global
>>        variables alignment.
>
>
> Can you describe this a little bit more?  What is going wrong here?
> Is it because there is no red zone between the variables?
I've described situation in bugzilla PR 81697 with compiled files dumps, but
briefly yes, red zone size is incorrect between global variables.

On certain platforms (we checked at least arm and ppc) the flow is following:
make_decl_rtl() calls create_block_symbol() on decl tree with ASan global
variable describing a string. But then get_variable_section() returns wrong
section (.rodata.str1.4 in my case) because check in asan_protect_global()
returns false due to !DECL_RTL_SET_P check.

When variable is placed not into .rodata, but into .rodata.str ld treats it as
string and just shrinks the large part of red zone silently (gold at least
prints warning about wrong string alignment). But in run time libasan expects
that red zone is still there and reports false positives.

In order to prevent the setting of RTL before ASan handling I tried to
reproduce the x86_64 behavior and found that use_object_blocks_p() returns
false for that platform. Accordingly to the function comment it reports if
'current compilation mode benefits from grouping', and just switching it off
for any ASan builds resolves the issue.

> Also I noticed you are using .cc as the testcase file name, why don't
> you use .C instead and then you won't need the other patch which you
> just posted.
Okay, attaching rebased version.

> Thanks,
> Andrew Pinski
>

Best Regards,
Vyacheslav Barinov

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Forbid-section-anchors-for-ASan-build-PR-sanitizer-8.patch --]
[-- Type: text/x-diff, Size: 2922 bytes --]

From 88b27d5f39a5671a429db52750564245a3b35141 Mon Sep 17 00:00:00 2001
From: Slava Barinov <v.barinov@samsung.com>
Date: Thu, 3 Aug 2017 16:14:59 +0300
Subject: [PATCH] Forbid section anchors for ASan build (PR sanitizer/81697)

       gcc/
       * varasm.c (use_object_blocks_p): Forbid section anchors for ASan

       gcc/testsuite/
       * g++.dg/asan/global-alignment.C: New test to test global
       variables alignment.

Signed-off-by: Slava Barinov <v.barinov@samsung.com>
---
 gcc/ChangeLog                                |  6 ++++++
 gcc/testsuite/ChangeLog                      |  6 ++++++
 gcc/testsuite/g++.dg/asan/global-alignment.C | 17 +++++++++++++++++
 gcc/varasm.c                                 |  3 ++-
 4 files changed, 31 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/asan/global-alignment.C

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index dde91ceea5b..d840825e7c8 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,9 @@
+2017-08-08  Vyacheslav Barinov  <v.barinov@samsung.com>
+
+	PR sanitizer/81697
+	* varasm.c (use_object_blocks_p): Forbid section anchors for ASan
+	build.
+
 2017-08-08  Martin Liska  <mliska@suse.cz>
 
 	* asan.c: Include header files.
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index c2119f478ba..e0d65103d30 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2017-08-08  Vyacheslav Barinov  <v.barinov@samsung.com>
+
+	PR sanitizer/81697
+	* g++.dg/asan/global-alignment.C: New test to test global
+	variables alignment.
+
 2017-08-07  Michael Meissner  <meissner@linux.vnet.ibm.com>
 
 	PR target/81593
diff --git a/gcc/testsuite/g++.dg/asan/global-alignment.C b/gcc/testsuite/g++.dg/asan/global-alignment.C
new file mode 100644
index 00000000000..c011c703ea6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/asan/global-alignment.C
@@ -0,0 +1,17 @@
+/* { dg-options "-fmerge-all-constants" } */
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-O0" } { "" } } */
+
+#include <string>
+#include <map>
+
+const char kRecoveryInstallString[] = "NEW";
+const char kRecoveryUpdateString[] = "UPDATE";
+const char kRecoveryUninstallationString[] = "UNINSTALL";
+
+const std::map<std::string, int> kStringToRequestMap = {
+  {kRecoveryInstallString, 0},
+  {kRecoveryUpdateString, 0},
+  {kRecoveryUninstallationString, 0},
+};
+/* { dg-final { scan-assembler-times {\.section\s+\.rodata\n(?:(?!\.section).)*\.(string|ascii|asciz)\s+"NEW} 1 } } */
diff --git a/gcc/varasm.c b/gcc/varasm.c
index e0834a1ff3b..dbeb8d9331e 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -346,7 +346,8 @@ get_section (const char *name, unsigned int flags, tree decl)
 static bool
 use_object_blocks_p (void)
 {
-  return flag_section_anchors;
+  return (flag_section_anchors
+	  && !(flag_sanitize & SANITIZE_ADDRESS));
 }
 
 /* Return the object_block structure for section SECT.  Create a new
-- 
2.13.3

Re: [PATCH] Forbid section anchors for ASan build (PR sanitizer/81697)

[Jeff Law]

On 08/08/2017 12:46 AM, Vyacheslav Barinov wrote:
> Hello,
> 
> Andrew Pinski <pinskia@gmail.com> writes:
> 
>> On Mon, Aug 7, 2017 at 11:15 PM, Slava Barinov <v.barinov@samsung.com> wrote:
>>>        gcc/
>>>        * varasm.c (use_object_blocks_p): Forbid section anchors for ASan
>>>
>>>        gcc/testsuite/
>>>        * g++.dg/asan/global-alignment.cc: New test to test global
>>>        variables alignment.
>>
>>
>> Can you describe this a little bit more?  What is going wrong here?
>> Is it because there is no red zone between the variables?
> I've described situation in bugzilla PR 81697 with compiled files dumps, but
> briefly yes, red zone size is incorrect between global variables.
> 
> On certain platforms (we checked at least arm and ppc) the flow is following:
> make_decl_rtl() calls create_block_symbol() on decl tree with ASan global
> variable describing a string. But then get_variable_section() returns wrong
> section (.rodata.str1.4 in my case) because check in asan_protect_global()
> returns false due to !DECL_RTL_SET_P check.
> 
> When variable is placed not into .rodata, but into .rodata.str ld treats it as
> string and just shrinks the large part of red zone silently (gold at least
> prints warning about wrong string alignment). But in run time libasan expects
> that red zone is still there and reports false positives.
> 
> In order to prevent the setting of RTL before ASan handling I tried to
> reproduce the x86_64 behavior and found that use_object_blocks_p() returns
> false for that platform. Accordingly to the function comment it reports if
> 'current compilation mode benefits from grouping', and just switching it off
> for any ASan builds resolves the issue.
> 
>> Also I noticed you are using .cc as the testcase file name, why don't
>> you use .C instead and then you won't need the other patch which you
>> just posted.
> Okay, attaching rebased version.
In many ways it'd be better if asan could be made to work with section
anchors.  Doing so means the code we're running for asan more closely
matches what we're running in the normal case.

But I'll defer to Jakub and Marek as they know the sanitizers far better
than I do and are more familiar with the expected tradeoffs.

jeff

Re: [PATCH] Forbid section anchors for ASan build (PR sanitizer/81697)

[Maxim Ostapenko]

+ Jakub

On 09/08/17 19:21, Jeff Law wrote:
> On 08/08/2017 12:46 AM, Vyacheslav Barinov wrote:
>> Hello,
>>
>> Andrew Pinski <pinskia@gmail.com> writes:
>>
>>> On Mon, Aug 7, 2017 at 11:15 PM, Slava Barinov <v.barinov@samsung.com> wrote:
>>>>         gcc/
>>>>         * varasm.c (use_object_blocks_p): Forbid section anchors for ASan
>>>>
>>>>         gcc/testsuite/
>>>>         * g++.dg/asan/global-alignment.cc: New test to test global
>>>>         variables alignment.
>>>
>>> Can you describe this a little bit more?  What is going wrong here?
>>> Is it because there is no red zone between the variables?
>> I've described situation in bugzilla PR 81697 with compiled files dumps, but
>> briefly yes, red zone size is incorrect between global variables.
>>
>> On certain platforms (we checked at least arm and ppc) the flow is following:
>> make_decl_rtl() calls create_block_symbol() on decl tree with ASan global
>> variable describing a string. But then get_variable_section() returns wrong
>> section (.rodata.str1.4 in my case) because check in asan_protect_global()
>> returns false due to !DECL_RTL_SET_P check.
>>
>> When variable is placed not into .rodata, but into .rodata.str ld treats it as
>> string and just shrinks the large part of red zone silently (gold at least
>> prints warning about wrong string alignment). But in run time libasan expects
>> that red zone is still there and reports false positives.
>>
>> In order to prevent the setting of RTL before ASan handling I tried to
>> reproduce the x86_64 behavior and found that use_object_blocks_p() returns
>> false for that platform. Accordingly to the function comment it reports if
>> 'current compilation mode benefits from grouping', and just switching it off
>> for any ASan builds resolves the issue.
>>
>>> Also I noticed you are using .cc as the testcase file name, why don't
>>> you use .C instead and then you won't need the other patch which you
>>> just posted.
>> Okay, attaching rebased version.
> In many ways it'd be better if asan could be made to work with section
> anchors.  Doing so means the code we're running for asan more closely
> matches what we're running in the normal case.

I'm not very familiar with this code, but as far as I can understand, 
the problem is that we have a circular dependency here -- 
categorize_decl_for_section calls asan_protect_global that returns false 
because DECL_RTL is not set at this point and assigns decl to 
SECCAT_RODATA_MERGE_CONST section. But when make_decl_rtl sets up 
SET_DECL_RTL (decl, x) and following asan_protect_global calls will 
return true and we end up with mismatch between section and alignment 
requirements. Perhaps we can add an additional flag to 
asan_protect_global to change its behavior in such cases (e.g. don't 
account for DECL_RTL_SET_P)? In this case we will change the section for 
SECCAT_RODATA without affecting global section anchors flag.

-Maxim

>
> But I'll defer to Jakub and Marek as they know the sanitizers far better
> than I do and are more familiar with the expected tradeoffs.
>
> jeff
>
>
>