diff mbox series

handle sanitizer built-ins in -Wuninitialized (PR 101300)

Message ID 44fabafb-9174-1739-9818-53122ef499f9@gmail.com
State New
Headers show
Series handle sanitizer built-ins in -Wuninitialized (PR 101300) | expand

Commit Message

Martin Sebor July 2, 2021, 7:21 p.m. UTC 
To avoid a class of false negatives for sanitized code
-Wuninitialized recognizes the ASAN_MARK internal function
doesn't modify its argument.  But the warning code doesn't do
the same for any sanitizer built-ins even though they don't
modify user-supplied arguments either.  This leaves another
class of false negatives unresolved.

The attached fix enhances the warning logic to recognize all
sanitizer built-ins as well and treat them as non-modifying.

Tested on x86_64-linux.

Martin

Comments

Bernhard Reutner-Fischer July 3, 2021, 10:13 a.m. UTC | #1 
On Fri, 2 Jul 2021 13:21:10 -0600
Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:

> --- a/gcc/tree-ssa-uninit.c
> +++ b/gcc/tree-ssa-uninit.c
> @@ -228,9 +228,26 @@ check_defs (ao_ref *ref, tree vdef, void *data_)
>    gimple *def_stmt = SSA_NAME_DEF_STMT (vdef);
>  
>    /* The ASAN_MARK intrinsic doesn't modify the variable.  */
> -  if (is_gimple_call (def_stmt)
> -      && gimple_call_internal_p (def_stmt, IFN_ASAN_MARK))
> -    return false;
> +  if (is_gimple_call (def_stmt))
> +    {
> +      if (gimple_call_internal_p (def_stmt)
> +         && gimple_call_internal_fn (def_stmt) == IFN_ASAN_MARK)
> +       return false;
> +
> +      if (tree fndecl = gimple_call_fndecl (def_stmt))
> +       {
> +         /* Some sanitizer calls pass integer arguments to built-ins
> +            that expect pointets. Avoid using gimple_call_builtin_p()

pointers

What happened to the suspicion that the fnspec attribs are
allegedly not correct (
https://gcc.gnu.org/legacy-ml/gcc-patches/2018-02/msg00541.html )?

Wouldn't that deal with this issue transparently if additionally the
args were passed in correctly?
Didn't follow *san closely though.

thanks,

> +            which fails for such calls.  */
> +         if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
> +           {
> +             built_in_function fncode = DECL_FUNCTION_CODE (fndecl);
> +             if (fncode > BEGIN_SANITIZER_BUILTINS
> +                 && fncode < END_SANITIZER_BUILTINS)
> +               return false;
> +           }
> +       }
> +    }
>
Martin Sebor July 6, 2021, 3:41 p.m. UTC | #2 
On 7/3/21 4:13 AM, Bernhard Reutner-Fischer wrote:
> On Fri, 2 Jul 2021 13:21:10 -0600
> Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 
>> --- a/gcc/tree-ssa-uninit.c
>> +++ b/gcc/tree-ssa-uninit.c
>> @@ -228,9 +228,26 @@ check_defs (ao_ref *ref, tree vdef, void *data_)
>>     gimple *def_stmt = SSA_NAME_DEF_STMT (vdef);
>>   
>>     /* The ASAN_MARK intrinsic doesn't modify the variable.  */
>> -  if (is_gimple_call (def_stmt)
>> -      && gimple_call_internal_p (def_stmt, IFN_ASAN_MARK))
>> -    return false;
>> +  if (is_gimple_call (def_stmt))
>> +    {
>> +      if (gimple_call_internal_p (def_stmt)
>> +         && gimple_call_internal_fn (def_stmt) == IFN_ASAN_MARK)
>> +       return false;
>> +
>> +      if (tree fndecl = gimple_call_fndecl (def_stmt))
>> +       {
>> +         /* Some sanitizer calls pass integer arguments to built-ins
>> +            that expect pointets. Avoid using gimple_call_builtin_p()
> 
> pointers
> 
> What happened to the suspicion that the fnspec attribs are
> allegedly not correct (
> https://gcc.gnu.org/legacy-ml/gcc-patches/2018-02/msg00541.html )?
> 
> Wouldn't that deal with this issue transparently if additionally the
> args were passed in correctly?
> Didn't follow *san closely though.

I didn't follow the 2018 thread either but the attribute changes
to ASAN_MARK discussed in the patch above ended up reverted in
r257625.  In general, the fnspec (and other similar) attributes
need to capture even the side-effects of the sanitizer functions
that aren't observable by a program.  Warnings only care about
the strictly observable subset of those, so there isn't always
a 1-to-1 correspondence.

Martin


> 
> thanks,
> 
>> +            which fails for such calls.  */
>> +         if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
>> +           {
>> +             built_in_function fncode = DECL_FUNCTION_CODE (fndecl);
>> +             if (fncode > BEGIN_SANITIZER_BUILTINS
>> +                 && fncode < END_SANITIZER_BUILTINS)
>> +               return false;
>> +           }
>> +       }
>> +    }
>>
Martin Sebor July 12, 2021, 6:06 p.m. UTC | #3 
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-July/574385.html

