[PATCH 1/5] comctl32/listbox: Check for out of bounds index using an unsigned index

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu [email protected]
---
 dlls/comctl32/listbox.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
index cb645b4..71f1c05 100644
--- a/dlls/comctl32/listbox.c
+++ b/dlls/comctl32/listbox.c
@@ -489,7 +489,7 @@ static INT LISTBOX_GetItemFromPoint( const LB_DESCR *descr, INT x, INT y )
  * Paint an item.
  */
 static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect,
-			       INT index, UINT action, BOOL ignoreFocus )
+			       UINT index, UINT action, BOOL ignoreFocus )
 {
     BOOL selected = FALSE, focused;
     LB_ITEMDATA *item = NULL;
@@ -508,7 +508,7 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect,
         RECT r;
         HRGN hrgn;

-	if (!item)
+	if (index >= descr->nb_items)
    {
        if (action == ODA_FOCUS)
    	DrawFocusRect( hdc, rect );

[PATCH 2/5] comctl32/listbox: Use a helper to expand the item array

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu [email protected]
---
 dlls/comctl32/listbox.c | 63 +++++++++++++++--------------------------
 1 file changed, 23 insertions(+), 40 deletions(-)

diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
index 71f1c05..498c549 100644
--- a/dlls/comctl32/listbox.c
+++ b/dlls/comctl32/listbox.c
@@ -36,12 +36,13 @@
 #include "wine/unicode.h"
 #include "wine/exception.h"
 #include "wine/debug.h"
+#include "wine/heap.h"

#include "comctl32.h"

WINE_DEFAULT_DEBUG_CHANNEL(listbox);

-/* Items array granularity */
+/* Items array granularity (must be power of 2) */
 #define LB_ARRAY_GRANULARITY 16

/* Scrolling timeout in ms */
@@ -126,6 +127,25 @@ static TIMER_DIRECTION LISTBOX_Timer = LB_TIMER_NONE;

static LRESULT LISTBOX_GetItemRect( const LB_DESCR *descr, INT index, RECT *rect );

+static BOOL resize_storage(LB_DESCR *descr, UINT items_size)
+{
+    LB_ITEMDATA *items;
+
+    if (items_size > descr->items_size)
+    {
+        items_size = (items_size + LB_ARRAY_GRANULARITY - 1) & ~(LB_ARRAY_GRANULARITY - 1);
+        items = heap_realloc(descr->items, items_size * sizeof(LB_ITEMDATA));
+        if (!items)
+        {
+            SEND_NOTIFICATION(descr, LBN_ERRSPACE);
+            return FALSE;
+        }
+        descr->items_size = items_size;
+        descr->items = items;
+    }
+    return TRUE;
+}
+
 static BOOL is_item_selected( const LB_DESCR *descr, UINT index )
 {
     return descr->items[index].selected;
@@ -684,27 +704,8 @@ static void LISTBOX_DrawFocusRect( LB_DESCR *descr, BOOL on )
  */
 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 += descr->items_size;
-	item = HeapReAlloc( GetProcessHeap(), 0, descr->items,
-                              nb_items * sizeof(LB_ITEMDATA));
-    }
-    else {
-	item = HeapAlloc( GetProcessHeap(), 0,
-                              nb_items * sizeof(LB_ITEMDATA));
-    }
-
-    if (!item)
-    {
-        SEND_NOTIFICATION( descr, LBN_ERRSPACE );
+    if (!resize_storage(descr, descr->nb_items + nb_items))
         return LB_ERRSPACE;
-    }
-    descr->items_size = nb_items;
-    descr->items = item;
     return LB_OKAY;
 }

@@ -1525,29 +1526,11 @@ static LRESULT LISTBOX_InsertItem( LB_DESCR *descr, INT index,
                                    LPWSTR str, ULONG_PTR data )
 {
     LB_ITEMDATA *item;
-    INT max_items;
     INT oldfocus = descr->focus_item;

if (index == -1) index = descr->nb_items;
     else if ((index < 0) || (index > descr->nb_items)) return LB_ERR;
-    if (descr->nb_items == descr->items_size)
-    {
-        /* We need to grow the array */
-        max_items = descr->items_size + LB_ARRAY_GRANULARITY;
-	if (descr->items)
-	    item = HeapReAlloc( GetProcessHeap(), 0, descr->items,
-                                  max_items * sizeof(LB_ITEMDATA) );
-	else
-	    item = HeapAlloc( GetProcessHeap(), 0,
-                                  max_items * sizeof(LB_ITEMDATA) );
-        if (!item)
-        {
-            SEND_NOTIFICATION( descr, LBN_ERRSPACE );
-            return LB_ERRSPACE;
-        }
-        descr->items_size = max_items;
-        descr->items = item;
-    }
+    if (!resize_storage(descr, descr->nb_items + 1)) return LB_ERR;

/* Insert the item structure */

[PATCH 3/5] comctl32/listbox: Return proper value for LB_INITSTORAGE

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu [email protected]
---
 dlls/comctl32/listbox.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
index 498c549..f1ecc52 100644
--- a/dlls/comctl32/listbox.c
+++ b/dlls/comctl32/listbox.c
@@ -706,7 +706,7 @@ static LRESULT LISTBOX_InitStorage( LB_DESCR *descr, INT nb_items )
 {
     if (!resize_storage(descr, descr->nb_items + nb_items))
         return LB_ERRSPACE;
-    return LB_OKAY;
+    return descr->items_size;
 }

[PATCH 4/5] comctl32/tests: Add tests for LB_INITSTORAGE

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu [email protected]
---
 dlls/comctl32/tests/listbox.c | 64 +++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/dlls/comctl32/tests/listbox.c b/dlls/comctl32/tests/listbox.c
index e789483..68e4073 100644
--- a/dlls/comctl32/tests/listbox.c
+++ b/dlls/comctl32/tests/listbox.c
@@ -1906,6 +1906,69 @@ static void test_GetListBoxInfo(void)
     DestroyWindow(parent);
 }

+static void test_init_storage( void )
+{
+    static const DWORD styles[] =
+    {
+        LBS_HASSTRINGS,
+        LBS_NODATA | LBS_OWNERDRAWFIXED,
+    };
+    HWND parent, listbox;
+    LONG ret, items_size;
+    int i, j;
+
+    parent = create_parent();
+    for (i = 0; i < ARRAY_SIZE(styles); i++)
+    {
+        listbox = CreateWindowA(WC_LISTBOXA, "TestList", styles[i] | WS_CHILD,
+                                0, 0, 100, 100, parent, (HMENU)ID_LISTBOX, NULL, 0);
+
+        items_size = SendMessageA(listbox, LB_INITSTORAGE, 100, 0);
+        ok(items_size >= 100, "expected at least 100, got %d\n", items_size);
+
+        ret = SendMessageA(listbox, LB_INITSTORAGE, 0, 0);
+        ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
+
+        /* it doesn't grow since the space was already reserved */
+        ret = SendMessageA(listbox, LB_INITSTORAGE, items_size, 0);
+        ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
+
+        /* it doesn't shrink the reserved space */
+        ret = SendMessageA(listbox, LB_INITSTORAGE, 42, 0);
+        ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
+
+        /* now populate almost all of it so it's not reserved anymore */
+        if (styles[i] & LBS_NODATA)
+        {
+            ret = SendMessageA(listbox, LB_SETCOUNT, items_size - 1, 0);
+            ok(ret == 0, "unexpected return value %d\n", ret);
+        }
+        else
+        {
+            for (j = 0; j < items_size - 1; j++)
+            {
+                ret = SendMessageA(listbox, LB_INSERTSTRING, -1, (LPARAM)"");
+                ok(ret == j, "expected %d, got %d\n", j, ret);
+            }
+        }
+
+        /* we still have one more reserved slot, so it doesn't grow yet */
+        ret = SendMessageA(listbox, LB_INITSTORAGE, 1, 0);
+        ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
+
+        /* fill the slot and check again, it should grow this time */
+        ret = SendMessageA(listbox, LB_INSERTSTRING, -1, (LPARAM)"");
+        ok(ret == items_size - 1, "expected %d, got %d\n", items_size - 1, ret);
+        ret = SendMessageA(listbox, LB_INITSTORAGE, 0, 0);
+        ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
+        ret = SendMessageA(listbox, LB_INITSTORAGE, 1, 0);
+        ok(ret > items_size, "expected it to grow past %d, got %d\n", items_size, ret);
+
+        DestroyWindow(listbox);
+    }
+    DestroyWindow(parent);
+}
+
 static void test_missing_lbuttonup(void)
 {
     HWND listbox, parent, capture;
@@ -2425,6 +2488,7 @@ START_TEST(listbox)
     test_listbox_LB_DIR();
     test_listbox_dlgdir();
     test_set_count();
+    test_init_storage();
     test_GetListBoxInfo();
     test_missing_lbuttonup();
     test_extents();

[PATCH 4/5] comctl32/tests: Add tests for LB_INITSTORAGE

[Huw Davies]

On Thu, Jan 31, 2019 at 05:23:22PM +0200, Gabriel Ivăncescu wrote:
> Signed-off-by: Gabriel Ivăncescu [email protected]
>
> dlls/comctl32/tests/listbox.c | 64 +++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>
> diff --git a/dlls/comctl32/tests/listbox.c b/dlls/comctl32/tests/listbox.c
> index e789483..68e4073 100644
> --- a/dlls/comctl32/tests/listbox.c
> +++ b/dlls/comctl32/tests/listbox.c
> @@ -1906,6 +1906,69 @@ static void test_GetListBoxInfo(void)
>      DestroyWindow(parent);
>  }
>
> +static void test_init_storage( void )
> +{
> - static const DWORD styles[] =
> - {
> -
>    LBS_HASSTRINGS,
> -
>    LBS_NODATA | LBS_OWNERDRAWFIXED,
> - };
> - HWND parent, listbox;
> - LONG ret, items_size;
> - int i, j;
> -
> - parent = create_parent();
> - for (i = 0; i < ARRAY_SIZE(styles); i++)
> - {
> -
>    listbox = CreateWindowA(WC_LISTBOXA, "TestList", styles[i] | WS_CHILD,
> -
>                            0, 0, 100, 100, parent, (HMENU)ID_LISTBOX, NULL, 0);
> -
> -
>    items_size = SendMessageA(listbox, LB_INITSTORAGE, 100, 0);
> -
>    ok(items_size >= 100, "expected at least 100, got %d\n", items_size);
> -
> -
>    ret = SendMessageA(listbox, LB_INITSTORAGE, 0, 0);
> -
>    ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
> -
> -
>    /* it doesn't grow since the space was already reserved */
> -
>    ret = SendMessageA(listbox, LB_INITSTORAGE, items_size, 0);
> -
>    ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
> -
> -
>    /* it doesn't shrink the reserved space */
> -
>    ret = SendMessageA(listbox, LB_INITSTORAGE, 42, 0);
> -
>    ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
> -
> -
>    /* now populate almost all of it so it's not reserved anymore */
> -
>    if (styles[i] & LBS_NODATA)
> -
>    {
> -
>        ret = SendMessageA(listbox, LB_SETCOUNT, items_size - 1, 0);
> -
>        ok(ret == 0, "unexpected return value %d\n", ret);
> -
>    }
> -
>    else
> -
>    {
> -
>        for (j = 0; j < items_size - 1; j++)
> -
>        {
> -
>            ret = SendMessageA(listbox, LB_INSERTSTRING, -1, (LPARAM)"");
> -
>            ok(ret == j, "expected %d, got %d\n", j, ret);
> -
>        }
> -
>    }
> -
> -
>    /* we still have one more reserved slot, so it doesn't grow yet */
> -
>    ret = SendMessageA(listbox, LB_INITSTORAGE, 1, 0);
> -
>    ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
> -
> -
>    /* fill the slot and check again, it should grow this time */
> -
>    ret = SendMessageA(listbox, LB_INSERTSTRING, -1, (LPARAM)"");

Don't you need to treat the NODATA case separately here just as you do above?
> -
>    ok(ret == items_size - 1, "expected %d, got %d\n", items_size - 1, ret);
> -
>    ret = SendMessageA(listbox, LB_INITSTORAGE, 0, 0);
> -
>    ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
> -
>    ret = SendMessageA(listbox, LB_INITSTORAGE, 1, 0);
> -
>    ok(ret > items_size, "expected it to grow past %d, got %d\n", items_size, ret);
> -
> -
>    DestroyWindow(listbox);
> - }
> - DestroyWindow(parent);
>
> +}
> -
>
> static void test_missing_lbuttonup(void)
>  {
>      HWND listbox, parent, capture;
> @@ -2425,6 +2488,7 @@ START_TEST(listbox)
>      test_listbox_LB_DIR();
>      test_listbox_dlgdir();
>      test_set_count();
> - test_init_storage();
> test_GetListBoxInfo();
> test_missing_lbuttonup();
> test_extents();
>
> -- 
> 2.19.1

[PATCH 4/5] comctl32/tests: Add tests for LB_INITSTORAGE

[Gabriel Ivăncescu]

On 2/7/19 11:55 AM, Huw Davies wrote:
> On Thu, Jan 31, 2019 at 05:23:22PM +0200, Gabriel Ivăncescu wrote:
> > Signed-off-by: Gabriel Ivăncescu [email protected]
> >
> > dlls/comctl32/tests/listbox.c | 64 +++++++++++++++++++++++++++++++++++
> >   1 file changed, 64 insertions(+)
> >
> > diff --git a/dlls/comctl32/tests/listbox.c b/dlls/comctl32/tests/listbox.c
> > index e789483..68e4073 100644
> > --- a/dlls/comctl32/tests/listbox.c
> > +++ b/dlls/comctl32/tests/listbox.c
> > @@ -1906,6 +1906,69 @@ static void test_GetListBoxInfo(void)
> >       DestroyWindow(parent);
> >   }
> >
> > +static void test_init_storage( void )
> > +{
> > - static const DWORD styles[] =
> > - {
> > -
> >    LBS_HASSTRINGS,
> > -
> >    LBS_NODATA | LBS_OWNERDRAWFIXED,
> > - };
> > - HWND parent, listbox;
> > - LONG ret, items_size;
> > - int i, j;
> > -
> > - parent = create_parent();
> > - for (i = 0; i < ARRAY_SIZE(styles); i++)
> > - {
> > -
> >    listbox = CreateWindowA(WC_LISTBOXA, "TestList", styles[i] | WS_CHILD,
> > -
> >                            0, 0, 100, 100, parent, (HMENU)ID_LISTBOX, NULL, 0);
> > -
> > -
> >    items_size = SendMessageA(listbox, LB_INITSTORAGE, 100, 0);
> > -
> >    ok(items_size >= 100, "expected at least 100, got %d\n", items_size);
> > -
> > -
> >    ret = SendMessageA(listbox, LB_INITSTORAGE, 0, 0);
> > -
> >    ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
> > -
> > -
> >    /* it doesn't grow since the space was already reserved */
> > -
> >    ret = SendMessageA(listbox, LB_INITSTORAGE, items_size, 0);
> > -
> >    ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
> > -
> > -
> >    /* it doesn't shrink the reserved space */
> > -
> >    ret = SendMessageA(listbox, LB_INITSTORAGE, 42, 0);
> > -
> >    ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
> > -
> > -
> >    /* now populate almost all of it so it's not reserved anymore */
> > -
> >    if (styles[i] & LBS_NODATA)
> > -
> >    {
> > -
> >        ret = SendMessageA(listbox, LB_SETCOUNT, items_size - 1, 0);
> > -
> >        ok(ret == 0, "unexpected return value %d\n", ret);
> > -
> >    }
> > -
> >    else
> > -
> >    {
> > -
> >        for (j = 0; j < items_size - 1; j++)
> > -
> >        {
> > -
> >            ret = SendMessageA(listbox, LB_INSERTSTRING, -1, (LPARAM)"");
> > -
> >            ok(ret == j, "expected %d, got %d\n", j, ret);
> > -
> >        }
> > -
> >    }
> > -
> > -
> >    /* we still have one more reserved slot, so it doesn't grow yet */
> > -
> >    ret = SendMessageA(listbox, LB_INITSTORAGE, 1, 0);
> > -
> >    ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
> > -
> > -
> >    /* fill the slot and check again, it should grow this time */
> > -
> >    ret = SendMessageA(listbox, LB_INSERTSTRING, -1, (LPARAM)"");
>
> Don't you need to treat the NODATA case separately here just as you do above?

I don't think so, but perhaps I'm missing something.

I treat it specially above because I want to also test SETCOUNT's 
behavior for init storage (test if it reserves or not, etc) and it only 
works with NODATA so it has to be special cased. The init storage itself 
should be the same.

[PATCH 4/5] comctl32/tests: Add tests for LB_INITSTORAGE

[Huw Davies]

On Thu, Feb 07, 2019 at 01:16:39PM +0200, Gabriel Ivăncescu wrote:
> On 2/7/19 11:55 AM, Huw Davies wrote:
> > On Thu, Jan 31, 2019 at 05:23:22PM +0200, Gabriel Ivăncescu wrote:
> > > Signed-off-by: Gabriel Ivăncescu [email protected]
> > >
> > > dlls/comctl32/tests/listbox.c | 64 +++++++++++++++++++++++++++++++++++
> > >   1 file changed, 64 insertions(+)
> > >
> > > diff --git a/dlls/comctl32/tests/listbox.c b/dlls/comctl32/tests/listbox.c
> > > index e789483..68e4073 100644
> > > --- a/dlls/comctl32/tests/listbox.c
> > > +++ b/dlls/comctl32/tests/listbox.c
> > > @@ -1906,6 +1906,69 @@ static void test_GetListBoxInfo(void)
> > >       DestroyWindow(parent);
> > >   }
> > > +static void test_init_storage( void )
> > > +{
> > > - static const DWORD styles[] =
> > > - {
> > > -
> > >    LBS_HASSTRINGS,
> > > -
> > >    LBS_NODATA | LBS_OWNERDRAWFIXED,
> > > - };
> > > - HWND parent, listbox;
> > > - LONG ret, items_size;
> > > - int i, j;
> > > -
> > > - parent = create_parent();
> > > - for (i = 0; i < ARRAY_SIZE(styles); i++)
> > > - {
> > > -
> > >    listbox = CreateWindowA(WC_LISTBOXA, "TestList", styles[i] | WS_CHILD,
> > > -
> > >                            0, 0, 100, 100, parent, (HMENU)ID_LISTBOX, NULL, 0);
> > > -
> > > -
> > >    items_size = SendMessageA(listbox, LB_INITSTORAGE, 100, 0);
> > > -
> > >    ok(items_size >= 100, "expected at least 100, got %d\n", items_size);
> > > -
> > > -
> > >    ret = SendMessageA(listbox, LB_INITSTORAGE, 0, 0);
> > > -
> > >    ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
> > > -
> > > -
> > >    /* it doesn't grow since the space was already reserved */
> > > -
> > >    ret = SendMessageA(listbox, LB_INITSTORAGE, items_size, 0);
> > > -
> > >    ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
> > > -
> > > -
> > >    /* it doesn't shrink the reserved space */
> > > -
> > >    ret = SendMessageA(listbox, LB_INITSTORAGE, 42, 0);
> > > -
> > >    ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
> > > -
> > > -
> > >    /* now populate almost all of it so it's not reserved anymore */
> > > -
> > >    if (styles[i] & LBS_NODATA)
> > > -
> > >    {
> > > -
> > >        ret = SendMessageA(listbox, LB_SETCOUNT, items_size - 1, 0);
> > > -
> > >        ok(ret == 0, "unexpected return value %d\n", ret);
> > > -
> > >    }
> > > -
> > >    else
> > > -
> > >    {
> > > -
> > >        for (j = 0; j < items_size - 1; j++)
> > > -
> > >        {
> > > -
> > >            ret = SendMessageA(listbox, LB_INSERTSTRING, -1, (LPARAM)"");
> > > -
> > >            ok(ret == j, "expected %d, got %d\n", j, ret);
> > > -
> > >        }
> > > -
> > >    }
> > > -
> > > -
> > >    /* we still have one more reserved slot, so it doesn't grow yet */
> > > -
> > >    ret = SendMessageA(listbox, LB_INITSTORAGE, 1, 0);
> > > -
> > >    ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
> > > -
> > > -
> > >    /* fill the slot and check again, it should grow this time */
> > > -
> > >    ret = SendMessageA(listbox, LB_INSERTSTRING, -1, (LPARAM)"");
> >
> > Don't you need to treat the NODATA case separately here just as you do above?
>
> I don't think so, but perhaps I'm missing something.
>
> I treat it specially above because I want to also test SETCOUNT's behavior
> for init storage (test if it reserves or not, etc) and it only works with
> NODATA so it has to be special cased. The init storage itself should be the
> same.

So does LB_INSERTSTRING really fill up a slot in the NODATA case?  That seems
odd, but ok.

[PATCH 4/5] comctl32/tests: Add tests for LB_INITSTORAGE

[Gabriel Ivăncescu]

On 2/7/19 1:35 PM, Huw Davies wrote:
> On Thu, Feb 07, 2019 at 01:16:39PM +0200, Gabriel Ivăncescu wrote:
> > On 2/7/19 11:55 AM, Huw Davies wrote:
> > > On Thu, Jan 31, 2019 at 05:23:22PM +0200, Gabriel Ivăncescu wrote:
> > > > Signed-off-by: Gabriel Ivăncescu [email protected]
> > > >
> > > > dlls/comctl32/tests/listbox.c | 64 +++++++++++++++++++++++++++++++++++
> > > >    1 file changed, 64 insertions(+)
> > > >
> > > > diff --git a/dlls/comctl32/tests/listbox.c b/dlls/comctl32/tests/listbox.c
> > > > index e789483..68e4073 100644
> > > > --- a/dlls/comctl32/tests/listbox.c
> > > > +++ b/dlls/comctl32/tests/listbox.c
> > > > @@ -1906,6 +1906,69 @@ static void test_GetListBoxInfo(void)
> > > >        DestroyWindow(parent);
> > > >    }
> > > > +static void test_init_storage( void )
> > > > +{
> > > > - static const DWORD styles[] =
> > > > - {
> > > > -
> > > >    LBS_HASSTRINGS,
> > > > -
> > > >    LBS_NODATA | LBS_OWNERDRAWFIXED,
> > > > - };
> > > > - HWND parent, listbox;
> > > > - LONG ret, items_size;
> > > > - int i, j;
> > > > -
> > > > - parent = create_parent();
> > > > - for (i = 0; i < ARRAY_SIZE(styles); i++)
> > > > - {
> > > > -
> > > >    listbox = CreateWindowA(WC_LISTBOXA, "TestList", styles[i] | WS_CHILD,
> > > > -
> > > >                            0, 0, 100, 100, parent, (HMENU)ID_LISTBOX, NULL, 0);
> > > > -
> > > > -
> > > >    items_size = SendMessageA(listbox, LB_INITSTORAGE, 100, 0);
> > > > -
> > > >    ok(items_size >= 100, "expected at least 100, got %d\n", items_size);
> > > > -
> > > > -
> > > >    ret = SendMessageA(listbox, LB_INITSTORAGE, 0, 0);
> > > > -
> > > >    ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
> > > > -
> > > > -
> > > >    /* it doesn't grow since the space was already reserved */
> > > > -
> > > >    ret = SendMessageA(listbox, LB_INITSTORAGE, items_size, 0);
> > > > -
> > > >    ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
> > > > -
> > > > -
> > > >    /* it doesn't shrink the reserved space */
> > > > -
> > > >    ret = SendMessageA(listbox, LB_INITSTORAGE, 42, 0);
> > > > -
> > > >    ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
> > > > -
> > > > -
> > > >    /* now populate almost all of it so it's not reserved anymore */
> > > > -
> > > >    if (styles[i] & LBS_NODATA)
> > > > -
> > > >    {
> > > > -
> > > >        ret = SendMessageA(listbox, LB_SETCOUNT, items_size - 1, 0);
> > > > -
> > > >        ok(ret == 0, "unexpected return value %d\n", ret);
> > > > -
> > > >    }
> > > > -
> > > >    else
> > > > -
> > > >    {
> > > > -
> > > >        for (j = 0; j < items_size - 1; j++)
> > > > -
> > > >        {
> > > > -
> > > >            ret = SendMessageA(listbox, LB_INSERTSTRING, -1, (LPARAM)"");
> > > > -
> > > >            ok(ret == j, "expected %d, got %d\n", j, ret);
> > > > -
> > > >        }
> > > > -
> > > >    }
> > > > -
> > > > -
> > > >    /* we still have one more reserved slot, so it doesn't grow yet */
> > > > -
> > > >    ret = SendMessageA(listbox, LB_INITSTORAGE, 1, 0);
> > > > -
> > > >    ok(ret == items_size, "expected %d, got %d\n", items_size, ret);
> > > > -
> > > > -
> > > >    /* fill the slot and check again, it should grow this time */
> > > > -
> > > >    ret = SendMessageA(listbox, LB_INSERTSTRING, -1, (LPARAM)"");
> > >
> > > Don't you need to treat the NODATA case separately here just as you do above?
> >
> > I don't think so, but perhaps I'm missing something.
> >
> > I treat it specially above because I want to also test SETCOUNT's behavior
> > for init storage (test if it reserves or not, etc) and it only works with
> > NODATA so it has to be special cased. The init storage itself should be the
> > same.
>
> So does LB_INSERTSTRING really fill up a slot in the NODATA case?  That seems
> odd, but ok.

Heh yeah, it does work but obviously adds no data to it (it does seem to 
separate reserved from "allocated" though, even if it's virtual for 
LBS_NODATA). Seems weird but it's just Microsoft's naming for 
INSERTSTRING :-)

[PATCH 5/5] comctl32/listbox: Shrink the item array with resize_storage

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu [email protected]
---
 dlls/comctl32/listbox.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
index f1ecc52..4ff98d4 100644
--- a/dlls/comctl32/listbox.c
+++ b/dlls/comctl32/listbox.c
@@ -131,7 +131,8 @@ static BOOL resize_storage(LB_DESCR *descr, UINT items_size)
 {
     LB_ITEMDATA *items;

-    if (items_size > descr->items_size)
+    if (items_size > descr->items_size ||
+        items_size + LB_ARRAY_GRANULARITY * 2 < descr->items_size)
     {
         items_size = (items_size + LB_ARRAY_GRANULARITY - 1) & ~(LB_ARRAY_GRANULARITY - 1);
         items = heap_realloc(descr->items, items_size * sizeof(LB_ITEMDATA));
@@ -704,7 +705,9 @@ static void LISTBOX_DrawFocusRect( LB_DESCR *descr, BOOL on )
  */
 static LRESULT LISTBOX_InitStorage( LB_DESCR *descr, INT nb_items )
 {
-    if (!resize_storage(descr, descr->nb_items + nb_items))
+    UINT new_size = descr->nb_items + nb_items;
+
+    if (new_size > descr->items_size && !resize_storage(descr, new_size))
         return LB_ERRSPACE;
     return descr->items_size;
 }
@@ -1666,7 +1669,6 @@ static void LISTBOX_DeleteItem( LB_DESCR *descr, INT index )
 static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index )
 {
     LB_ITEMDATA *item;
-    INT max_items;

if ((index < 0) || (index >= descr->nb_items)) return LB_ERR;

@@ -1686,20 +1688,8 @@ static LRESULT LISTBOX_RemoveItem( LB_DESCR *descr, INT index )
                        (descr->nb_items - index) * sizeof(LB_ITEMDATA) );
     if (descr->anchor_item == descr->nb_items) descr->anchor_item--;

-    /* Shrink the item array if possible */
+    resize_storage(descr, descr->nb_items);

-    max_items = descr->items_size;
-    if (descr->nb_items < max_items - 2*LB_ARRAY_GRANULARITY)
-    {
-        max_items -= LB_ARRAY_GRANULARITY;
-        item = HeapReAlloc( GetProcessHeap(), 0, descr->items,
-                            max_items * sizeof(LB_ITEMDATA) );
-        if (item)
-        {
-            descr->items_size = max_items;
-            descr->items = item;
-        }
-    }
     /* Repaint the items */

LISTBOX_UpdateScroll( descr );

[PATCH 5/5] comctl32/listbox: Shrink the item array with resize_storage

[Huw Davies]

On Thu, Jan 31, 2019 at 05:23:23PM +0200, Gabriel Ivăncescu wrote:
> Signed-off-by: Gabriel Ivăncescu [email protected]
>
> dlls/comctl32/listbox.c | 22 ++++++----------------
>  1 file changed, 6 insertions(+), 16 deletions(-)
>
> diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> index f1ecc52..4ff98d4 100644
> --- a/dlls/comctl32/listbox.c
> +++ b/dlls/comctl32/listbox.c
> @@ -131,7 +131,8 @@ static BOOL resize_storage(LB_DESCR *descr, UINT items_size)
>  {
>      LB_ITEMDATA *items;
> - if (items_size > descr->items_size)
> - if (items_size > descr->items_size ||
> -
>    items_size + LB_ARRAY_GRANULARITY * 2 < descr->items_size)
> {
>        items_size = (items_size + LB_ARRAY_GRANULARITY - 1) & ~(LB_ARRAY_GRANULARITY - 1);
>        items = heap_realloc(descr->items, items_size * sizeof(LB_ITEMDATA));
>
> @@ -704,7 +705,9 @@ static void LISTBOX_DrawFocusRect( LB_DESCR *descr, BOOL on )
>   */
>  static LRESULT LISTBOX_InitStorage( LB_DESCR *descr, INT nb_items )
>  {
> - if (!resize_storage(descr, descr->nb_items + nb_items))
> - UINT new_size = descr->nb_items + nb_items;
> -
> - if (new_size > descr->items_size && !resize_storage(descr, new_size))
>     return LB_ERRSPACE;
> return descr->items_size;

To avoid patching something you changed in [2/5] you could move this hunk to [2/5].

[PATCH 5/5] comctl32/listbox: Shrink the item array with resize_storage

[Gabriel Ivăncescu]

On 2/7/19 11:54 AM, Huw Davies wrote:
> On Thu, Jan 31, 2019 at 05:23:23PM +0200, Gabriel Ivăncescu wrote:
> > Signed-off-by: Gabriel Ivăncescu [email protected]
> >
> > dlls/comctl32/listbox.c | 22 ++++++----------------
> >   1 file changed, 6 insertions(+), 16 deletions(-)
> >
> > diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> > index f1ecc52..4ff98d4 100644
> > --- a/dlls/comctl32/listbox.c
> > +++ b/dlls/comctl32/listbox.c
> > @@ -131,7 +131,8 @@ static BOOL resize_storage(LB_DESCR *descr, UINT items_size)
> >   {
> >       LB_ITEMDATA *items;
> > - if (items_size > descr->items_size)
> > - if (items_size > descr->items_size ||
> > -
> >    items_size + LB_ARRAY_GRANULARITY * 2 < descr->items_size)
> > {
> >     items_size = (items_size + LB_ARRAY_GRANULARITY - 1) & ~(LB_ARRAY_GRANULARITY - 1);
> >     items = heap_realloc(descr->items, items_size * sizeof(LB_ITEMDATA));
> >
> > @@ -704,7 +705,9 @@ static void LISTBOX_DrawFocusRect( LB_DESCR *descr, BOOL on )
> >    */
> >   static LRESULT LISTBOX_InitStorage( LB_DESCR *descr, INT nb_items )
> >   {
> > - if (!resize_storage(descr, descr->nb_items + nb_items))
> > - UINT new_size = descr->nb_items + nb_items;
> > -
> > - if (new_size > descr->items_size && !resize_storage(descr, new_size))
> >      return LB_ERRSPACE;
> >  return descr->items_size;
>
> To avoid patching something you changed in [2/5] you could move this hunk to [2/5].

Sure will do, I wanted to reduce the patch size/impact.

[Huw Davies]

On Thu, Jan 31, 2019 at 05:23:19PM +0200, Gabriel Ivăncescu wrote:
> Signed-off-by: Gabriel Ivăncescu [email protected]
>
> dlls/comctl32/listbox.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> index cb645b4..71f1c05 100644
> --- a/dlls/comctl32/listbox.c
> +++ b/dlls/comctl32/listbox.c
> @@ -489,7 +489,7 @@ static INT LISTBOX_GetItemFromPoint( const LB_DESCR *descr, INT x, INT y )
> - Paint an item.
>
> */
>  static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect,
> -
> 	       INT index, UINT action, BOOL ignoreFocus )
> -
> 	       UINT index, UINT action, BOOL ignoreFocus )

Could you leave this change out?  descr->nb_items itself is signed as is the index
parameter of LISTBOX_RePaintItem().
> {
>      BOOL selected = FALSE, focused;
>      LB_ITEMDATA *item = NULL;
> @@ -508,7 +508,7 @@ static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect,
>          RECT r;
>          HRGN hrgn;
> - if (!item)
> - if (index >= descr->nb_items)
> {
>   if (action == ODA_FOCUS)
> DrawFocusRect( hdc, rect );
>
> -- 
> 2.19.1

[Gabriel Ivăncescu]

On 2/7/19 11:52 AM, Huw Davies wrote:
> On Thu, Jan 31, 2019 at 05:23:19PM +0200, Gabriel Ivăncescu wrote:
> > Signed-off-by: Gabriel Ivăncescu [email protected]
> >
> > dlls/comctl32/listbox.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> > index cb645b4..71f1c05 100644
> > --- a/dlls/comctl32/listbox.c
> > +++ b/dlls/comctl32/listbox.c
> > @@ -489,7 +489,7 @@ static INT LISTBOX_GetItemFromPoint( const LB_DESCR *descr, INT x, INT y )
> > - Paint an item.
> >
> > */
> >   static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect,
> > -
> > 	       INT index, UINT action, BOOL ignoreFocus )
> > -
> > 	       UINT index, UINT action, BOOL ignoreFocus )
>
> Could you leave this change out?  descr->nb_items itself is signed as is the index
> parameter of LISTBOX_RePaintItem().

Hi Huw,

We have to make it unsigned because checking for bounds then only really 
needs one comparison, as Nikolay suggested awhile ago. And the bounds 
check will be needed for LBS_NODATA later (can't test for NULL as that 
would be wrong when the item is 0 and nodata).

[Huw Davies]

On Thu, Feb 07, 2019 at 01:13:36PM +0200, Gabriel Ivăncescu wrote:
> On 2/7/19 11:52 AM, Huw Davies wrote:
> > On Thu, Jan 31, 2019 at 05:23:19PM +0200, Gabriel Ivăncescu wrote:
> > > Signed-off-by: Gabriel Ivăncescu [email protected]
> > >
> > > dlls/comctl32/listbox.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> > > index cb645b4..71f1c05 100644
> > > --- a/dlls/comctl32/listbox.c
> > > +++ b/dlls/comctl32/listbox.c
> > > @@ -489,7 +489,7 @@ static INT LISTBOX_GetItemFromPoint( const LB_DESCR *descr, INT x, INT y )
> > > - Paint an item.
> > >
> > > */
> > >   static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect,
> > > -
> > > 	       INT index, UINT action, BOOL ignoreFocus )
> > > -
> > > 	       UINT index, UINT action, BOOL ignoreFocus )
> >
> > Could you leave this change out?  descr->nb_items itself is signed as is the index
> > parameter of LISTBOX_RePaintItem().
>
> Hi Huw,
>
> We have to make it unsigned because checking for bounds then only really
> needs one comparison, as Nikolay suggested awhile ago. And the bounds check
> will be needed for LBS_NODATA later (can't test for NULL as that would be
> wrong when the item is 0 and nodata).

How does LISTBOX_PaintItem() get called with index < 0 ?  If it did, we'd
already be in trouble.

My point is really that if we're going to fix the signess of index, we need to
go through and fix it properly in all the code, not just one parameter at a
time.  And then it should be a separate patch.

Huw.

[Gabriel Ivăncescu]

On 2/7/19 1:29 PM, Huw Davies wrote:
> On Thu, Feb 07, 2019 at 01:13:36PM +0200, Gabriel Ivăncescu wrote:
> > On 2/7/19 11:52 AM, Huw Davies wrote:
> > > On Thu, Jan 31, 2019 at 05:23:19PM +0200, Gabriel Ivăncescu wrote:
> > > > Signed-off-by: Gabriel Ivăncescu [email protected]
> > > >
> > > > dlls/comctl32/listbox.c | 4 ++--
> > > >    1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> > > > index cb645b4..71f1c05 100644
> > > > --- a/dlls/comctl32/listbox.c
> > > > +++ b/dlls/comctl32/listbox.c
> > > > @@ -489,7 +489,7 @@ static INT LISTBOX_GetItemFromPoint( const LB_DESCR *descr, INT x, INT y )
> > > >     * Paint an item.
> > > >     */
> > > >    static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const RECT *rect,
> > > > -
> > > > 	       INT index, UINT action, BOOL ignoreFocus )
> > > > -
> > > > 	       UINT index, UINT action, BOOL ignoreFocus )
> > >
> > > Could you leave this change out?  descr->nb_items itself is signed as is the index
> > > parameter of LISTBOX_RePaintItem().
> >
> > Hi Huw,
> >
> > We have to make it unsigned because checking for bounds then only really
> > needs one comparison, as Nikolay suggested awhile ago. And the bounds check
> > will be needed for LBS_NODATA later (can't test for NULL as that would be
> > wrong when the item is 0 and nodata).
>
> How does LISTBOX_PaintItem() get called with index < 0 ?  If it did, we'd
> already be in trouble.
>
> My point is really that if we're going to fix the signess of index, we need to
> go through and fix it properly in all the code, not just one parameter at a
> time.  And then it should be a separate patch.
>
> Huw.

I think the index *can* be negative in many cases, so not sure if it can 
be easily changed in other parts of the code (or at least it can be -1).

In this particular case, since we only check for bounds, it made more 
sense to treat it as unsigned (less comparisons and clutter in the 
code), or at least it's what Nikolay initially suggested if I recall 
correctly.

Now my question is: if I keep it signed, should I change the check to 
something like:

if (index < 0 || index >= descr->nb_items)

or keep it as just  if (index >= descr->nb_items)  like now, and assume 
the index won't be negative? It's an ERR path if it happens, so I 
assumed it has a reason to be there.

[Gabriel Ivăncescu]

On 2/7/19 1:54 PM, Gabriel Ivăncescu wrote:
> On 2/7/19 1:29 PM, Huw Davies wrote:
> > On Thu, Feb 07, 2019 at 01:13:36PM +0200, Gabriel Ivăncescu wrote:
> > > On 2/7/19 11:52 AM, Huw Davies wrote:
> > > > On Thu, Jan 31, 2019 at 05:23:19PM +0200, Gabriel Ivăncescu wrote:
> > > > > Signed-off-by: Gabriel Ivăncescu [email protected]
> > > > >
> > > > > dlls/comctl32/listbox.c | 4 ++--
> > > > >    1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> > > > > index cb645b4..71f1c05 100644
> > > > > --- a/dlls/comctl32/listbox.c
> > > > > +++ b/dlls/comctl32/listbox.c
> > > > > @@ -489,7 +489,7 @@ static INT LISTBOX_GetItemFromPoint( const 
> > > > > LB_DESCR *descr, INT x, INT y )
> > > > >     * Paint an item.
> > > > >     */
> > > > >    static void LISTBOX_PaintItem( LB_DESCR *descr, HDC hdc, const 
> > > > > RECT *rect,
> > > > > -                   INT index, UINT action, BOOL ignoreFocus )
> > > > > +                   UINT index, UINT action, BOOL ignoreFocus )
> > > >
> > > > Could you leave this change out?  descr->nb_items itself is signed 
> > > > as is the index
> > > > parameter of LISTBOX_RePaintItem().
> > >
> > > Hi Huw,
> > >
> > > We have to make it unsigned because checking for bounds then only really
> > > needs one comparison, as Nikolay suggested awhile ago. And the bounds 
> > > check
> > > will be needed for LBS_NODATA later (can't test for NULL as that 
> > > would be
> > > wrong when the item is 0 and nodata).
> >
> > How does LISTBOX_PaintItem() get called with index < 0 ?  If it did, we'd
> > already be in trouble.
> >
> > My point is really that if we're going to fix the signess of index, we 
> > need to
> > go through and fix it properly in all the code, not just one parameter 
> > at a
> > time.  And then it should be a separate patch.
> >
> > Huw.
>
> I think the index *can* be negative in many cases, so not sure if it can 
> be easily changed in other parts of the code (or at least it can be -1).
>
> In this particular case, since we only check for bounds, it made more 
> sense to treat it as unsigned (less comparisons and clutter in the 
> code), or at least it's what Nikolay initially suggested if I recall 
> correctly.
>
> Now my question is: if I keep it signed, should I change the check to 
> something like:
>
> if (index < 0 || index >= descr->nb_items)
>
> or keep it as just  if (index >= descr->nb_items)  like now, and assume 
> the index won't be negative? It's an ERR path if it happens, so I 
> assumed it has a reason to be there.

Sorry I meant: the index can be negative in many parts of the code. 
*Not* that it can be negative for PaintItem -- I don't know about the 
latter. Was just playing it safe.

[Huw Davies]

On Thu, Feb 07, 2019 at 01:57:09PM +0200, Gabriel Ivăncescu wrote:
> On 2/7/19 1:54 PM, Gabriel Ivăncescu wrote:
> > On 2/7/19 1:29 PM, Huw Davies wrote:
> > > On Thu, Feb 07, 2019 at 01:13:36PM +0200, Gabriel Ivăncescu wrote:
> > > > On 2/7/19 11:52 AM, Huw Davies wrote:
> > > > > On Thu, Jan 31, 2019 at 05:23:19PM +0200, Gabriel Ivăncescu wrote:
> > > > > > Signed-off-by: Gabriel Ivăncescu [email protected]
> > > > > >
> > > > > > dlls/comctl32/listbox.c | 4 ++--
> > > > > >    1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> > > > > > index cb645b4..71f1c05 100644
> > > > > > --- a/dlls/comctl32/listbox.c
> > > > > > +++ b/dlls/comctl32/listbox.c
> > > > > > @@ -489,7 +489,7 @@ static INT LISTBOX_GetItemFromPoint(
> > > > > > const LB_DESCR *descr, INT x, INT y )
> > > > > >     * Paint an item.
> > > > > >     */
> > > > > >    static void LISTBOX_PaintItem( LB_DESCR *descr, HDC
> > > > > > hdc, const RECT *rect,
> > > > > > -                   INT index, UINT action, BOOL ignoreFocus )
> > > > > > +                   UINT index, UINT action, BOOL ignoreFocus )
> > > > >
> > > > > Could you leave this change out?  descr->nb_items itself is
> > > > > signed as is the index
> > > > > parameter of LISTBOX_RePaintItem().
> > > >
> > > > Hi Huw,
> > > >
> > > > We have to make it unsigned because checking for bounds then only really
> > > > needs one comparison, as Nikolay suggested awhile ago. And the
> > > > bounds check
> > > > will be needed for LBS_NODATA later (can't test for NULL as that
> > > > would be
> > > > wrong when the item is 0 and nodata).
> > >
> > > How does LISTBOX_PaintItem() get called with index < 0 ?  If it did, we'd
> > > already be in trouble.
> > >
> > > My point is really that if we're going to fix the signess of index,
> > > we need to
> > > go through and fix it properly in all the code, not just one
> > > parameter at a
> > > time.  And then it should be a separate patch.
> > >
> > > Huw.
> >
> > I think the index *can* be negative in many cases, so not sure if it can
> > be easily changed in other parts of the code (or at least it can be -1).
> >
> > In this particular case, since we only check for bounds, it made more
> > sense to treat it as unsigned (less comparisons and clutter in the
> > code), or at least it's what Nikolay initially suggested if I recall
> > correctly.
> >
> > Now my question is: if I keep it signed, should I change the check to
> > something like:
> >
> > if (index < 0 || index >= descr->nb_items)
> >
> > or keep it as just  if (index >= descr->nb_items)  like now, and assume
> > the index won't be negative? It's an ERR path if it happens, so I
> > assumed it has a reason to be there.
>
> Sorry I meant: the index can be negative in many parts of the code. *Not*
> that it can be negative for PaintItem -- I don't know about the latter. Was
> just playing it safe.

The current code assumes index >= 0 (otherwise item will be invalid)
so, for this patch, it's ok to carry on that assumption.

Huw.