[PATCH 1/5] comctl32/listbox: Check for out of bounds index using an unsigned index
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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 ); -- 2.19.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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 */ -- 2.19.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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; } -- 2.19.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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(); -- 2.19.1
On Thu, Jan 31, 2019 at 05:23:22PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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
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 <gabrielopcode(a)gmail.com> --- 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.
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 <gabrielopcode(a)gmail.com> --- 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.
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 <gabrielopcode(a)gmail.com> --- 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 :-)
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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 ); -- 2.19.1
On Thu, Jan 31, 2019 at 05:23:23PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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].
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 <gabrielopcode(a)gmail.com> --- 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.
On Thu, Jan 31, 2019 at 05:23:19PM +0200, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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
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 <gabrielopcode(a)gmail.com> --- 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).
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 <gabrielopcode(a)gmail.com> --- 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.
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 <gabrielopcode(a)gmail.com> --- 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.
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 <gabrielopcode(a)gmail.com> --- 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.
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 <gabrielopcode(a)gmail.com> --- 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.
participants (2)
-
Gabriel Ivăncescu -
Huw Davies