On 7/2/21 1:21 PM, Martin Sebor wrote:
> To avoid a class of false negatives for sanitized code
> -Wuninitialized recognizes the ASAN_MARK internal function
> doesn't modify its argument.  But the warning code doesn't do
> the same for any sanitizer built-ins even though they don't
> modify user-supplied arguments either.  This leaves another
> class of false negatives unresolved.
> 
> The attached fix enhances the warning logic to recognize all
> sanitizer built-ins as well and treat them as non-modifying.
> 
> Tested on x86_64-linux.
> 
> Martin
Martin Sebor July 20, 2021, 12:01 a.m. UTC | #4 
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-July/574385.html

On 7/12/21 12:06 PM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-July/574385.html
> 
> On 7/2/21 1:21 PM, Martin Sebor wrote:
>> To avoid a class of false negatives for sanitized code
>> -Wuninitialized recognizes the ASAN_MARK internal function
>> doesn't modify its argument.  But the warning code doesn't do
>> the same for any sanitizer built-ins even though they don't
>> modify user-supplied arguments either.  This leaves another
>> class of false negatives unresolved.
>>
>> The attached fix enhances the warning logic to recognize all
>> sanitizer built-ins as well and treat them as non-modifying.
>>
>> Tested on x86_64-linux.
>>
>> Martin
>
Jeff Law July 20, 2021, 10:48 a.m. UTC | #5 
On 7/19/2021 6:01 PM, Martin Sebor via Gcc-patches wrote:
> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-July/574385.html
>
> On 7/12/21 12:06 PM, Martin Sebor wrote:
>> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-July/574385.html
>>
>> On 7/2/21 1:21 PM, Martin Sebor wrote:
>>> To avoid a class of false negatives for sanitized code
>>> -Wuninitialized recognizes the ASAN_MARK internal function
>>> doesn't modify its argument.  But the warning code doesn't do
>>> the same for any sanitizer built-ins even though they don't
>>> modify user-supplied arguments either.  This leaves another
>>> class of false negatives unresolved.
>>>
>>> The attached fix enhances the warning logic to recognize all
>>> sanitizer built-ins as well and treat them as non-modifying.
>>>
>>> Tested on x86_64-linux.
OK after fixing the "pointets" -> "pointers" typo.

Jeff
Martin Sebor July 20, 2021, 7:12 p.m. UTC | #6 
On 7/20/21 4:48 AM, Jeff Law wrote:
> 
> 
> On 7/19/2021 6:01 PM, Martin Sebor via Gcc-patches wrote:
>> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-July/574385.html
>>
>> On 7/12/21 12:06 PM, Martin Sebor wrote:
>>> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-July/574385.html
>>>
>>> On 7/2/21 1:21 PM, Martin Sebor wrote:
>>>> To avoid a class of false negatives for sanitized code
>>>> -Wuninitialized recognizes the ASAN_MARK internal function
>>>> doesn't modify its argument.  But the warning code doesn't do
>>>> the same for any sanitizer built-ins even though they don't
>>>> modify user-supplied arguments either.  This leaves another
>>>> class of false negatives unresolved.
>>>>
>>>> The attached fix enhances the warning logic to recognize all
>>>> sanitizer built-ins as well and treat them as non-modifying.
>>>>
>>>> Tested on x86_64-linux.
> OK after fixing the "pointets" -> "pointers" typo.

