[Bug libstdc++/112314] New: Missing index assertions in basic_string_view

[Bug libstdc++/112314] New: Missing index assertions in basic_string_view

[jdapena at igalia dot com]

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

            Bug ID: 112314
           Summary: Missing index assertions in basic_string_view
           Product: gcc
           Version: 12.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jdapena at igalia dot com
  Target Milestone: ---

While testing Chromium base unit tests with _GLIBCXX_ASSERTIONS=1, that include
checks for base::StringPiece, that wraps nowadays std::string_view, two unit
tests are failing because they are expected to assert and they don't.

One failing tests is:
  {
    StringPiece piece;
    ASSERT_DEATH_IF_SUPPORTED(piece.remove_suffix(1), "");
  }

In this case, std::basic_string_view::remove_suffix should check if the index
provided is valid.

Another failing test is:

  int length = -1;
  ASSERT_DEATH_IF_SUPPORTED({ StringPiece piece("hello", length); }, "");

In this case, std::basic_string_view::basic_string_view(const _CharT* __str,
size_type __len) is not validating that __len is a valid index for __str.

[Bug libstdc++/112314] Missing index assertions in basic_string_view

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-10-31
           Severity|normal                      |enhancement

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
For the first problem:

--- a/libstdc++-v3/include/std/string_view
+++ b/libstdc++-v3/include/std/string_view
@@ -301,7 +301,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION

       constexpr void
       remove_suffix(size_type __n) noexcept
-      { this->_M_len -= __n; }
+      {
+       __glibcxx_assert(this->_M_len >= __n);
+       this->_M_len -= __n;
+      }

       constexpr void
       swap(basic_string_view& __sv) noexcept


For the second one, I did start work on a patch that attempts to verify that
the provided length is not more than __builtin_object_size. I can't remember
why I stopped working on that. There's no way to make that check 100% reliable,
because __builtin_object_size depends on optimizations and whether the compiler
can see enough data.

[Bug libstdc++/112314] Missing index assertions in basic_string_view

[jdapena at igalia dot com]

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

