Ok, thanks, here is the patch I'm going to install after bootstrap
completes.
On 30/11/17 14:54, Jakub Jelinek wrote:
On Thu, Nov 30, 2017 at 02:38:25PM +0300, Maxim Ostapenko wrote:
Hi Jakub, thanks for review.
I've fixed the issues you've pointed in review.
Regarding a testcase -- I've cooked a runtime test, but it shows FP on
unpatched GCC version only when linking with Gold (because it strips
redzones more aggressively).
I think we can live with that.
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6550,7 +6550,19 @@ categorize_decl_for_section (const_tree decl, int reloc)
ret = reloc == 1 ? SECCAT_DATA_REL_RO_LOCAL : SECCAT_DATA_REL_RO;
else if (reloc || flag_merge_constants < 2
|| ((flag_sanitize & SANITIZE_ADDRESS)
- && asan_protect_global (CONST_CAST_TREE (decl))))
+ /* PR 81697: for architectures that use section anchors we
+ need to ignore DECL_RTL_SET_P (decl) for string constants
+ inside this asan_protect_global call because otherwise
+ we'll wrongly put them into SECCAT_RODATA_MERGE_CONST
+ section, set DECL_RTL (decl) later on and add DECL to
+ protected globals via successive asan_protect_global
+ calls. In this scenario we'll end up with wrong
+ alignment of these strings at runtime and possible ASan
+ false positives. */
+ && asan_protect_global (CONST_CAST_TREE (decl),
+ use_object_blocks_p ()
+ && use_blocks_for_decl_p (
+ CONST_CAST_TREE (decl)))))
Formatting is too bad here. && should go below use_object_block_p..
The opening ( should either go on the next line, like:
use_object_blocks_p ()
&& use_blocks_for_decl_p
(CONST_CAST_TREE (decl)))))
or perhaps better just introduce a temporary somewhere:
else if (VAR_P (decl))
{
+ tree d = CONST_CAST_TREE (decl);
if (bss_initializer_p (decl))
ret = SECCAT_BSS;
and use d instead of CONST_CAST_TREE (decl) later?
Ok with those changes.
Jakub
gcc/ChangeLog:
2017-11-30 Maxim Ostapenko <m.ostape...@samsung.com>
PR sanitizer/81697
* asan.c (asan_protect_global): Add new ignore_decl_rtl_set_p
parameter. Return true if ignore_decl_rtl_set_p is true and other
conditions are satisfied.
* asan.h (asan_protect_global): Add new parameter.
* varasm.c (categorize_decl_for_section): Pass true as second parameter
to asan_protect_global calls.
gcc/testsuite/ChangeLog:
2017-11-30 Maxim Ostapenko <m.ostape...@samsung.com>
PR sanitizer/81697
* c-c++-common/asan/pr81697.c: New test.
diff --git a/gcc/asan.c b/gcc/asan.c
index ca5fceed..873687f 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1605,7 +1605,7 @@ is_odr_indicator (tree decl)
ASAN_RED_ZONE_SIZE bytes. */
bool
-asan_protect_global (tree decl)
+asan_protect_global (tree decl, bool ignore_decl_rtl_set_p)
{
if (!ASAN_GLOBALS)
return false;
@@ -1627,7 +1627,13 @@ asan_protect_global (tree decl)
|| DECL_THREAD_LOCAL_P (decl)
/* Externs will be protected elsewhere. */
|| DECL_EXTERNAL (decl)
- || !DECL_RTL_SET_P (decl)
+ /* PR sanitizer/81697: For architectures that use section anchors first
+ call to asan_protect_global may occur before DECL_RTL (decl) is set.
+ We should ignore DECL_RTL_SET_P then, because otherwise the first call
+ to asan_protect_global will return FALSE and the following calls on the
+ same decl after setting DECL_RTL (decl) will return TRUE and we'll end
+ up with inconsistency at runtime. */
+ || (!DECL_RTL_SET_P (decl) && !ignore_decl_rtl_set_p)
/* Comdat vars pose an ABI problem, we can't know if
the var that is selected by the linker will have
padding or not. */
@@ -1651,14 +1657,18 @@ asan_protect_global (tree decl)
|| is_odr_indicator (decl))
return false;
- rtl = DECL_RTL (decl);
- if (!MEM_P (rtl) || GET_CODE (XEXP (rtl, 0)) != SYMBOL_REF)
- return false;
- symbol = XEXP (rtl, 0);
+ if (!ignore_decl_rtl_set_p || DECL_RTL_SET_P (decl))
+ {
- if (CONSTANT_POOL_ADDRESS_P (symbol)
- || TREE_CONSTANT_POOL_ADDRESS_P (symbol))
- return false;
+ rtl = DECL_RTL (decl);
+ if (!MEM_P (rtl) || GET_CODE (XEXP (rtl, 0)) != SYMBOL_REF)
+ return false;
+ symbol = XEXP (rtl, 0);
+
+ if (CONSTANT_POOL_ADDRESS_P (symbol)
+ || TREE_CONSTANT_POOL_ADDRESS_P (symbol))
+ return false;
+ }
if (lookup_attribute ("weakref", DECL_ATTRIBUTES (decl)))
return false;
diff --git a/gcc/asan.h b/gcc/asan.h
index c82d4d9..885b47e 100644
--- a/gcc/asan.h
+++ b/gcc/asan.h
@@ -26,7 +26,7 @@ extern void asan_finish_file (void);
extern rtx_insn *asan_emit_stack_protection (rtx, rtx, unsigned int,
HOST_WIDE_INT *, tree *, int);
extern rtx_insn *asan_emit_allocas_unpoison (rtx, rtx, rtx_insn *);
-extern bool asan_protect_global (tree);
+extern bool asan_protect_global (tree, bool ignore_decl_rtl_set_p = false);
extern void initialize_sanitizer_builtins (void);
extern tree asan_dynamic_init_call (bool);
extern bool asan_expand_check_ifn (gimple_stmt_iterator *, bool);
diff --git a/gcc/testsuite/c-c++-common/asan/pr81697.c b/gcc/testsuite/c-c++-common/asan/pr81697.c
new file mode 100644
index 0000000..3a85813
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/pr81697.c
@@ -0,0 +1,20 @@
+/* { dg-options "-fmerge-all-constants" } */
+/* { dg-do run } */
+/* { dg-skip-if "" { *-*-* } { "-O0" } { "" } } */
+
+const char kRecoveryInstallString[] = "NEW";
+const char kRecoveryUpdateString[] = "UPDATE";
+const char kRecoveryUninstallationString1[] = "INSTALL";
+const char kRecoveryUninstallationString2[] = "UNINSTALL";
+
+volatile const int zero = 0;
+
+int
+main()
+{
+ char x1 = kRecoveryInstallString[zero + 0];
+ char x2 = kRecoveryUpdateString[zero + 0];
+ char x3 = kRecoveryUninstallationString1[zero + 0];
+ char x4 = kRecoveryUninstallationString2[zero + 0];
+ return (x1 + x2 + x3 + x4) == 0;
+}
diff --git a/gcc/varasm.c b/gcc/varasm.c
index 0c7b26e..392ac44 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6530,6 +6530,7 @@ categorize_decl_for_section (const_tree decl, int reloc)
}
else if (VAR_P (decl))
{
+ tree d = CONST_CAST_TREE (decl);
if (bss_initializer_p (decl))
ret = SECCAT_BSS;
else if (! TREE_READONLY (decl)
@@ -6550,7 +6551,17 @@ categorize_decl_for_section (const_tree decl, int reloc)
ret = reloc == 1 ? SECCAT_DATA_REL_RO_LOCAL : SECCAT_DATA_REL_RO;
else if (reloc || flag_merge_constants < 2
|| ((flag_sanitize & SANITIZE_ADDRESS)
- && asan_protect_global (CONST_CAST_TREE (decl))))
+ /* PR 81697: for architectures that use section anchors we
+ need to ignore DECL_RTL_SET_P (decl) for string constants
+ inside this asan_protect_global call because otherwise
+ we'll wrongly put them into SECCAT_RODATA_MERGE_CONST
+ section, set DECL_RTL (decl) later on and add DECL to
+ protected globals via successive asan_protect_global
+ calls. In this scenario we'll end up with wrong
+ alignment of these strings at runtime and possible ASan
+ false positives. */
+ && asan_protect_global (d, use_object_blocks_p ()
+ && use_blocks_for_decl_p (d))))
/* C and C++ don't allow different variables to share the same
location. -fmerge-all-constants allows even that (at the
expense of not conforming). */
