* [Ping][PATCH v3] Fix Incorrect ASan global variables alignment on arm (PR sanitizer/81697) [not found] <CGME20171121084326eucas1p1b6791865d9d8c4d2b17939650b66da45@eucas1p1.samsung.com> @ 2017-11-21 8:57 ` Maxim Ostapenko [not found] ` <CGME20171128070454eucas1p2ecf098de3fc9ced1e4e283b5e24f4c6f@eucas1p2.samsung.com> 0 siblings, 1 reply; 6+ messages in thread From: Maxim Ostapenko @ 2017-11-21 8:57 UTC (permalink / raw) To: GCC Patches [-- Attachment #1: Type: text/plain, Size: 120 bytes --] Hi, I would like to ping the following patch: https://gcc.gnu.org/ml/gcc-patches/2017-10/msg02288.html Thanks, -Maxim [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: pr81697-2.diff --] [-- Type: text/x-diff; name="pr81697-2.diff", Size: 4528 bytes --] gcc/ChangeLog: 2017-11-21 Maxim Ostapenko <m.ostapenko@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-21 Maxim Ostapenko <m.ostapenko@samsung.com> PR sanitizer/81697 * g++.dg/asan/global-alignment.C: New test. diff --git a/gcc/asan.c b/gcc/asan.c index d5128aa..78c3b60 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,6 +1657,9 @@ asan_protect_global (tree decl) || is_odr_indicator (decl)) return false; + if (ignore_decl_rtl_set_p) + return true; + rtl = DECL_RTL (decl); if (!MEM_P (rtl) || GET_CODE (XEXP (rtl, 0)) != SYMBOL_REF) 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/g++.dg/asan/global-alignment.C b/gcc/testsuite/g++.dg/asan/global-alignment.C new file mode 100644 index 0000000..84dac37 --- /dev/null +++ b/gcc/testsuite/g++.dg/asan/global-alignment.C @@ -0,0 +1,18 @@ +/* { 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).)*\.\w+\s+"NEW} 1 } } */ diff --git a/gcc/varasm.c b/gcc/varasm.c index a139151..849eae0 100644 --- a/gcc/varasm.c +++ b/gcc/varasm.c @@ -6508,7 +6508,7 @@ categorize_decl_for_section (const_tree decl, int reloc) else if (TREE_CODE (decl) == STRING_CST) { if ((flag_sanitize & SANITIZE_ADDRESS) - && asan_protect_global (CONST_CAST_TREE (decl))) + && asan_protect_global (CONST_CAST_TREE (decl), true)) /* or !flag_merge_constants */ return SECCAT_RODATA; else @@ -6536,7 +6536,7 @@ 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)))) + && asan_protect_global (CONST_CAST_TREE (decl), true))) /* 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). */ ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <CGME20171128070454eucas1p2ecf098de3fc9ced1e4e283b5e24f4c6f@eucas1p2.samsung.com>]
* Re: [Ping][PATCH v3] Fix Incorrect ASan global variables alignment on arm (PR sanitizer/81697) [not found] ` <CGME20171128070454eucas1p2ecf098de3fc9ced1e4e283b5e24f4c6f@eucas1p2.samsung.com> @ 2017-11-28 7:42 ` Maxim Ostapenko 2017-11-29 11:05 ` Jakub Jelinek 0 siblings, 1 reply; 6+ messages in thread From: Maxim Ostapenko @ 2017-11-28 7:42 UTC (permalink / raw) To: GCC Patches; +Cc: Jakub Jelinek, Ramana Radhakrishnan [-- Attachment #1: Type: text/plain, Size: 213 bytes --] (CC'ing Jakub and Ramana) Hi, I would like to ping the following patch: https://gcc.gnu.org/ml/gcc-patches/2017-10/msg02288.html Fix Incorrect ASan global variables alignment on arm (PR sanitizer/81697) -Maxim [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: pr81697-2.diff --] [-- Type: text/x-diff; name="pr81697-2.diff", Size: 4528 bytes --] gcc/ChangeLog: 2017-11-28 Maxim Ostapenko <m.ostapenko@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-28 Maxim Ostapenko <m.ostapenko@samsung.com> PR sanitizer/81697 * g++.dg/asan/global-alignment.C: New test. diff --git a/gcc/asan.c b/gcc/asan.c index d5128aa..78c3b60 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,6 +1657,9 @@ asan_protect_global (tree decl) || is_odr_indicator (decl)) return false; + if (ignore_decl_rtl_set_p) + return true; + rtl = DECL_RTL (decl); if (!MEM_P (rtl) || GET_CODE (XEXP (rtl, 0)) != SYMBOL_REF) 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/g++.dg/asan/global-alignment.C b/gcc/testsuite/g++.dg/asan/global-alignment.C new file mode 100644 index 0000000..84dac37 --- /dev/null +++ b/gcc/testsuite/g++.dg/asan/global-alignment.C @@ -0,0 +1,18 @@ +/* { 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).)*\.\w+\s+"NEW} 1 } } */ diff --git a/gcc/varasm.c b/gcc/varasm.c index a139151..849eae0 100644 --- a/gcc/varasm.c +++ b/gcc/varasm.c @@ -6508,7 +6508,7 @@ categorize_decl_for_section (const_tree decl, int reloc) else if (TREE_CODE (decl) == STRING_CST) { if ((flag_sanitize & SANITIZE_ADDRESS) - && asan_protect_global (CONST_CAST_TREE (decl))) + && asan_protect_global (CONST_CAST_TREE (decl), true)) /* or !flag_merge_constants */ return SECCAT_RODATA; else @@ -6536,7 +6536,7 @@ 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)))) + && asan_protect_global (CONST_CAST_TREE (decl), true))) /* 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). */ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Ping][PATCH v3] Fix Incorrect ASan global variables alignment on arm (PR sanitizer/81697) 2017-11-28 7:42 ` Maxim Ostapenko @ 2017-11-29 11:05 ` Jakub Jelinek 2017-11-30 11:55 ` Maxim Ostapenko 0 siblings, 1 reply; 6+ messages in thread From: Jakub Jelinek @ 2017-11-29 11:05 UTC (permalink / raw) To: Maxim Ostapenko; +Cc: GCC Patches, Ramana Radhakrishnan On Tue, Nov 28, 2017 at 10:04:51AM +0300, Maxim Ostapenko wrote: > (CC'ing Jakub and Ramana) > > I would like to ping the following patch: > https://gcc.gnu.org/ml/gcc-patches/2017-10/msg02288.html > Fix Incorrect ASan global variables alignment on arm (PR sanitizer/81697) > > -Maxim > gcc/ChangeLog: > > 2017-11-28 Maxim Ostapenko <m.ostapenko@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-28 Maxim Ostapenko <m.ostapenko@samsung.com> > > PR sanitizer/81697 > * g++.dg/asan/global-alignment.C: New test. > > diff --git a/gcc/asan.c b/gcc/asan.c > index d5128aa..78c3b60 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) This part is fine. > /* 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,6 +1657,9 @@ asan_protect_global (tree decl) > || is_odr_indicator (decl)) > return false; > > + if (ignore_decl_rtl_set_p) > + return true; This isn't, because then you bypass checks that really don't care about RTL, like: if (lookup_attribute ("weakref", DECL_ATTRIBUTES (decl))) return false; if (!TARGET_SUPPORTS_ALIASES && asan_needs_local_alias (decl)) return false; So, IMHO just wrap only the DECL_RTL/symbol related stuff in if (!ignore_decl_rtl_set_p). Perhaps even better in if (!ignore_decl_rtl_set_p || DECL_RTL_SET_P (decl)) so that if the rtl is already set, you do the check anyway. > --- /dev/null > +++ b/gcc/testsuite/g++.dg/asan/global-alignment.C > @@ -0,0 +1,18 @@ > +/* { 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).)*\.\w+\s+"NEW} 1 } } */ I don't really like the testcase. The scaning for .rodata section is too fragile, e.g. darwin will need something completely different, doesn't e.g. powerpc use .sdata section instead, etc. And, isn't std::map and std::string completely unnecessary? I'd prefer a runtime test that shows the false positive without the patch, preferrably in C, just with those const char arrays used by some C code without any headers. > diff --git a/gcc/varasm.c b/gcc/varasm.c > index a139151..849eae0 100644 > --- a/gcc/varasm.c > +++ b/gcc/varasm.c > @@ -6508,7 +6508,7 @@ categorize_decl_for_section (const_tree decl, int reloc) > else if (TREE_CODE (decl) == STRING_CST) > { > if ((flag_sanitize & SANITIZE_ADDRESS) > - && asan_protect_global (CONST_CAST_TREE (decl))) > + && asan_protect_global (CONST_CAST_TREE (decl), true)) This doesn't make sense to me. asan_protect_global for STRING_CST doesn't care about any RTL, nor -fsection-anchors puts them into any kind of block. > /* or !flag_merge_constants */ > return SECCAT_RODATA; > else > @@ -6536,7 +6536,7 @@ 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)))) > + && asan_protect_global (CONST_CAST_TREE (decl), true))) I don't like this, there is no reason to change behavior for targets without section anchors, or when it is disabled, or when decl is not going to be put into any. So, perhaps && asan_protect_global (CONST_CAST_TREE (decl), use_object_blocks_p () && use_blocks_for_decl_p (decl)))) instead, with a comment why? > /* 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). */ Jakub ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Ping][PATCH v3] Fix Incorrect ASan global variables alignment on arm (PR sanitizer/81697) 2017-11-29 11:05 ` Jakub Jelinek @ 2017-11-30 11:55 ` Maxim Ostapenko 2017-11-30 11:55 ` Jakub Jelinek 0 siblings, 1 reply; 6+ messages in thread From: Maxim Ostapenko @ 2017-11-30 11:55 UTC (permalink / raw) To: Jakub Jelinek; +Cc: GCC Patches, Ramana Radhakrishnan [-- Attachment #1: Type: text/plain, Size: 6018 bytes --] 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). This patch passes tests on arm-linux and x86_64-linux, bootstrap is in progress. Thanks, -Maxim On 29/11/17 13:10, Jakub Jelinek wrote: > On Tue, Nov 28, 2017 at 10:04:51AM +0300, Maxim Ostapenko wrote: >> (CC'ing Jakub and Ramana) >> >> I would like to ping the following patch: >> https://gcc.gnu.org/ml/gcc-patches/2017-10/msg02288.html >> Fix Incorrect ASan global variables alignment on arm (PR sanitizer/81697) >> >> -Maxim >> gcc/ChangeLog: >> >> 2017-11-28 Maxim Ostapenko <m.ostapenko@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-28 Maxim Ostapenko <m.ostapenko@samsung.com> >> >> PR sanitizer/81697 >> * g++.dg/asan/global-alignment.C: New test. >> >> diff --git a/gcc/asan.c b/gcc/asan.c >> index d5128aa..78c3b60 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) > This part is fine. > >> /* 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,6 +1657,9 @@ asan_protect_global (tree decl) >> || is_odr_indicator (decl)) >> return false; >> >> + if (ignore_decl_rtl_set_p) >> + return true; > This isn't, because then you bypass checks that really don't care > about RTL, like: > if (lookup_attribute ("weakref", DECL_ATTRIBUTES (decl))) > return false; > > if (!TARGET_SUPPORTS_ALIASES && asan_needs_local_alias (decl)) > return false; > > So, IMHO just wrap only the DECL_RTL/symbol related stuff in if > (!ignore_decl_rtl_set_p). Perhaps even better in > if (!ignore_decl_rtl_set_p || DECL_RTL_SET_P (decl)) > so that if the rtl is already set, you do the check anyway. > >> --- /dev/null >> +++ b/gcc/testsuite/g++.dg/asan/global-alignment.C >> @@ -0,0 +1,18 @@ >> +/* { 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).)*\.\w+\s+"NEW} 1 } } */ > I don't really like the testcase. The scaning for .rodata section is too > fragile, e.g. darwin will need something completely different, doesn't e.g. > powerpc use .sdata section instead, etc. > > And, isn't std::map and std::string completely unnecessary? > I'd prefer a runtime test that shows the false positive without the patch, > preferrably in C, just with those const char arrays used by some C code > without any headers. > >> diff --git a/gcc/varasm.c b/gcc/varasm.c >> index a139151..849eae0 100644 >> --- a/gcc/varasm.c >> +++ b/gcc/varasm.c >> @@ -6508,7 +6508,7 @@ categorize_decl_for_section (const_tree decl, int reloc) >> else if (TREE_CODE (decl) == STRING_CST) >> { >> if ((flag_sanitize & SANITIZE_ADDRESS) >> - && asan_protect_global (CONST_CAST_TREE (decl))) >> + && asan_protect_global (CONST_CAST_TREE (decl), true)) > This doesn't make sense to me. asan_protect_global for STRING_CST doesn't > care about any RTL, nor -fsection-anchors puts them into any kind of > block. > >> /* or !flag_merge_constants */ >> return SECCAT_RODATA; >> else >> @@ -6536,7 +6536,7 @@ 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)))) >> + && asan_protect_global (CONST_CAST_TREE (decl), true))) > I don't like this, there is no reason to change behavior for > targets without section anchors, or when it is disabled, or when decl > is not going to be put into any. > So, perhaps > && asan_protect_global (CONST_CAST_TREE (decl), > use_object_blocks_p () > && use_blocks_for_decl_p (decl)))) > instead, with a comment why? > >> /* 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). */ > > Jakub > > > [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: pr81697-3.diff --] [-- Type: text/x-diff; name="pr81697-3.diff", Size: 5321 bytes --] gcc/ChangeLog: 2017-11-30 Maxim Ostapenko <m.ostapenko@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.ostapenko@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]; + char x2 = kRecoveryUpdateString[zero]; + char x3 = kRecoveryUninstallationString1[zero]; + char x4 = kRecoveryUninstallationString2[zero]; + return (x1 + x2 + x3 + x4) == 0; +} diff --git a/gcc/varasm.c b/gcc/varasm.c index 0c7b26e..bac02d5 100644 --- 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))))) /* 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). */ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Ping][PATCH v3] Fix Incorrect ASan global variables alignment on arm (PR sanitizer/81697) 2017-11-30 11:55 ` Maxim Ostapenko @ 2017-11-30 11:55 ` Jakub Jelinek 2017-11-30 12:47 ` Maxim Ostapenko 0 siblings, 1 reply; 6+ messages in thread From: Jakub Jelinek @ 2017-11-30 11:55 UTC (permalink / raw) To: Maxim Ostapenko; +Cc: GCC Patches, Ramana Radhakrishnan 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Ping][PATCH v3] Fix Incorrect ASan global variables alignment on arm (PR sanitizer/81697) 2017-11-30 11:55 ` Jakub Jelinek @ 2017-11-30 12:47 ` Maxim Ostapenko 0 siblings, 0 replies; 6+ messages in thread From: Maxim Ostapenko @ 2017-11-30 12:47 UTC (permalink / raw) To: Jakub Jelinek; +Cc: GCC Patches, Ramana Radhakrishnan [-- Attachment #1: Type: text/plain, Size: 2125 bytes --] 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 > > > [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: pr81697-4.diff --] [-- Type: text/x-diff; name="pr81697-4.diff", Size: 5538 bytes --] gcc/ChangeLog: 2017-11-30 Maxim Ostapenko <m.ostapenko@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.ostapenko@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). */ ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-11-30 12:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20171121084326eucas1p1b6791865d9d8c4d2b17939650b66da45@eucas1p1.samsung.com>
2017-11-21 8:57 ` [Ping][PATCH v3] Fix Incorrect ASan global variables alignment on arm (PR sanitizer/81697) Maxim Ostapenko
[not found] ` <CGME20171128070454eucas1p2ecf098de3fc9ced1e4e283b5e24f4c6f@eucas1p2.samsung.com>
2017-11-28 7:42 ` Maxim Ostapenko
2017-11-29 11:05 ` Jakub Jelinek
2017-11-30 11:55 ` Maxim Ostapenko
2017-11-30 11:55 ` Jakub Jelinek
2017-11-30 12:47 ` Maxim Ostapenko
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).