From 0f375ceafa7a2bf45350647dcbe3136260b1ad6c Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Thu, 21 Sep 2023 18:30:48 -0700 Subject: [PATCH 1/3] ASAN found _two_ bugs in `_Copy_vbool`! * `_IsSingleBlockDest` wasn't accounting for the fact that `_DestEnd` is past-the-end * we sometimes don't realize we're done and read past the end of the input --- stl/inc/vector | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/stl/inc/vector b/stl/inc/vector index 95ef86aae3..18b5c88ba6 100644 --- a/stl/inc/vector +++ b/stl/inc/vector @@ -3748,7 +3748,7 @@ _CONSTEXPR20 _OutIt _Copy_vbool(_VbIt _First, _VbIt _Last, _OutIt _Dest) { const auto _LastDestMask = static_cast<_Vbase>(-1) << _DestEnd._Myoff; const bool _IsSingleBlockSource = _VbFirst == _VbLast; - const bool _IsSingleBlockDest = _VbDest == _DestEnd._Myptr; + const bool _IsSingleBlockDest = _VbDest == _DestEnd._Myptr - (_DestEnd._Myoff ? 0 : 1); const bool _IsRightShift = _Dest._Myoff < _First._Myoff; if (_IsSingleBlockSource) { // We already excluded _First == _Last, so here _Last._Myoff > 0 and the shift is safe @@ -3757,7 +3757,7 @@ _CONSTEXPR20 _OutIt _Copy_vbool(_VbIt _First, _VbIt _Last, _OutIt _Dest) { const auto _SourceVal = _IsRightShift ? (*_VbFirst & _SourceMask) >> _SourceShift // : (*_VbFirst & _SourceMask) << _SourceShift; if (_IsSingleBlockDest) { - const auto _DestMask = _FirstDestMask | _LastDestMask; + const auto _DestMask = _FirstDestMask | (_DestEnd._Myoff ? _LastDestMask : 0); *_VbDest = (*_VbDest & _DestMask) | _SourceVal; } else { *_VbDest = (*_VbDest & _FirstDestMask) | _SourceVal; @@ -3774,7 +3774,7 @@ _CONSTEXPR20 _OutIt _Copy_vbool(_VbIt _First, _VbIt _Last, _OutIt _Dest) { const auto _SourceVal = _IsRightShift ? (*_VbFirst & _FirstSourceMask) >> _SourceShift // : (*_VbFirst & _FirstSourceMask) << _SourceShift; - const auto _DestMask = _FirstDestMask | _LastDestMask; + const auto _DestMask = _FirstDestMask | (_DestEnd._Myoff ? _LastDestMask : 0); if (_Last._Myoff != 0) { const auto _LastShift = _DestEnd._Myoff - _Last._Myoff; const auto _LastSourceVal = (*_VbLast & _LastSourceMask) << _LastShift; @@ -3842,20 +3842,23 @@ _CONSTEXPR20 _OutIt _Copy_vbool(_VbIt _First, _VbIt _Last, _OutIt _Dest) { *_VbDest = (*_VbDest & _DestMask) | _SourceVal; } - const auto _CarryVal = (*_VbFirst & _LastSourceMask) << _CarryShift; - if (_Last._Myoff >= _SourceShift) { - *_VbDest = (*_VbDest & _CarryMask) | _CarryVal; - - // We have more bits remaining than the final block has left - if (_Last._Myoff != _SourceShift) { - ++_VbDest; - const auto _SourceVal = (*_VbFirst & _LastSourceMask) >> _SourceShift; - *_VbDest = (*_VbDest & _LastDestMask) | _SourceVal; + if (_Last._Myoff != 0) { + if (_Last._Myoff >= _SourceShift) { + const auto _CarryVal = (*_VbFirst & _LastSourceMask) << _CarryShift; + *_VbDest = (*_VbDest & _CarryMask) | _CarryVal; + + // We have more bits remaining than the final block has left + if (_Last._Myoff != _SourceShift) { + ++_VbDest; + const auto _SourceVal = (*_VbFirst & _LastSourceMask) >> _SourceShift; + *_VbDest = (*_VbDest & _LastDestMask) | _SourceVal; + } + } else { + // There are not enough bits to fill the final block so we need to mask both ends + const auto _CarryVal = (*_VbFirst & _LastSourceMask) << _CarryShift; + const auto _FinalMask = _CarryMask | _LastDestMask; + *_VbDest = (*_VbDest & _FinalMask) | _CarryVal; } - } else if (_Last._Myoff != 0) { - // There are not enough bits to fill the final block so we need to mask both ends - const auto _FinalMask = _CarryMask | _LastDestMask; - *_VbDest = (*_VbDest & _FinalMask) | _CarryVal; } } else { const auto _SourceShift = _Dest._Myoff - _First._Myoff; From 06302e7cf161004100985c7b5c4987d3940c4083 Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Thu, 21 Sep 2023 18:34:29 -0700 Subject: [PATCH 2/3] Ooops: forget to push this back up --- stl/inc/vector | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/stl/inc/vector b/stl/inc/vector index 18b5c88ba6..53c2d570f3 100644 --- a/stl/inc/vector +++ b/stl/inc/vector @@ -3843,9 +3843,9 @@ _CONSTEXPR20 _OutIt _Copy_vbool(_VbIt _First, _VbIt _Last, _OutIt _Dest) { } if (_Last._Myoff != 0) { + const auto _CarryVal = (*_VbFirst & _LastSourceMask) << _CarryShift; if (_Last._Myoff >= _SourceShift) { - const auto _CarryVal = (*_VbFirst & _LastSourceMask) << _CarryShift; - *_VbDest = (*_VbDest & _CarryMask) | _CarryVal; + *_VbDest = (*_VbDest & _CarryMask) | _CarryVal; // We have more bits remaining than the final block has left if (_Last._Myoff != _SourceShift) { @@ -3855,7 +3855,6 @@ _CONSTEXPR20 _OutIt _Copy_vbool(_VbIt _First, _VbIt _Last, _OutIt _Dest) { } } else { // There are not enough bits to fill the final block so we need to mask both ends - const auto _CarryVal = (*_VbFirst & _LastSourceMask) << _CarryShift; const auto _FinalMask = _CarryMask | _LastDestMask; *_VbDest = (*_VbDest & _FinalMask) | _CarryVal; } From 1f08c28112caa0e77a0d97d29e5c8ed054495df5 Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Thu, 21 Sep 2023 19:06:15 -0700 Subject: [PATCH 3/3] Stephan's review comments --- stl/inc/vector | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/stl/inc/vector b/stl/inc/vector index 53c2d570f3..3f86a5258d 100644 --- a/stl/inc/vector +++ b/stl/inc/vector @@ -3748,7 +3748,7 @@ _CONSTEXPR20 _OutIt _Copy_vbool(_VbIt _First, _VbIt _Last, _OutIt _Dest) { const auto _LastDestMask = static_cast<_Vbase>(-1) << _DestEnd._Myoff; const bool _IsSingleBlockSource = _VbFirst == _VbLast; - const bool _IsSingleBlockDest = _VbDest == _DestEnd._Myptr - (_DestEnd._Myoff ? 0 : 1); + const bool _IsSingleBlockDest = _VbDest == _DestEnd._Myptr - (_DestEnd._Myoff == 0 ? 1 : 0); const bool _IsRightShift = _Dest._Myoff < _First._Myoff; if (_IsSingleBlockSource) { // We already excluded _First == _Last, so here _Last._Myoff > 0 and the shift is safe @@ -3757,7 +3757,7 @@ _CONSTEXPR20 _OutIt _Copy_vbool(_VbIt _First, _VbIt _Last, _OutIt _Dest) { const auto _SourceVal = _IsRightShift ? (*_VbFirst & _SourceMask) >> _SourceShift // : (*_VbFirst & _SourceMask) << _SourceShift; if (_IsSingleBlockDest) { - const auto _DestMask = _FirstDestMask | (_DestEnd._Myoff ? _LastDestMask : 0); + const auto _DestMask = _FirstDestMask | (_DestEnd._Myoff == 0 ? 0 : _LastDestMask); *_VbDest = (*_VbDest & _DestMask) | _SourceVal; } else { *_VbDest = (*_VbDest & _FirstDestMask) | _SourceVal; @@ -3774,7 +3774,7 @@ _CONSTEXPR20 _OutIt _Copy_vbool(_VbIt _First, _VbIt _Last, _OutIt _Dest) { const auto _SourceVal = _IsRightShift ? (*_VbFirst & _FirstSourceMask) >> _SourceShift // : (*_VbFirst & _FirstSourceMask) << _SourceShift; - const auto _DestMask = _FirstDestMask | (_DestEnd._Myoff ? _LastDestMask : 0); + const auto _DestMask = _FirstDestMask | (_DestEnd._Myoff == 0 ? 0 : _LastDestMask); if (_Last._Myoff != 0) { const auto _LastShift = _DestEnd._Myoff - _Last._Myoff; const auto _LastSourceVal = (*_VbLast & _LastSourceMask) << _LastShift;