* [PATCH v2 1/1] libstdc++: Set _M_string_length before calling _M_dispose() [PR109703]
2023-05-03 2:17 ` [PATCH v2 0/1] Set _M_string_length before calling _M_dispose() Kefu Chai
@ 2023-05-03 2:17 ` Kefu Chai
2023-05-03 12:22 ` [PATCH v2 0/1] Set _M_string_length before calling _M_dispose() Jonathan Wakely
1 sibling, 0 replies; 4+ messages in thread
From: Kefu Chai @ 2023-05-03 2:17 UTC (permalink / raw)
To: libstdc++; +Cc: gcc-patches, Kefu Chai, Kefu Chai, Jonathan Wakely
This patch always sets _M_string_length in the constructor specialized
for range of input_iterator, for the cases like istringstream.
We copy from source range to the local buffer, and then reallocate to
a larger one if necessary. When disposing the old buffer, the old buffer
could be provisioned by the local buffer or an allocated buffer.
_M_is_local() is used to tell if the buffer is the local one or not. In
addition to comparing the buffer address with the local buffer, this
function also performs the sanity checking if _M_string_length is greater
than _S_local_capacity, if the check fails __builtin_unreachable() is
called. But we failed to set _M_string_length in this constructor is
specialized for std::input_iterator. So, if UBSan is enabled when
compiling the source, there are chances that the uninitialized data in
_M_string_length is greater than _S_local_capacity, and the application
aborts a runtime error or exception emitted by the UBSan.
In this change, to avoid the false alarm, _M_string_length is
initialized to zero before doing anything else, so that _M_is_local()
doesn't see an uninitialized value.
This issue only surfaces when constructing a string with a range of
input_iterator, and the uninitialized _M_string_length is greater than
_S_local_capacity, i.e., 15.
libstdc++-v3/ChangeLog:
PR libstdc++/109703
* include/bits/basic_string.h (basic_string(Iter, Iter, Alloc)):
Initialize _M_string_length.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Co-authored-by: Jonathan Wakely <jwakely@redhat.com>
---
libstdc++-v3/include/bits/basic_string.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h
index 8247ee6bdc6..b16b2898b62 100644
--- a/libstdc++-v3/include/bits/basic_string.h
+++ b/libstdc++-v3/include/bits/basic_string.h
@@ -760,7 +760,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
_GLIBCXX20_CONSTEXPR
basic_string(_InputIterator __beg, _InputIterator __end,
const _Alloc& __a = _Alloc())
- : _M_dataplus(_M_local_data(), __a)
+ : _M_dataplus(_M_local_data(), __a), _M_string_length(0)
{
#if __cplusplus >= 201103L
_M_construct(__beg, __end, std::__iterator_category(__beg));
--
2.40.1
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2 0/1] Set _M_string_length before calling _M_dispose()
2023-05-03 2:17 ` [PATCH v2 0/1] Set _M_string_length before calling _M_dispose() Kefu Chai
2023-05-03 2:17 ` [PATCH v2 1/1] libstdc++: Set _M_string_length before calling _M_dispose() [PR109703] Kefu Chai
@ 2023-05-03 12:22 ` Jonathan Wakely
2023-05-03 12:30 ` kefu chai
1 sibling, 1 reply; 4+ messages in thread
From: Jonathan Wakely @ 2023-05-03 12:22 UTC (permalink / raw)
To: Kefu Chai; +Cc: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]
On Wed, 3 May 2023 at 03:17, Kefu Chai via Libstdc++ <libstdc++@gcc.gnu.org>
wrote:
> Hi Jonathan,
>
> Thank you for your review and suggestion. The change looks great!
> Assigning a value with an immediate zero is indeed much faster.
>
> in v2:
>
> * revised the commit message a little bit, I found it a little bit
> difficult to parse when re-reading it.
> * associated the commit with PR/libstdc++/109703. as I just filed
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109706, which turns out
> to be a dup of https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109703
>
> The rest of the v2 patch is identical to the one attached in your reply.
>
> Would you please taking another look?
>
Thanks. I've pushed it to trunk as
cbf6c7a1d16490a1e63e9a5ce00e9a5c44c4c2f2 and will backport it too.
I altered the commit msg again, because "input_iterator" is a C++20
concept, and here we're just talking about types meeting the old
C[[17InputIterator requirements, not types modelling the concept. I also
added references to the commit and PR that added the
__builtin_unreachable().
> Kefu Chai (1):
> libstdc++: Set _M_string_length before calling _M_dispose() [PR109703]
>
> libstdc++-v3/include/bits/basic_string.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --
> 2.40.1
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread