[Bug sanitizer/108894] New: -fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size()

[Bug sanitizer/108894] New: -fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size()

[kees at outflux dot net]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108894

            Bug ID: 108894
           Summary: -fsanitize=bounds missing bounds provided by
                    __builtin_dynamic_object_size()
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: sanitizer
          Assignee: unassigned at gcc dot gnu.org
          Reporter: kees at outflux dot net
                CC: dodji at gcc dot gnu.org, dvyukov at gcc dot gnu.org,
                    jakub at gcc dot gnu.org, kcc at gcc dot gnu.org, marxin at gcc dot gnu.org
  Target Milestone: ---

Created attachment 54508
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54508&action=edit
PoC showing lack of __bdos support in -fsanitize=bounds

While -fsanitize-bounds is able to perform run-time bounds checking on
fixed-size arrays (i.e. when __builtin_object_size(x, 1) does not return
SIZE_MAX), it does not perform bounds checking when
__builtin_dynamic_object_size(x, 1) is available.

For example, the attached program produces _no_ bounds-checker warnings:

$ gcc -Wall -O2 -fstrict-flex-arrays=3 -fsanitize=bounds -fstrict-flex-arrays=3
-o bounds bounds.c
$ ./bounds

p->array has a fixed size: 64 (16 elements of size 4)
p->array[0] assignment: 255 (should be ok)
p->array[16] assignment: 255 (should be failure)

p->array has a dynamic size: 64 (16 elements of size 4)
p->array[0] assignment: 255 (should be ok)
p->array[16] assignment: 255 (should be failure)

p->array has unknowable size
p->array[0] assignment: 255 (should be ok)
p->array[16] assignment: 255 (should be failure)


Note that the first failure for a fixed size array implies that
-fsanitize=bounds has also not been wired up to -fstrict-flex-arrays=3, so it
is ignoring all trailing arrays.

[Bug sanitizer/108894] -fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size()

[kees at outflux dot net]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108894

--- Comment #1 from Kees Cook <kees at outflux dot net> ---
The matching Clang bug is: https://github.com/llvm/llvm-project/issues/60926

[Bug sanitizer/108894] -fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size()

[kees at outflux dot net]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108894

Kees Cook <kees at outflux dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #54508|0                           |1
        is obsolete|                            |

--- Comment #2 from Kees Cook <kees at outflux dot net> ---
Created attachment 54509
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54509&action=edit
PoC showing lack of __bdos support in -fsanitize=bounds

(Updated PoC so the "unknown" case correctly said "ignored" instead of
"ok"/"failure" -- the unknown size case must continue to be ignored.)

[Bug sanitizer/108894] -fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size()

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108894

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement

[Bug sanitizer/108894] -fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size()

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108894

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mpolacek at gcc dot gnu.org
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-02-22
             Status|UNCONFIRMED                 |NEW

[Bug sanitizer/108894] -fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size()

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108894

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
-fstrict-flex-array= option doesn't affect the sanitization, if you want strict
sanitization of bounds, you should use -fsanitize=bounds-strict rather than
-fsanitize=bounds.
Furthermore, it is misunderstanding on what either of those sanitizers does,
they check the array index against the array domain.  In the case of flexible
array member, that size is unlimited, not some constant or variable (that would
be just in case of a VLA).
If you want sanitization against object size, there is -fsanitize=object-size
for it.

[Bug sanitizer/108894] -fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size()

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108894

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
so not a bug?

[Bug sanitizer/108894] -fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size()

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108894

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
For the uses of __bdos for -fsanitize=bounds* IMHO certainly, we really
shouldn't duplicate what another sanitizer does there.
As for whether -fstrict-flex-arrays= should or shouldn't affect
-fsanitize=bounds, making it gradually equivalent to -fsanitize=bounds-strict,
that is a question, perhaps if -fstrict-flex-arrays= is considered as changing
the exactly applicable language standard, with that option it might change what
is and is not undefined behavior.
Siddhesh/Qing, what do you think?
Note, clang doesn't support -fsanitize=bounds-strict it seems, on the other
side
-fsanitize=bounds there is a union of -fsanitize=array-bounds and
-fsanitize=local-bounds where the latter according to documentation isn't
included in -fsanitize=undefined, whatever it means.  In gcc
-fsanitize=undefined similarly includes -fsanitize=bounds but doesn't
-fsanitize=bounds-strict.

