Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ASAN found _two_ bugs in _Copy_vbool! #4045

Merged
merged 3 commits into from
Sep 22, 2023
Merged

ASAN found _two_ bugs in _Copy_vbool! #4045

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0f375ce
ASAN found _two_ bugs in `_Copy_vbool`!
CaseyCarter Sep 22, 2023
06302e7
Ooops: forget to push this back up
CaseyCarter Sep 22, 2023
1f08c28
Stephan's review comments
CaseyCarter Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 18 additions & 16 deletions stl/inc/vector
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 : 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
Expand All @@ -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 == 0 ? 0 : _LastDestMask);
*_VbDest = (*_VbDest & _DestMask) | _SourceVal;
} else {
*_VbDest = (*_VbDest & _FirstDestMask) | _SourceVal;
Expand All @@ -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 == 0 ? 0 : _LastDestMask);
if (_Last._Myoff != 0) {
const auto _LastShift = _DestEnd._Myoff - _Last._Myoff;
const auto _LastSourceVal = (*_VbLast & _LastSourceMask) << _LastShift;
Expand Down Expand Up @@ -3842,20 +3842,22 @@ _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) {
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;
}
} else {
// 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;
Comment on lines +3845 to +3859
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if that change has been necessary,

We only get into the first branch with _Last._Myoff >= _SourceShift so if _Last._Myoff == _SourceShift == 0 then we would only do *_VbDest = (*_VbDest & _CarryMask) | _CarryVal; and be done with it. because the second condition would ne been satisfied.

That said it is hell and I am really not sure if I need to add some more documentation of the various conditions

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When _Last._Myoff == 0, _VbFirst is past-the-end of the input sequence when the for loop completes. The unguarded read on red line 3845 was out of bounds in this case, and triggered ASan:

=================================================================
==11404==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x11c5b1dee9e0 at pc 0x7ff7389645a3 bp 0x00b1fa95b7e0 sp 0x00b1fa95b7e0
READ of size 4 at 0x11c5b1dee9e0 thread T0
    #0 0x7ff7389645a2 in std::_Copy_vbool<class std::_Vb_const_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>, class std::_Vb_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>>(class std::_Vb_const_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>, class std::_Vb_const_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>, class std::_Vb_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>) C:\STL\out\x64\out\inc\vector:3846
    #1 0x7ff7389627df in std::_Copy_unchecked<class std::_Vb_const_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>, class std::_Vb_const_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>, class std::_Vb_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>>(class std::_Vb_const_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>, class std::_Vb_const_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>, class std::_Vb_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>) C:\STL\out\x64\out\inc\xutility:4590
    #2 0x7ff7389749ea in std::copy<class std::_Vb_const_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>, class std::_Vb_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>>(class std::_Vb_const_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>, class std::_Vb_const_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>, class std::_Vb_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>) C:\STL\out\x64\out\inc\xutility:4625
    #3 0x7ff738958bec in randomized_test_copy(class std::mersenne_twister_engine<unsigned __int64, 64, 312, 156, 31, -5403634167711393303, 29, 6148914691236517205, 17, 8202884508482404352, 37, -2270628950310912, 43, 6364136223846793005> &) C:\STL\tests\std\tests\GH_000625_vector_bool_optimization\test.cpp:1208
    #4 0x7ff73895c4f8 in main C:\STL\tests\std\tests\GH_000625_vector_bool_optimization\test.cpp:1317
    #5 0x7ff7389a4c58 in invoke_main D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:78
    #6 0x7ff7389a4bad in __scrt_common_main_seh D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
    #7 0x7ff7389a4a6d in __scrt_common_main D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:330
    #8 0x7ff7389a4ccd in mainCRTStartup D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_main.cpp:16
    #9 0x7ffbf04d257c  (C:\Windows\System32\KERNEL32.DLL+0x18001257c)
    #10 0x7ffbf124aa77  (C:\Windows\SYSTEM32\ntdll.dll+0x18005aa77)

0x11c5b1dee9e0 is located 0 bytes to the right of 16-byte region [0x11c5b1dee9d0,0x11c5b1dee9e0)
allocated by thread T0 here:
    #0 0x7ff7389a38d5 in operator new(unsigned __int64) D:\a\_work\1\s\src\vctools\asan\llvm\compiler-rt\lib\asan\asan_win_new_scalar_thunk.cpp:40
    #1 0x7ff73898b7d2 in std::_Default_allocate_traits::_Allocate(unsigned __int64) C:\STL\out\x64\out\inc\xmemory:90
    #2 0x7ff73895f79d in std::_Allocate<16, struct std::_Default_allocate_traits, 0>(unsigned __int64) C:\STL\out\x64\out\inc\xmemory:248
    #3 0x7ff73899a2af in std::allocator<unsigned int>::allocate(unsigned __int64) C:\STL\out\x64\out\inc\xmemory:982
    #4 0x7ff73895f925 in std::_Allocate_at_least_helper<class std::allocator<unsigned int>>(class std::allocator<unsigned int> &, unsigned __int64 &) C:\STL\out\x64\out\inc\xmemory:2211
    #5 0x7ff73898d375 in std::vector<unsigned int, class std::allocator<unsigned int>>::_Buy_raw(unsigned __int64) C:\STL\out\x64\out\inc\vector:1997
    #6 0x7ff73898c9bc in std::vector<unsigned int, class std::allocator<unsigned int>>::_Buy_nonzero(unsigned __int64) C:\STL\out\x64\out\inc\vector:2018
    #7 0x7ff7389608e2 in std::vector<unsigned int, class std::allocator<unsigned int>>::_Construct_n<unsigned int const &>(unsigned __int64, unsigned int const &) C:\STL\out\x64\out\inc\vector:2074
    #8 0x7ff738981099 in std::vector<unsigned int, class std::allocator<unsigned int>>::vector<unsigned int, class std::allocator<unsigned int>>(unsigned __int64, unsigned int const &, class std::allocator<unsigned int> const &) C:\STL\out\x64\out\inc\vector:623
    #9 0x7ff73897fe8d in std::_Vb_val<class std::allocator<bool>>::_Vb_val<class std::allocator<bool>>(unsigned __int64, bool const &, class std::allocator<bool> const &) C:\STL\out\x64\out\inc\vector:2801
    #10 0x7ff738981880 in std::vector<bool, class std::allocator<bool>>::vector<bool, class std::allocator<bool>>(unsigned __int64, class std::allocator<bool> const &) C:\STL\out\x64\out\inc\vector:2894
    #11 0x7ff738958482 in randomized_test_copy(class std::mersenne_twister_engine<unsigned __int64, 64, 312, 156, 31, -5403634167711393303, 29, 6148914691236517205, 17, 8202884508482404352, 37, -2270628950310912, 43, 6364136223846793005> &) C:\STL\tests\std\tests\GH_000625_vector_bool_optimization\test.cpp:1197
    #12 0x7ff73895c4f8 in main C:\STL\tests\std\tests\GH_000625_vector_bool_optimization\test.cpp:1317
    #13 0x7ff7389a4c58 in invoke_main D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:78
    #14 0x7ff7389a4bad in __scrt_common_main_seh D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
    #15 0x7ff7389a4a6d in __scrt_common_main D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:330
    #16 0x7ff7389a4ccd in mainCRTStartup D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_main.cpp:16
    #17 0x7ffbf04d257c  (C:\Windows\System32\KERNEL32.DLL+0x18001257c)
    #18 0x7ffbf124aa77  (C:\Windows\SYSTEM32\ntdll.dll+0x18005aa77)

SUMMARY: AddressSanitizer: heap-buffer-overflow C:\STL\out\x64\out\inc\vector:3846 in std::_Copy_vbool<class std::_Vb_const_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>, class std::_Vb_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>>(class std::_Vb_const_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>, class std::_Vb_const_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>, class std::_Vb_iterator<struct std::_Wrap_alloc<class std::allocator<unsigned int>>>)
Shadow bytes around the buggy address:
  0x03fa6813dce0: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
  0x03fa6813dcf0: fa fa fd fd fa fa fd fd fa fa fd fa fa fa fd fd
  0x03fa6813dd00: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
  0x03fa6813dd10: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
  0x03fa6813dd20: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
=>0x03fa6813dd30: fa fa fd fd fa fa 00 00 fa fa 00 00[fa]fa 00 00
  0x03fa6813dd40: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa 00 00
  0x03fa6813dd50: fa fa 00 00 fa fa 00 00 fa fa fa fa fa fa fa fa
  0x03fa6813dd60: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x03fa6813dd70: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x03fa6813dd80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==11404==ABORTING
--

}
} 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;
Expand Down