[PATCH resend 1/2] comctl32/listbox: Fix InitStorage heap extension

[Gabriel Ivăncescu]

Only increase the item array if we actually have to. Previously, sending
for example just 1 to nb_items repeatedly would always increase the array
by LB_ARRAY_GRANULARITY, even if there was plenty of space.

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---

Assume LB_ARRAY_GRANULARITY is a power of 2 and note it in the comments. This
simplifies the code (and it's already the case anyway).

 dlls/comctl32/listbox.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
index 2137ef8..5c171ab 100644
--- a/dlls/comctl32/listbox.c
+++ b/dlls/comctl32/listbox.c
@@ -41,7 +41,7 @@
 
 WINE_DEFAULT_DEBUG_CHANNEL(listbox);
 
-/* Items array granularity */
+/* Items array granularity; must be a power of 2 */
 #define LB_ARRAY_GRANULARITY 16
 
 /* Scrolling timeout in ms */
@@ -673,16 +673,18 @@ static LRESULT LISTBOX_InitStorage( LB_DESCR *descr, INT nb_items )
 {
     LB_ITEMDATA *item;
 
-    nb_items += LB_ARRAY_GRANULARITY - 1;
-    nb_items -= (nb_items % LB_ARRAY_GRANULARITY);
     if (descr->items) {
-        nb_items += HeapSize( GetProcessHeap(), 0, descr->items ) / sizeof(*item);
-	item = HeapReAlloc( GetProcessHeap(), 0, descr->items,
-                              nb_items * sizeof(LB_ITEMDATA));
+        nb_items += descr->nb_items;
+        if (nb_items > HeapSize(GetProcessHeap(), 0, descr->items) / sizeof(*item))
+        {
+            UINT n = (nb_items + LB_ARRAY_GRANULARITY - 1) & ~(LB_ARRAY_GRANULARITY - 1);
+            item = HeapReAlloc(GetProcessHeap(), 0, descr->items, n * sizeof(*item));
+        }
+        else return LB_OKAY;
     }
     else {
-	item = HeapAlloc( GetProcessHeap(), 0,
-                              nb_items * sizeof(LB_ITEMDATA));
+        UINT n = (nb_items + LB_ARRAY_GRANULARITY - 1) & ~(LB_ARRAY_GRANULARITY - 1);
+        item = HeapAlloc(GetProcessHeap(), 0, n * sizeof(*item));
     }
 
     if (!item)
-- 
2.19.1

[PATCH resend 2/2] user32/listbox: Fix InitStorage heap extension

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/user32/listbox.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/dlls/user32/listbox.c b/dlls/user32/listbox.c
index c8bd148..af325d2 100644
--- a/dlls/user32/listbox.c
+++ b/dlls/user32/listbox.c
@@ -36,7 +36,7 @@
 
 WINE_DEFAULT_DEBUG_CHANNEL(listbox);
 
-/* Items array granularity */
+/* Items array granularity; must be a power of 2 */
 #define LB_ARRAY_GRANULARITY 16
 
 /* Scrolling timeout in ms */
@@ -698,16 +698,18 @@ static LRESULT LISTBOX_InitStorage( LB_DESCR *descr, INT nb_items )
 {
     LB_ITEMDATA *item;
 
-    nb_items += LB_ARRAY_GRANULARITY - 1;
-    nb_items -= (nb_items % LB_ARRAY_GRANULARITY);
     if (descr->items) {
-        nb_items += HeapSize( GetProcessHeap(), 0, descr->items ) / sizeof(*item);
-	item = HeapReAlloc( GetProcessHeap(), 0, descr->items,
-                              nb_items * sizeof(LB_ITEMDATA));
+        nb_items += descr->nb_items;
+        if (nb_items > HeapSize(GetProcessHeap(), 0, descr->items) / sizeof(*item))
+        {
+            UINT n = (nb_items + LB_ARRAY_GRANULARITY - 1) & ~(LB_ARRAY_GRANULARITY - 1);
+            item = HeapReAlloc(GetProcessHeap(), 0, descr->items, n * sizeof(*item));
+        }
+        else return LB_OKAY;
     }
     else {
-	item = HeapAlloc( GetProcessHeap(), 0,
-                              nb_items * sizeof(LB_ITEMDATA));
+        UINT n = (nb_items + LB_ARRAY_GRANULARITY - 1) & ~(LB_ARRAY_GRANULARITY - 1);
+        item = HeapAlloc(GetProcessHeap(), 0, n * sizeof(*item));
     }
 
     if (!item)
-- 
2.19.1

[Nikolay Sivov]

I think it's better to introduce 'capacity' or 'nb_allocated' field, and 
use that instead of HeapSize().

Then you can have new array length aligned in one place, with 
heap_realloc() that will handle initial state of items == NULL.

[Gabriel Ivăncescu]

On Mon, Nov 5, 2018 at 2:14 PM Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
> I think it's better to introduce 'capacity' or 'nb_allocated' field, and
> use that instead of HeapSize().
>
> Then you can have new array length aligned in one place, with
> heap_realloc() that will handle initial state of items == NULL.

I feel as if it's out of the scope of this (small) patch. That would
require quite some changes since a lot of the other code also uses
HeapSize().

Also, I'm probably misunderstanding something, but HeapSize is aligned
already, though. It's just the current code which rounds up the size
requested *without* looking at the available space. So it will always
grow, even if there's plenty of free space.