diff mbox series

libstdc++: avoid uninitialized read in basic_string constructor

Message ID 20231102195652.9965-1-ben.sherman@chicagotrading.com
State Not Applicable
Headers
Series libstdc++: avoid uninitialized read in basic_string constructor |

Checks

Context Check Description
snail/gcc-patch-check fail Git am fail log

Commit Message

Ben Sherman Nov. 2, 2023, 7:56 p.m. UTC 
  Tested on x86_64-pc-linux-gnu, please let me know if there's anything
else needed. I haven't contributed before and don't have write access, so
apologies if I've missed anything.

-- >8 --

The basic_string input iterator constructor incrementally reads data and
allocates the internal buffer as-needed. When _M_dispose() is called, there
is a check for whether the local buffer is being used - if it is, there is
an additional check guarding __builtin_unreachable() for the value of
_M_string_length. The constructor does not initialize _M_string_length
until all data has been read, so the first re-allocation out of the local
buffer will have an uninitialized read.

This updates the basic_string input iterator constructor to properly set
_M_string_length as data is being read.  It additionally introduces a new
_M_assign_terminator() function to assign the null-terminator based on the
currently-stored _M_string_length.

libstdc++-v3/ChangeLog:

        * include/bits/basic_string.h (_M_assign_terminator()): New
          function.
          (_M_set_length()): Use _M_assign_terminator().
        * include/bits/basic_string.tcc (_M_construct(InIter, InIter,
          input_iterator_tag)): Set length incrementally, use
          _M_assign_terminator().

--
2.21.0







This electronic mail message and any attached files contain information intended for the exclusive use of the individual or entity to whom it is addressed and may contain information that is proprietary, confidential and/or exempt from disclosure under applicable law. If you are not the intended recipient, you are hereby notified that any viewing, copying, disclosure or distribution of this information may be subject to legal restriction or sanction. Please notify the sender, by electronic mail or telephone, of any unintended recipients and delete the original message without making any copies.

Comments

Jonathan Wakely Nov. 2, 2023, 8:53 p.m. UTC | #1 
On Thu, 2 Nov 2023 at 19:58, Ben Sherman <ben.sherman@chicagotrading.com> wrote:
>
> Tested on x86_64-pc-linux-gnu, please let me know if there's anything
> else needed. I haven't contributed before and don't have write access, so
> apologies if I've missed anything.

This was https://gcc.gnu.org/PR109703 (and several duplicates) and
should already be fixed in all affected branches. Where are you seeing
this?

> The basic_string input iterator constructor incrementally reads data and
> allocates the internal buffer as-needed. When _M_dispose() is called, there
> is a check for whether the local buffer is being used - if it is, there is
> an additional check guarding __builtin_unreachable() for the value of
> _M_string_length. The constructor does not initialize _M_string_length
> until all data has been read, so the first re-allocation out of the local
> buffer will have an uninitialized read.
>
> This updates the basic_string input iterator constructor to properly set
> _M_string_length as data is being read.  It additionally introduces a new
> _M_assign_terminator() function to assign the null-terminator based on the
> currently-stored _M_string_length.

Adding new member functions to std::string requires exporting them
from the shared library, which requires bumping the shared library
version, which is an ABI change that isn't suitable for backporting to
release branches. But it doesn't matter if we don't need to make this
change (and I don't think we do need to).


> libstdc++-v3/ChangeLog:
>
>         * include/bits/basic_string.h (_M_assign_terminator()): New
>           function.
>           (_M_set_length()): Use _M_assign_terminator().
>         * include/bits/basic_string.tcc (_M_construct(InIter, InIter,
>           input_iterator_tag)): Set length incrementally, use
>           _M_assign_terminator().
>
> diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h
> index 0fa32afeb..ba02d8f0f 100644
> --- a/libstdc++-v3/include/bits/basic_string.h
> +++ b/libstdc++-v3/include/bits/basic_string.h
> @@ -258,12 +258,17 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
>        _M_capacity(size_type __capacity)
>        { _M_allocated_capacity = __capacity; }
>
> +      _GLIBCXX20_CONSTEXPR
> +      void
> +      _M_assign_terminator()
> +      { traits_type::assign(_M_data()[_M_string_length], _CharT()); }
> +
>        _GLIBCXX20_CONSTEXPR
>        void
>        _M_set_length(size_type __n)
>        {
>         _M_length(__n);
> -       traits_type::assign(_M_data()[__n], _CharT());
> +       _M_assign_terminator();
>        }
>
>        _GLIBCXX20_CONSTEXPR
> diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc
> index f0a44e5e8..84366a44a 100644
> --- a/libstdc++-v3/include/bits/basic_string.tcc
> +++ b/libstdc++-v3/include/bits/basic_string.tcc
> @@ -182,6 +182,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>             ++__beg;
>           }
>
> +       _M_length(__len);
> +
>         struct _Guard
>         {
>           _GLIBCXX20_CONSTEXPR
> @@ -206,12 +208,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>                 _M_capacity(__capacity);
>               }
>             traits_type::assign(_M_data()[__len++], *__beg);
> +           _M_length(__len);
>             ++__beg;
>           }
>
>         __guard._M_guarded = 0;
>
> -       _M_set_length(__len);
> +       _M_assign_terminator();
>        }
>
>    template<typename _CharT, typename _Traits, typename _Alloc>
> --
> 2.21.0
>
>
>
>
>
>
>
> This electronic mail message and any attached files contain information intended for the exclusive use of the individual or entity to whom it is addressed and may contain information that is proprietary, confidential and/or exempt from disclosure under applicable law. If you are not the intended recipient, you are hereby notified that any viewing, copying, disclosure or distribution of this information may be subject to legal restriction or sanction. Please notify the sender, by electronic mail or telephone, of any unintended recipients and delete the original message without making any copies.
>
Ben Sherman Nov. 3, 2023, 1:51 p.m. UTC | #2 
> This was https://gcc.gnu.org/PR109703 (and several duplicates) and
> should already be fixed in all affected branches. Where are you seeing
> this?