--- Comment #2 from Jose Dapena Paz <jdapena at igalia dot com> ---
(In reply to Jonathan Wakely from comment #1)
> For the second one, I did start work on a patch that attempts to verify that
> the provided length is not more than __builtin_object_size. I can't remember
> why I stopped working on that. There's no way to make that check 100%
> reliable, because __builtin_object_size depends on optimizations and whether
> the compiler can see enough data.

I guess in this case a "best-effort" approach could be good enough? Providing
there are no false positives asserting for actually valid calls. In the case of
Chromium unit test it is passing a predefined const char[] so, at least making
sure those cases are validated would add an extra layer of protection.

In any case, the failing test is actually passing -1, my understanding is that
that one should always assert no matter what we are passing as const char*.

[Bug libstdc++/112314] Missing index assertions in basic_string_view

[cvs-commit at gcc dot gnu.org]

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

--- Comment #3 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:6afa984f47e16e8bd958646d7407b74e61041f5d

commit r14-5085-g6afa984f47e16e8bd958646d7407b74e61041f5d
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Nov 1 15:01:22 2023 +0000

    libstdc++: Add assertion to std::string_view::remove_suffix [PR112314]

    libstdc++-v3/ChangeLog:

            PR libstdc++/112314
            * include/std/string_view (string_view::remove_suffix): Add
            debug assertion.
            *
testsuite/21_strings/basic_string_view/modifiers/remove_prefix/debug.cc:
            New test.
            *
testsuite/21_strings/basic_string_view/modifiers/remove_suffix/debug.cc:
            New test.

[Bug libstdc++/112314] Missing index assertions in basic_string_view

[redi at gcc dot gnu.org]

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

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jose Dapena Paz from comment #2)
> In any case, the failing test is actually passing -1, my understanding is
> that that one should always assert no matter what we are passing as const
> char*.

Yes but I'm not going to add a check for -1 just to make some unit test pass.
In real code (size_t)-2 or (size_t)(-some other value) is at least as likely.

__glibcxx_assert(not in chromium tests) doesn't seem useful.

[Bug libstdc++/112314] Missing index assertions in basic_string_view

[jdapena at igalia dot com]

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

--- Comment #5 from Jose Dapena Paz <jdapena at igalia dot com> ---
(In reply to Jonathan Wakely from comment #4)
> (In reply to Jose Dapena Paz from comment #2)
> > In any case, the failing test is actually passing -1, my understanding is
> > that that one should always assert no matter what we are passing as const
> > char*.
> 
> Yes but I'm not going to add a check for -1 just to make some unit test
> pass. In real code (size_t)-2 or (size_t)(-some other value) is at least as
> likely.
> 
> __glibcxx_assert(not in chromium tests) doesn't seem useful.

Oh, I just checked the LLVM implementation and it is... interesting:
https://github.com/llvm/llvm-project/blob/main/libcxx/include/string_view#L311C1-L318C1

It checks two conditions:
- Length should be 0 OR pointer should not be null.
- The length is less than the possible pointer difference (checked with
numeric_limits).

[Bug libstdc++/112314] Missing index assertions in basic_string_view

[redi at gcc dot gnu.org]

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

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Created attachment 56494
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56494&action=edit
Check [ptr,end) and [ptr,ptr+n) ranges with _GLIBCXX_ASSERTIONS

With this change we could add:

__glibcxx_requires_string_len(__str, __len);

to the string_view constructor.

We could include checks for __len == 0 || __str != nullptr and PTRDIFF_MAX too.

[Bug libstdc++/112314] Missing index assertions in basic_string_view

[redi at gcc dot gnu.org]

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

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jose Dapena Paz from comment #5)
> - The length is less than the possible pointer difference (checked with
> numeric_limits).

That seems too lenient to me, because for wchar_t, char16_t and char32_t the
maximum length that can actually exist in the program will be less than
PTRDIFF_MAX.

Our std::vector::max_size() uses this:

        // std::distance(begin(), end()) cannot be greater than PTRDIFF_MAX,
        // and realistically we can't store more than PTRDIFF_MAX/sizeof(T)
        // (even if std::allocator_traits::max_size says we can).
        const size_t __diffmax
          = __gnu_cxx::__numeric_traits<ptrdiff_t>::__max / sizeof(_Tp);

[Bug libstdc++/112314] Missing index assertions in basic_string_view

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by Jonathan Wakely
<redi@gcc.gnu.org>:

https://gcc.gnu.org/g:66d0abdf0ade07228eba4dedcd1a9da09960ef53

commit r13-8014-g66d0abdf0ade07228eba4dedcd1a9da09960ef53
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Nov 1 15:01:22 2023 +0000

    libstdc++: Add assertion to std::string_view::remove_suffix [PR112314]

    libstdc++-v3/ChangeLog:

            PR libstdc++/112314
            * include/std/string_view (string_view::remove_suffix): Add
            debug assertion.
            *
testsuite/21_strings/basic_string_view/modifiers/remove_prefix/debug.cc:
            New test.
            *
testsuite/21_strings/basic_string_view/modifiers/remove_suffix/debug.cc:
            New test.

    (cherry picked from commit 6afa984f47e16e8bd958646d7407b74e61041f5d)

[Bug libstdc++/112314] Missing index assertions in basic_string_view

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Jonathan Wakely
<redi@gcc.gnu.org>:

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

commit r12-10029-gc5c57aa7e63da2e769f4fda6e2ec9e8bd0c7b344
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Nov 1 15:01:22 2023 +0000

    libstdc++: Add assertion to std::string_view::remove_suffix [PR112314]

    libstdc++-v3/ChangeLog:

            PR libstdc++/112314
            * include/std/string_view (string_view::remove_suffix): Add
            debug assertion.
            *
testsuite/21_strings/basic_string_view/modifiers/remove_prefix/debug.cc:
            New test.
            *
testsuite/21_strings/basic_string_view/modifiers/remove_suffix/debug.cc:
            New test.

    (cherry picked from commit 6afa984f47e16e8bd958646d7407b74e61041f5d)

[Bug libstdc++/112314] Missing index assertions in basic_string_view

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Jonathan Wakely
<redi@gcc.gnu.org>:

https://gcc.gnu.org/g:4f74f6c7aa0820943ba0777dc41d69a969576e18

commit r11-11127-g4f74f6c7aa0820943ba0777dc41d69a969576e18
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Nov 1 15:01:22 2023 +0000

    libstdc++: Add assertion to std::string_view::remove_suffix [PR112314]

    libstdc++-v3/ChangeLog:

            PR libstdc++/112314
            * include/std/string_view (string_view::remove_suffix): Add
            debug assertion.
            *
testsuite/21_strings/basic_string_view/modifiers/remove_prefix/debug.cc:
            New test.
            *
testsuite/21_strings/basic_string_view/modifiers/remove_suffix/debug.cc:
            New test.

    (cherry picked from commit 6afa984f47e16e8bd958646d7407b74e61041f5d)