[PATCH v2 0/1] MR6169: comctl32: fixed first we check index then array access by index is safe

[Herman Semenov (＠GermanAizek)]

I think this PR changes will protect against situation if out-of-bounds access array.

--
  v2: comctl32: fixed first we check index then array access by index is safe

https://gitlab.winehq.org/wine/wine/-/merge_requests/6169

[PATCH v2 1/1] comctl32: fixed first we check index then array access by index is safe

[Herman Semenov]

From: Herman Semenov

---
 dlls/comctl32/toolbar.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/dlls/comctl32/toolbar.c b/dlls/comctl32/toolbar.c
index 690a02db6ee..26d662d1a44 100644
--- a/dlls/comctl32/toolbar.c
+++ b/dlls/comctl32/toolbar.c
@@ -3240,11 +3240,12 @@ static LRESULT
 TOOLBAR_DeleteButton (TOOLBAR_INFO *infoPtr, INT nIndex)
 {
     NMTOOLBARW nmtb;
-    TBUTTON_INFO *btnPtr = &infoPtr->buttons[nIndex];

if ((nIndex < 0) || (nIndex >= infoPtr->nNumButtons))
         return FALSE;

+    TBUTTON_INFO *btnPtr = &infoPtr->buttons[nIndex];
+
     memset(&nmtb, 0, sizeof(nmtb));
     nmtb.iItem = btnPtr->idCommand;
     nmtb.tbButton.iBitmap = btnPtr->iBitmap;
@@ -3545,10 +3546,11 @@ TOOLBAR_GetItemRect (const TOOLBAR_INFO *infoPtr, INT nIndex, LPRECT lpRect)
 {
     TBUTTON_INFO *btnPtr;

-    btnPtr = &infoPtr->buttons[nIndex];
     if ((nIndex < 0) || (nIndex >= infoPtr->nNumButtons))
    return FALSE;

+    btnPtr = &infoPtr->buttons[nIndex];
+
     if (lpRect == NULL)
    return FALSE;
     if (btnPtr->fsState & TBSTATE_HIDDEN)
@@ -3596,10 +3598,12 @@ TOOLBAR_GetRect (const TOOLBAR_INFO *infoPtr, INT Id, LPRECT lpRect)
     INT        nIndex;

nIndex = TOOLBAR_GetButtonIndex (infoPtr, Id, FALSE);
-    btnPtr = &infoPtr->buttons[nIndex];
+
     if ((nIndex < 0) || (nIndex >= infoPtr->nNumButtons))
    return FALSE;

+    btnPtr = &infoPtr->buttons[nIndex];
+
     if (lpRect == NULL)
    return FALSE;

[PATCH v2 1/1] comctl32: fixed first we check index then array access by index is safe

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The tests also ran into some preexisting test failures. If you know how
to fix them that would be helpful. See the TestBot job for the details:

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=147408

Your paranoid android.

=== debian11b (64 bit WoW report) ===

wmvcore:
wmvcore.c:2831: Test failed: Got pts 2860000.
wmvcore.c:2843: Test failed: Got pts 3250000.
wmvcore.c:2850: Test failed: Got pts 2860000.
wmvcore.c:2862: Test failed: Got pts 3250000.
wmvcore.c:2869: Test succeeded inside todo block: Got pts 2860000.

[Herman Semenov (＠GermanAizek)]

On Mon Jul 29 04:13:29 2024 +0000, **** wrote:
> changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/6169/diffs?diff_id=124062&start_sha=48787018aa2355f29a5d837c5ebf35eb8fb57cd7#80bec5fb0f9a667beeb7953ce870eb92dd1cfbdc_3555_3555)

@zhiyi fixed it https://gitlab.winehq.org/wine/wine/-/merge_requests/6169/diffs?commit_id=d7...

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/toolbar.c:
> TOOLBAR_DeleteButton (TOOLBAR_INFO *infoPtr, INT nIndex)
>  {

I don't think this patch is needed at all. Is this for warnings from compilers or some static analysis tool?

[Nikolay Sivov (＠nsivov)]

On Mon Jul 29 09:52:59 2024 +0000, Zhiyi Zhang wrote:
> I don't think this patch is needed at all. Is this for warnings from
> compilers or some static analysis tool?

Agreed. We could have a helper to return btnPtr for given index, also checking for bounds. Otherwise this change shouldn't make any difference.

[Alfred Agrell (＠Alcaro)]

On Tue Jul 30 08:54:54 2024 +0000, Nikolay Sivov wrote:
> Agreed. We could have a helper to return btnPtr for given index, also
> checking for bounds. Otherwise this change shouldn't make any difference.

I'm nor aware of this kind of stuff causing any trouble in practice, but I agree with fixing it anyways. Creating a two-or-more-past-the-end pointer is UB.

I also agree with refactoring the bounds check into a function instead of having five different copies. (The last two are TOOLBAR_GetButton and TOOLBAR_SetCmdId, which are correct, but still duplicate.)

Finally, I strongly suspect TOOLBAR_GetCheckedGroupButtonIndex contains a similar bug. I suspect the > should be >=.

[Herman Semenov (＠GermanAizek)]

On Tue Jul 30 18:07:42 2024 +0000, Alfred Agrell wrote:
> I'm nor aware of this kind of stuff causing any trouble in practice, but
> I agree with fixing it anyways. Creating a two-or-more-past-the-end
> pointer is UB.
> I also agree with refactoring the bounds check into a function instead
> of having five different copies. (The last two are TOOLBAR_GetButton and
> TOOLBAR_SetCmdId, which are correct, but still duplicate.)
> Finally, I strongly suspect TOOLBAR_GetCheckedGroupButtonIndex contains
> a similar bug. I suspect the > should be >=.

@zhiyi, how do you like offer from @Alcaro?

[Zhiyi Zhang (＠zhiyi)]

On Mon Oct 21 14:46:07 2024 +0000, Herman Semenov wrote:
> @zhiyi, how do you like offer from @Alcaro?

Having a helper makes sense.