I saw this on 13.1.0 and could not find the bug report or fix for it, so I
didn't realize it had already been fixed.  Sorry about that.






This electronic mail message and any attached files contain information intended for the exclusive use of the individual or entity to whom it is addressed and may contain information that is proprietary, confidential and/or exempt from disclosure under applicable law. If you are not the intended recipient, you are hereby notified that any viewing, copying, disclosure or distribution of this information may be subject to legal restriction or sanction. Please notify the sender, by electronic mail or telephone, of any unintended recipients and delete the original message without making any copies.
Sam James Nov. 3, 2023, 1:53 p.m. UTC | #3 
Jonathan Wakely <jwakely@redhat.com> writes:

> On Thu, 2 Nov 2023 at 19:58, Ben Sherman <ben.sherman@chicagotrading.com> wrote:
>>
>> Tested on x86_64-pc-linux-gnu, please let me know if there's anything
>> else needed. I haven't contributed before and don't have write access, so
>> apologies if I've missed anything.
>
> This was https://gcc.gnu.org/PR109703 (and several duplicates) and
> should already be fixed in all affected branches. Where are you seeing
> this?

It would help to know where they got their GCC from too. Ben?
Jonathan Wakely Nov. 3, 2023, 2:27 p.m. UTC | #4 
On Fri, 3 Nov 2023 at 13:51, Ben Sherman <Ben.Sherman@chicagotrading.com> wrote:
>
> > This was https://gcc.gnu.org/PR109703 (and several duplicates) and
> > should already be fixed in all affected branches. Where are you seeing
> > this?
>
> I saw this on 13.1.0 and could not find the bug report or fix for it, so I
> didn't realize it had already been fixed.  Sorry about that.

No problem, sorry you spent your time analyzing the bug and writing the fix.

By default the bugzilla search results don't include closed bugs. That
means that if you search for a bug that was closed recently but is
still present in the release you're using, it won't show up.
diff mbox series

Patch

diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h
index 0fa32afeb..ba02d8f0f 100644
--- a/libstdc++-v3/include/bits/basic_string.h
+++ b/libstdc++-v3/include/bits/basic_string.h
@@ -258,12 +258,17 @@  _GLIBCXX_BEGIN_NAMESPACE_CXX11
       _M_capacity(size_type __capacity)
       { _M_allocated_capacity = __capacity; }

+      _GLIBCXX20_CONSTEXPR
+      void
+      _M_assign_terminator()
+      { traits_type::assign(_M_data()[_M_string_length], _CharT()); }
+
       _GLIBCXX20_CONSTEXPR
       void
       _M_set_length(size_type __n)
       {
        _M_length(__n);
-       traits_type::assign(_M_data()[__n], _CharT());
+       _M_assign_terminator();
       }

       _GLIBCXX20_CONSTEXPR
diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc
index f0a44e5e8..84366a44a 100644
--- a/libstdc++-v3/include/bits/basic_string.tcc
+++ b/libstdc++-v3/include/bits/basic_string.tcc
@@ -182,6 +182,8 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
            ++__beg;
          }

+       _M_length(__len);
+
        struct _Guard
        {
          _GLIBCXX20_CONSTEXPR
@@ -206,12 +208,13 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
                _M_capacity(__capacity);
              }
            traits_type::assign(_M_data()[__len++], *__beg);
+           _M_length(__len);
            ++__beg;
          }

        __guard._M_guarded = 0;

-       _M_set_length(__len);
+       _M_assign_terminator();
       }

   template<typename _CharT, typename _Traits, typename _Alloc>