[PATCH v3 0/2] MR8433: msvcp90: Fix a couple of problems with vector.

[Yuxuan Shui (＠yshui)]

* * *
The test mentioned in the first commit: https://testbot.winehq.org/JobDetails.pl?Key=158969&f201=exe32.report#k2...

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

[PATCH v3 1/2] msvcp90: Fix vector_base_v4 allocation sizes.

[Yuxuan Shui]

From: Yuxuan Shui

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
             {

[PATCH v3 2/2] msvcp90: Fix calculation of segment addresses in vector.

[Yuxuan Shui]

From: Yuxuan Shui

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

[Yuxuan Shui (＠yshui)]

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.

[PATCH v3 0/2] MR8433: msvcp90: Fix a couple of problems with vector. - approved

[Piotr Caban (＠piotr)]

This merge request was approved by Piotr Caban.