Re: [PATCH] libstdc++: Check [ptr, end) and [ptr, ptr+n) ranges with _GLIBCXX_ASSERTIONS

[Jonathan Wakely <jwakely@redhat.com>]

On Fri, 15 Oct 2021 at 06:19, François Dumont wrote:
>
> On 14/10/21 7:43 pm, Jonathan Wakely wrote:
> > On Thu, 14 Oct 2021 at 18:11, François Dumont <frs.dumont@gmail.com> wrote:
> >> Hi
> >>
> >>       On a related subject I am waiting for some feedback on:
> >>
> >> https://gcc.gnu.org/pipermail/libstdc++/2021-August/053005.html
> > I'm concerned that this adds too much overhead for the
> > _GLIBCXX_ASSERTIONS case. It adds function calls which are not
> > necessarily inlined, and which perform arithmetic and comparisons on
> > the arguments. That has a runtime cost which is non-zero.
>
> I thought that limiting the checks to __valid_range would be fine for
> _GLIBCXX_ASSERTIONS. If you do not want any overhead you just don't
> define it.

Then you get no checks at all. The point of _GLIBCXX_ASSERTIONS is to
get *some* checking, without too much overhead. If you are willing to
accept the overhead we already have _GLIBCXX_DEBUG for that.

We could consider a second level of _GLIBCXX_ASSERTIONS=2 that turns
on extra checks, but we need to be careful about adding any
non-trivial checks to _GLIBCXX_ASSERTIONS=1 (which is what is used
today in major linux distributions, to build every C++ program and
library in the OS).


>
> >
> > The patches I sent in this thread have zero runtime cost, because they
> > use the compiler built-in which compiles away to nothing if the sizes
> > aren't known.
> I'll try to find out if it can help for the test case on std::copy which
> I was adding with my proposal.
> >
> >> On 11/10/21 6:49 pm, Jonathan Wakely wrote:
> >>> This enables lightweight checks for the __glibcxx_requires_valid_range
> >>> and __glibcxx_requires_string_len macros  when _GLIBCXX_ASSERTIONS is
> >>> defined.  By using __builtin_object_size we can check whether the end of
> >>> the range is part of the same object as the start of the range, and
> >>> detect problems like in PR 89927.
> >>>
> >>> libstdc++-v3/ChangeLog:
> >>>
> >>>        * include/debug/debug.h (__valid_range_p, __valid_range_n): New
> >>>        inline functions using __builtin_object_size to check ranges
> >>>        delimited by pointers.
> >>>        [_GLIBCXX_ASSERTIONS] (__glibcxx_requires_valid_range): Use
> >>>        __valid_range_p.
> >>>        [_GLIBCXX_ASSERTIONS] (__glibcxx_requires_string_len): Use
> >>>        __valid_range_n.
> >>>
> >>>
> >>> The first patch allows us to detect bugs like string("foo", "bar"),
> >>> like in PR 89927. Debug mode cannot currently detect this. The new
> >>> check uses the compiler built-in to detect when the two arguments are
> >>> not part of the same object. This assumes we're optimizing and the
> >>> compiler knows the values of the pointers. If it doesn't, then the
> >>> function just returns true and should inline to nothing.
> >> I see, it does not detect that input pointers are unrelated but as they
> >> are the computed size is >= __sz.
> >>
> >> Isn't it UB to compare unrelated pointers ?
> > Yes, and my patch doesn't compare any pointers, does it?
> >
> +      __UINTPTR_TYPE__ __f = (__UINTPTR_TYPE__)__first;
> +      __UINTPTR_TYPE__ __l = (__UINTPTR_TYPE__)__last;
> +      if (const std::size_t __sz = __builtin_object_size(__first, 3))
> +    return __f <= __l && (__l - __f) <= __sz;
>
> Isn't it a comparison ?

It's not comparing pointers, it's comparing integers. To avoid the
unspecified behaviour of comparing unrelated pointers.

>
> But maybe this is what the previous cast is for, I never understood it.
>
> Note that those cast could be moved within the if branch, even if I
> guess that the compiler does it.

At -O1 the casts are zero cost, they don't generate any code. At -O0,
you have so much overhead for every line of code that this doesn't
make much difference! But yes, we could move them into the if
statement.