Re: [Ping][PATCH v3] Fix Incorrect ASan global variables alignment on arm (PR sanitizer/81697)

[Jakub Jelinek <jakub@redhat.com>]

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