Done and pushed in r12-2420.

Martin
diff mbox series

Patch

PR middle-end/101300 - -fsanitize=undefined suppresses -Wuninitialized for a VLA read at -O0

gcc/ChangeLog:

	* tree-ssa-uninit.c (check_defs): Handle UBSAN built-ins.

gcc/testsuite/ChangeLog:

	* gcc.dg/uninit-pr101300.c: New test.

diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
index 99442d7f975..dfcb7aba7c1 100644
--- a/gcc/tree-ssa-uninit.c
+++ b/gcc/tree-ssa-uninit.c
@@ -228,9 +228,26 @@  check_defs (ao_ref *ref, tree vdef, void *data_)
   gimple *def_stmt = SSA_NAME_DEF_STMT (vdef);
 
   /* The ASAN_MARK intrinsic doesn't modify the variable.  */
-  if (is_gimple_call (def_stmt)
-      && gimple_call_internal_p (def_stmt, IFN_ASAN_MARK))
-    return false;
+  if (is_gimple_call (def_stmt))
+    {
+      if (gimple_call_internal_p (def_stmt)
+         && gimple_call_internal_fn (def_stmt) == IFN_ASAN_MARK)
+       return false;
+
+      if (tree fndecl = gimple_call_fndecl (def_stmt))
+       {
+         /* Some sanitizer calls pass integer arguments to built-ins
+            that expect pointets. Avoid using gimple_call_builtin_p()
+            which fails for such calls.  */
+         if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
+           {
+             built_in_function fncode = DECL_FUNCTION_CODE (fndecl);
+             if (fncode > BEGIN_SANITIZER_BUILTINS
+                 && fncode < END_SANITIZER_BUILTINS)
+               return false;
+           }
+       }
+    }
 
   /* End of VLA scope is not a kill.  */
   if (gimple_call_builtin_p (def_stmt, BUILT_IN_STACK_RESTORE))
diff --git a/gcc/testsuite/gcc.dg/uninit-pr101300.c b/gcc/testsuite/gcc.dg/uninit-pr101300.c
new file mode 100644
index 00000000000..4392e8bae4f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/uninit-pr101300.c
@@ -0,0 +1,53 @@ 
+/* PR middle-end/101300 - -fsanitize=undefined suppresses -Wuninitialized
+   for a VLA read at -O0
+   { dg-do compile }
+   { dg-options "-O0 -Wall -fsanitize=undefined" } */
+
+int warn_vla_rd0 (int n)
+{
+  char a[n];
+  return a[0];      // { dg-warning "\\\[-Wuninitialized]" }
+}
+
+int warn_vla_rd1 (int n)
+{
+  char a[n];
+  return a[1];      // { dg-warning "\\\[-Wuninitialized]" }
+}
+
+int warn_vla_rdi (int n, int i)
+{
+  char a[n];
+  return a[i];      // { dg-warning "\\\[-Wuninitialized]" }
+}
+
+
+int warn_vla_wr0_rd2_1_0 (int n)
+{
+  char a[n];
+  a[0] = __LINE__;
+  int x = a[2];     // { dg-warning "\\\[-Wuninitialized]" }
+  int y = a[1];     // { dg-warning "\\\[-Wuninitialized]" }
+  int z = a[0];
+  return x + y + z;
+}
+
+int warn_vla_wr1_rd2_1_0 (int n)
+{
+  char a[n];
+  a[1] = __LINE__;
+  int x = a[2];     // { dg-warning "\\\[-Wuninitialized]" }
+  int y = a[1];
+  int z = a[0];     // { dg-warning "\\\[-Wuninitialized]" }
+  return x + y + z;
+}
+
+int warn_vla_wr2_rd2_1_0 (int n)
+{
+  char a[n];
+  a[2] = __LINE__;
+  int x = a[2];
+  int y = a[1];     // { dg-warning "\\\[-Wuninitialized]" }
+  int z = a[0];     // { dg-warning "\\\[-Wuninitialized]" }
+  return x + y + z;
+}