[PATCH v3 0/2] MR8433: msvcp90: Fix a couple of problems with vector.
* * * The test mentioned in the first commit: https://testbot.winehq.org/JobDetails.pl?Key=158969&f201=exe32.report#k201 Hopefully doing this test is not breaking any rules... -- v3: msvcp90: Fix calculation of segment addresses in vector. https://gitlab.winehq.org/wine/wine/-/merge_requests/8433
From: Yuxuan Shui <yshui(a)codeweavers.com> Before this commit, this->allocator appears to be given number of bytes as allocation size in msvcp90/details.c. But in msvcp120/tests/msvcp120.c, the provided allocator concurrent_vector_int_alloc, this size is multiplied by sizeof(int) again. A small change to log the size from concurrent_vector_int_alloc shows our size is exactly 4x native. I think that's enough evidence that the size here is meant to be No. elements. --- dlls/msvcp90/details.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/msvcp90/details.c b/dlls/msvcp90/details.c index ffac610400f..d51a4c716f1 100644 --- a/dlls/msvcp90/details.c +++ b/dlls/msvcp90/details.c @@ -497,12 +497,12 @@ static void concurrent_vector_alloc_segment(_Concurrent_vector_base_v4 *this, __TRY { if(seg == 0) - this->segment[seg] = this->allocator(this, element_size * (1 << this->first_block)); + this->segment[seg] = this->allocator(this, 1 << this->first_block); else if(seg < this->first_block) this->segment[seg] = (BYTE**)this->segment[0] + element_size * (1 << seg); else - this->segment[seg] = this->allocator(this, element_size * (1 << seg)); + this->segment[seg] = this->allocator(this, 1 << seg); } __EXCEPT_ALL { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8433
From: Yuxuan Shui <yshui(a)codeweavers.com> Casting this->segment[seg] to BYTE** cause the offset to be multiplied by the size of the pointer, which would be much larger than what's allocated. --- dlls/msvcp90/details.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/dlls/msvcp90/details.c b/dlls/msvcp90/details.c index d51a4c716f1..7dd7e23e384 100644 --- a/dlls/msvcp90/details.c +++ b/dlls/msvcp90/details.c @@ -499,8 +499,7 @@ static void concurrent_vector_alloc_segment(_Concurrent_vector_base_v4 *this, if(seg == 0) this->segment[seg] = this->allocator(this, 1 << this->first_block); else if(seg < this->first_block) - this->segment[seg] = (BYTE**)this->segment[0] - + element_size * (1 << seg); + this->segment[seg] = (BYTE *)this->segment[0] + element_size * (1 << seg); else this->segment[seg] = this->allocator(this, 1 << seg); } @@ -728,7 +727,7 @@ void __thiscall _Concurrent_vector_base_v4__Internal_assign( if(this->early_size > v_size) { if((i ? 1 << i : 2) - remain_element > 0) - clear((BYTE**)this->segment[i] + element_size * remain_element, + clear((BYTE*)this->segment[i] + element_size * remain_element, (i ? 1 << i : 2) - remain_element); if(i < seg_no) { @@ -740,8 +739,8 @@ void __thiscall _Concurrent_vector_base_v4__Internal_assign( else if(this->early_size < v_size) { if((i ? 1 << i : 2) - remain_element > 0) - copy((BYTE**)this->segment[i] + element_size * remain_element, - (BYTE**)v->segment[i] + element_size * remain_element, + copy((BYTE*)this->segment[i] + element_size * remain_element, + (BYTE*)v->segment[i] + element_size * remain_element, (i ? 1 << i : 2) - remain_element); if(i < v_seg_no) { @@ -777,7 +776,7 @@ size_t __thiscall _Concurrent_vector_base_v4__Internal_grow_by( last_seg_no = _vector_base_v4__Segment_index_of(size + count - 1); remain_size = min(size + count, 1 << (seg_no + 1)) - size; if(remain_size > 0) - copy(((BYTE**)this->segment[seg_no] + element_size * (size - ((1 << seg_no) & ~1))), v, + copy(((BYTE*)this->segment[seg_no] + element_size * (size - ((1 << seg_no) & ~1))), v, remain_size); if(seg_no != last_seg_no) { @@ -810,7 +809,7 @@ size_t __thiscall _Concurrent_vector_base_v4__Internal_grow_to_at_least_with_res last_seg_no = _vector_base_v4__Segment_index_of(count - 1); remain_size = min(count, 1 << (seg_no + 1)) - size; if(remain_size > 0) - copy(((BYTE**)this->segment[seg_no] + element_size * (size - ((1 << seg_no) & ~1))), v, + copy(((BYTE*)this->segment[seg_no] + element_size * (size - ((1 << seg_no) & ~1))), v, remain_size); if(seg_no != last_seg_no) { @@ -876,7 +875,7 @@ void __thiscall _Concurrent_vector_base_v4__Internal_resize( clear(this->segment[seg_no], 1 << seg_no); clear_element = (1 << (end_seg_no + 1)) - resize; if(clear_element > 0) - clear((BYTE**)this->segment[end_seg_no] + element_size * (resize - ((1 << end_seg_no) & ~1)), + clear((BYTE*)this->segment[end_seg_no] + element_size * (resize - ((1 << end_seg_no) & ~1)), clear_element); this->early_size = resize; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8433
i think on 32-bit, the extra `*sizeof(int)` and the extra `*sizeof(pointer)` kind of cancel out (for this particular test), just that each vector element now takes 4x the space. on 64-bit this is definitely out of bounds. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8433#note_108020
This merge request was approved by Piotr Caban. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8433
participants (3)
-
Piotr Caban (＠piotr) -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)