[Bug sanitizer/108894] -fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size()

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108894

--- Comment #6 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Ah, I thought we wanted to use __bdos for -fsanitize=object-size but that has
been done already:

commit 28896b38fabce818e59266b0063a46b3bc1b700f
Author: Siddhesh Poyarekar <siddhesh@gotplt.org>
Date:   Tue May 10 12:51:42 2022 +0530

    middle-end/70090: Dynamic sizes for -fsanitize=object-size


Sorry for confirming the bug before checking.

[Bug sanitizer/108894] -fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size()

[qinzhao at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108894

qinzhao at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |qinzhao at gcc dot gnu.org

--- Comment #7 from qinzhao at gcc dot gnu.org ---
(In reply to Jakub Jelinek from comment #5)
> As for whether -fstrict-flex-arrays= should or shouldn't affect
> -fsanitize=bounds, making it gradually equivalent to
> -fsanitize=bounds-strict, that is a question, perhaps if
> -fstrict-flex-arrays= is considered as changing the exactly applicable
> language standard, with that option it might change what is and is not
> undefined behavior.
> Siddhesh/Qing, what do you think?
from the doc:
"
-fsanitize=bounds
This option enables instrumentation of array bounds. Various out of bounds
accesses are detected. Flexible array members, flexible array member-like
arrays, and initializers of variables with static storage are not instrumented.

-fsanitize=bounds-strict
This option enables strict instrumentation of array bounds. Most out of bounds
accesses are detected, including flexible array members and flexible array
member-like arrays. Initializers of variables with static storage are not
instrumented.
"

the situation is very similar to the previous:

-Warray-bounds
-Warray-bounds=2

Per our previous discussion on  -Warray-bounds and -Warray-bounds=2 and
-fstrict-flex-arrays=N, I think it's very reasonable to handle the
-fsanitize=bounds and -fsanitize=bounds-strict + -fstrict-flex-arrays=N
similarly, i.e:

1. let -fstrict-flex-arrays=N to control the behavior of -fsanitize=bounds;
2. -fsanitize=bounds-strict actually is an alias of -fsanitize=bounds
-fstrict-flex-arrays=2,  i.e, it treats [], [0] as flexible array members, but
treat [1], [4], as regular arrays.

[Bug sanitizer/108894] -fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size()

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108894

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to qinzhao from comment #7)
> 1. let -fstrict-flex-arrays=N to control the behavior of -fsanitize=bounds;

I'm ok with that.

> 2. -fsanitize=bounds-strict actually is an alias of -fsanitize=bounds
> -fstrict-flex-arrays=2,  i.e, it treats [], [0] as flexible array members,
> but treat [1], [4], as regular arrays.

Well, -fsanitize=bounds-strict certainly shouldn't imply
-fstrict-flex-arrays=2,
it should just treat [1] and [4] (but I think it does even [0] right now) as
regular arrays for the purposes of the sanitization.

[Bug sanitizer/108894] -fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size()

[qinzhao at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108894

--- Comment #9 from qinzhao at gcc dot gnu.org ---
(In reply to Jakub Jelinek from comment #8)
> Well, -fsanitize=bounds-strict certainly shouldn't imply
> -fstrict-flex-arrays=2,
> it should just treat [1] and [4] (but I think it does even [0] right now) as
> regular arrays for the purposes of the sanitization.

with a small example I just tested, with -fsanitize=bounds-strict, I can see,
it treats:
   [], [0] as flexible array members;
but
   [1], [4] as regular arrays

This is the same level as -fstrict-flex-arrays=2. 
should we just keep its default behavior like this, or let it more strictly as
-fstrict-flex-arrays=3?

[Bug sanitizer/108894] -fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size()

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108894

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to qinzhao from comment #9)
> (In reply to Jakub Jelinek from comment #8)
> > Well, -fsanitize=bounds-strict certainly shouldn't imply
> > -fstrict-flex-arrays=2,
> > it should just treat [1] and [4] (but I think it does even [0] right now) as
> > regular arrays for the purposes of the sanitization.
> 
> with a small example I just tested, with -fsanitize=bounds-strict, I can
> see, it treats:
>    [], [0] as flexible array members;
> but
>    [1], [4] as regular arrays
> 
> This is the same level as -fstrict-flex-arrays=2. 
> should we just keep its default behavior like this, or let it more strictly
> as -fstrict-flex-arrays=3?

I'd keep its current behavior, perhaps except for -fsanitize=bounds-strict
-fstrict-flex-arrays{,=3} so that -fsanitize=bounds -fstrict-flex-arrays{,=3}
wouldn't be more strict than the former.

[Bug sanitizer/108894] -fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size()

[qinzhao at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108894

--- Comment #11 from qinzhao at gcc dot gnu.org ---
(In reply to Jakub Jelinek from comment #10)

> I'd keep its current behavior, perhaps except for -fsanitize=bounds-strict
> -fstrict-flex-arrays{,=3} so that -fsanitize=bounds
> -fstrict-flex-arrays{,=3} wouldn't be more strict than the former.
Agreed.

[Bug sanitizer/108894] -fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size()

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108894

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 54547
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54547&action=edit
gcc13-pr108894.patch

Untested fix.

[Bug sanitizer/108894] -fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size()

[qinzhao at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108894

--- Comment #13 from qinzhao at gcc dot gnu.org ---
(In reply to Jakub Jelinek from comment #12)
> Created attachment 54547 [details]
> gcc13-pr108894.patch
> 
> Untested fix.

several comments on the patch:

1. should the documentation of -fsanitize=bounds and -fsanitize=strict-bounds
be updated to reflect the interaction with -fstrict-flex-arrays=N?
2. there are several routines in c-decl.cc:
 static bool  flexible_array_member_type_p (const_tree type);
 static bool  one_element_array_type_p (const_tree type);
 static bool  zero_length_array_type_p (const_tree type);

can they be generalized  as well to be used in the routine 
"ubsan_instrument_bounds" to check for [], [0], or [1]? (in the patch lines
from 405 to 442). 
3. could you add comments for lines (I guess they are for [0])?

370       if (!bound)
371         bound = fold_build2 (PLUS_EXPR, TREE_TYPE (bound), bound,   371    
    {
372                              build_int_cst (TREE_TYPE (bound), 1)); 372    
      if (!c_dialect_cxx ()
373               && COMPLETE_TYPE_P (type)
374               && integer_zerop (TYPE_SIZE (type)))
375             bound = build_int_cst (TREE_TYPE (TYPE_MIN_VALUE (domain)),
-1);
376           else
377             return NULL_TREE;
378         }

[Bug sanitizer/108894] -fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size()

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108894

--- Comment #14 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:c7728805a7107444683290cd629d13f089130a0d

commit r13-6375-gc7728805a7107444683290cd629d13f089130a0d
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Feb 28 11:38:46 2023 +0100

    ubsan: Honor -fstrict-flex-arrays= in -fsanitize=bounds [PR108894]

    While this isn't really a regression, the -fstrict-flex-arrays*
    option is new in GCC 13 and so I think we should make -fsanitize=bounds
    play with it well from the beginning.

    The current behavior is that -fsanitize=bounds considers all trailing
    arrays as flexible member-like arrays and both -fsanitize=bounds and
    -fsanitize=bounds-strict because of a bug don't even instrument
    [0] arrays at all, not as trailing nor when followed by other members.

    I think -fstrict-flex-arrays* options can be considered as language
    mode changing options, by default flexible member-like arrays are
    handled like flexible arrays, but that option can change the set of
    the arrays which are treated like that.  So, -fsanitize=bounds should
    change with that on what is considered acceptable and what isn't.
    While -fsanitize=bounds-strict should reject them all always to
    continue previous behavior.

    The following patch implements that.  To support [0] array instrumentation,
    I had to change the meaning of the bounds argument to .UBSAN_BOUNDS,
    previously it was the TYPE_MAX_VALUE of the domain unless ignore_off_by_one
    (used for taking address of the array element rather than accessing it;
    in that case 1 is added to the bound argument) and the later lowered checks
    were if (index > bound) report_failure ().
    The problem with that is that for [0] arrays where (at least for C++)
    the max value is all ones, for accesses that condition will be never true;
    for addresses of elements it was working (in C++) correctly before.
    This patch changes it to add 1 + ignore_off_by_one, so -1 becomes 0 or
    1 for &array_ref and changing the lowering to be if (index >= bound)
    report_failure ().  Furthermore, as C represents the [0] arrays with
    NULL TYPE_MAX_VALUE, I treated those like the C++ ones.

    2023-02-28  Jakub Jelinek  <jakub@redhat.com>

            PR sanitizer/108894
    gcc/
            * ubsan.cc (ubsan_expand_bounds_ifn): Emit index >= bound
            comparison rather than index > bound.
            * gimple-fold.cc (gimple_fold_call): Use tree_int_cst_lt
            rather than tree_int_cst_le for IFN_UBSAN_BOUND comparison.
            * doc/invoke.texi (-fsanitize=bounds): Document that whether
            flexible array member-like arrays are instrumented or not depends
            on -fstrict-flex-arrays* options of strict_flex_array attributes.
            (-fsanitize=bounds-strict): Document that flexible array members
            are not instrumented.
    gcc/c-family/
            * c-common.h (c_strict_flex_array_level_of): Declare.
            * c-common.cc (c_strict_flex_array_level_of): New function,
            moved and renamed from c-decl.cc's strict_flex_array_level_of.
            * c-ubsan.cc (ubsan_instrument_bounds): Fix comment typo.  For
            C check c_strict_flex_array_level_of whether a trailing array
            should be treated as flexible member like.  Handle C [0] arrays.
            Add 1 + index_off_by_one rather than index_off_by_one to bounds
            and use tree_int_cst_lt rather than tree_int_cst_le for idx vs.
            bounds comparison.
    gcc/c/
            * c-decl.cc (strict_flex_array_level_of): Move to c-common.cc
            and rename to c_strict_flex_array_level_of.
            (is_flexible_array_member_p): Adjust caller.
    gcc/testsuite/
            * gcc.dg/ubsan/bounds-4.c: New test.
            * gcc.dg/ubsan/bounds-4a.c: New test.
            * gcc.dg/ubsan/bounds-4b.c: New test.
            * gcc.dg/ubsan/bounds-4c.c: New test.
            * gcc.dg/ubsan/bounds-4d.c: New test.
            * g++.dg/ubsan/bounds-1.C: New test.

[Bug sanitizer/108894] -fsanitize=bounds missing bounds provided by __builtin_dynamic_object_size()

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108894

--- Comment #15 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:f72c8918416f67aad907752f1892c19eda12eecb

commit r13-6389-gf72c8918416f67aad907752f1892c19eda12eecb
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Mar 1 10:49:38 2023 +0100

    ubsan: Add another testcase for [0] array in the middle of struct
[PR108894]

    I think it is useful to cover also this, rather than just arrays at the
    flexible array member positions.

    2023-03-01  Jakub Jelinek  <jakub@redhat.com>

            PR sanitizer/108894
            * c-c++-common/ubsan/bounds-16.c: New test.