[PATCH v3 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.

--
  v3: comctl32: fixed first we check index then array access by index is safe
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6169

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

[Semenov Herman (Семенов Герман)]

From: Semenov Herman (Семенов Герман)<GermanAizek(a)yandex.ru>

---
 dlls/comctl32/toolbar.c | 87 +++++++++++++++++++----------------------
 1 file changed, 40 insertions(+), 47 deletions(-)

diff --git a/dlls/comctl32/toolbar.c b/dlls/comctl32/toolbar.c
index 690a02db6ee..475ca4e4e90 100644
--- a/dlls/comctl32/toolbar.c
+++ b/dlls/comctl32/toolbar.c
@@ -1926,6 +1926,15 @@ TOOLBAR_GetButtonIndex (const TOOLBAR_INFO *infoPtr, INT idCommand, BOOL Command
     return -1;
 }
 
+static BOOL
+TOOLBAR_CheckIndexButton (TBUTTON_INFO *btnPtr, const TOOLBAR_INFO *infoPtr, INT nIndex)
+{
+    if ((nIndex < 0) || (nIndex >= infoPtr->nNumButtons))
+        return FALSE;
+
+    btnPtr = &infoPtr->buttons[nIndex];
+    return TRUE;
+}
 
 static INT
 TOOLBAR_GetCheckedGroupButtonIndex (const TOOLBAR_INFO *infoPtr, INT nIndex)
@@ -1933,11 +1942,10 @@ TOOLBAR_GetCheckedGroupButtonIndex (const TOOLBAR_INFO *infoPtr, INT nIndex)
     TBUTTON_INFO *btnPtr;
     INT nRunIndex;
 
-    if ((nIndex < 0) || (nIndex > infoPtr->nNumButtons))
-	return -1;
-
     /* check index button */
-    btnPtr = &infoPtr->buttons[nIndex];
+    if (!TOOLBAR_CheckIndexButton(btnPtr, infoPtr, nIndex))
+	    return -1;
+
     if ((btnPtr->fsStyle & BTNS_CHECKGROUP) == BTNS_CHECKGROUP) {
 	if (btnPtr->fsState & TBSTATE_CHECKED)
 	    return nIndex;
@@ -3146,10 +3154,9 @@ TOOLBAR_ChangeBitmap (TOOLBAR_INFO *infoPtr, INT Id, INT Index)
     TRACE("button %d, iBitmap now %d\n", Id, Index);
 
     nIndex = TOOLBAR_GetButtonIndex (infoPtr, Id, FALSE);
-    if (nIndex == -1)
-	return FALSE;
+    if (!TOOLBAR_CheckIndexButton(btnPtr, infoPtr, nIndex))
+	    return FALSE;
 
-    btnPtr = &infoPtr->buttons[nIndex];
     btnPtr->iBitmap = Index;
 
     /* we HAVE to erase the background, the new bitmap could be */
@@ -3172,10 +3179,8 @@ TOOLBAR_CheckButton (TOOLBAR_INFO *infoPtr, INT Id, LPARAM lParam)
 
     TRACE("hwnd %p, btn index %d, lParam %Ix\n", infoPtr->hwndSelf, nIndex, lParam);
 
-    if (nIndex == -1)
-	return FALSE;
-
-    btnPtr = &infoPtr->buttons[nIndex];
+    if (!TOOLBAR_CheckIndexButton(btnPtr, infoPtr, nIndex))
+	    return FALSE;
 
     bChecked = (btnPtr->fsState & TBSTATE_CHECKED) != 0;
 
@@ -3240,9 +3245,9 @@ static LRESULT
 TOOLBAR_DeleteButton (TOOLBAR_INFO *infoPtr, INT nIndex)
 {
     NMTOOLBARW nmtb;
-    TBUTTON_INFO *btnPtr = &infoPtr->buttons[nIndex];
+    TBUTTON_INFO *btnPtr;
 
-    if ((nIndex < 0) || (nIndex >= infoPtr->nNumButtons))
+    if (!TOOLBAR_CheckIndexButton(btnPtr, infoPtr, nIndex))
         return FALSE;
 
     memset(&nmtb, 0, sizeof(nmtb));
@@ -3304,10 +3309,8 @@ TOOLBAR_EnableButton (TOOLBAR_INFO *infoPtr, INT Id, LPARAM lParam)
 
     TRACE("hwnd %p, btn id %d, lParam %Ix\n", infoPtr->hwndSelf, Id, lParam);
 
-    if (nIndex == -1)
-	return FALSE;
-
-    btnPtr = &infoPtr->buttons[nIndex];
+    if (!TOOLBAR_CheckIndexButton(btnPtr, infoPtr, nIndex))
+	    return FALSE;
 
     bState = btnPtr->fsState & TBSTATE_ENABLED;
 
@@ -3361,10 +3364,9 @@ TOOLBAR_GetButton (const TOOLBAR_INFO *infoPtr, INT nIndex, TBBUTTON *lpTbb)
     if (lpTbb == NULL)
 	return FALSE;
 
-    if ((nIndex < 0) || (nIndex >= infoPtr->nNumButtons))
-	return FALSE;
+    if (!TOOLBAR_CheckIndexButton(btnPtr, infoPtr, nIndex))
+	    return FALSE;
 
-    btnPtr = &infoPtr->buttons[nIndex];
     lpTbb->iBitmap   = btnPtr->iBitmap;
     lpTbb->idCommand = btnPtr->idCommand;
     lpTbb->fsState   = btnPtr->fsState;
@@ -3399,10 +3401,9 @@ TOOLBAR_GetButtonInfoT(const TOOLBAR_INFO *infoPtr, INT Id, LPTBBUTTONINFOW lpTb
     }
 
     nIndex = TOOLBAR_GetButtonIndex (infoPtr, Id, lpTbInfo->dwMask & TBIF_BYINDEX);
-    if (nIndex == -1)
-	return -1;
+    if (!TOOLBAR_CheckIndexButton(btnPtr, infoPtr, nIndex))
+	    return -1;
 
-    btnPtr = &infoPtr->buttons[nIndex];
     if (lpTbInfo->dwMask & TBIF_COMMAND)
 	lpTbInfo->idCommand = btnPtr->idCommand;
     if (lpTbInfo->dwMask & TBIF_IMAGE)
@@ -3545,9 +3546,8 @@ 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;
+    if (!TOOLBAR_CheckIndexButton(btnPtr, infoPtr, nIndex))
+	    return FALSE;
 
     if (lpRect == NULL)
 	return FALSE;
@@ -3596,9 +3596,9 @@ 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;
+
+    if (!TOOLBAR_CheckIndexButton(btnPtr, infoPtr, nIndex))
+	    return FALSE;
 
     if (lpRect == NULL)
 	return FALSE;
@@ -3682,10 +3682,9 @@ TOOLBAR_HideButton (TOOLBAR_INFO *infoPtr, INT Id, BOOL fHide)
     TRACE("\n");
 
     nIndex = TOOLBAR_GetButtonIndex (infoPtr, Id, FALSE);
-    if (nIndex == -1)
-	return FALSE;
+    if (!TOOLBAR_CheckIndexButton(btnPtr, infoPtr, nIndex))
+	    return FALSE;
 
-    btnPtr = &infoPtr->buttons[nIndex];
     oldState = btnPtr->fsState;
 
     if (fHide)
@@ -3717,10 +3716,9 @@ TOOLBAR_Indeterminate (const TOOLBAR_INFO *infoPtr, INT Id, BOOL fIndeterminate)
     DWORD oldState;
 
     nIndex = TOOLBAR_GetButtonIndex (infoPtr, Id, FALSE);
-    if (nIndex == -1)
-	return FALSE;
+    if (!TOOLBAR_CheckIndexButton(btnPtr, infoPtr, nIndex))
+	    return FALSE;
 
-    btnPtr = &infoPtr->buttons[nIndex];
     oldState = btnPtr->fsState;
 
     if (fIndeterminate)
@@ -3907,10 +3905,9 @@ TOOLBAR_MarkButton (const TOOLBAR_INFO *infoPtr, INT Id, BOOL fMark)
     TRACE("hwnd = %p, Id = %d, fMark = 0%d\n", infoPtr->hwndSelf, Id, fMark);
 
     nIndex = TOOLBAR_GetButtonIndex (infoPtr, Id, FALSE);
-    if (nIndex == -1)
+    if (!TOOLBAR_CheckIndexButton(btnPtr, infoPtr, nIndex))
         return FALSE;
 
-    btnPtr = &infoPtr->buttons[nIndex];
     oldState = btnPtr->fsState;
 
     if (fMark)
@@ -4003,10 +4000,9 @@ TOOLBAR_PressButton (const TOOLBAR_INFO *infoPtr, INT Id, BOOL fPress)
     DWORD oldState;
 
     nIndex = TOOLBAR_GetButtonIndex (infoPtr, Id, FALSE);
-    if (nIndex == -1)
-	return FALSE;
+    if (!TOOLBAR_CheckIndexButton(btnPtr, infoPtr, nIndex))
+	    return FALSE;
 
-    btnPtr = &infoPtr->buttons[nIndex];
     oldState = btnPtr->fsState;
 
     if (fPress)
@@ -4432,10 +4428,9 @@ TOOLBAR_SetButtonInfo (TOOLBAR_INFO *infoPtr, INT Id,
 	return FALSE;
 
     nIndex = TOOLBAR_GetButtonIndex (infoPtr, Id, lptbbi->dwMask & TBIF_BYINDEX);
-    if (nIndex == -1)
-	return FALSE;
+    if (!TOOLBAR_CheckIndexButton(btnPtr, infoPtr, nIndex))
+	    return FALSE;
 
-    btnPtr = &infoPtr->buttons[nIndex];
     if (lptbbi->dwMask & TBIF_COMMAND)
 	btnPtr->idCommand = lptbbi->idCommand;
     if (lptbbi->dwMask & TBIF_IMAGE)
@@ -4997,10 +4992,8 @@ TOOLBAR_SetState (TOOLBAR_INFO *infoPtr, INT Id, LPARAM lParam)
     INT nIndex;
 
     nIndex = TOOLBAR_GetButtonIndex (infoPtr, Id, FALSE);
-    if (nIndex == -1)
-	return FALSE;
-
-    btnPtr = &infoPtr->buttons[nIndex];
+    if (!TOOLBAR_CheckIndexButton(btnPtr, infoPtr, nIndex))
+	    return FALSE;
 
     /* if hidden state has changed the invalidate entire window and recalc */
     if ((btnPtr->fsState & TBSTATE_HIDDEN) != (LOWORD(lParam) & TBSTATE_HIDDEN)) {
-- 
GitLab

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

[Herman Semenov (＠GermanAizek)]

On Mon Oct 21 14:50:35 2024 +0000, Zhiyi Zhang wrote:
> Having a helper makes sense.
> @zhiyi, Did I understand you correctly that you need this?
> https://gitlab.winehq.org/wine/wine/-/merge_requests/6169/diffs?commit_id=7c...

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

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/toolbar.c:
> TBUTTON_INFO *btnPtr;
>      INT nRunIndex;
>
> -    if ((nIndex < 0) || (nIndex > infoPtr->nNumButtons))
> -	return -1;
> -
>      /* check index button */
> -    btnPtr = &infoPtr->buttons[nIndex];
> +    if (!TOOLBAR_CheckIndexButton(btnPtr, infoPtr, nIndex))
> +	    return -1;

Don't mix tabs and spaces for indentation. You either use tabs or spaces. Same for other places.

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

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/toolbar.c:
> return -1;
>  }
>
> +static BOOL
> +TOOLBAR_CheckIndexButton (TBUTTON_INFO *btnPtr, const TOOLBAR_INFO *infoPtr, INT nIndex)

btnPtr should be of type TBUTTON_INFO **, which is pointer to a TBUTTON_INFO *. Currently, this helper doesn't set the btnPtr for the caller.

Let's just return &infoPtr->buttons[nIndex] when the index is valid and NULL when it's not. And change its name to something like TOOLBAR_GetButton()

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

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/toolbar.c:
> return -1;
>  }

Let's rename the patch commit subject to something like "comctl32/toolbar: Add a helper to check button index."

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

[Zhiyi Zhang (＠zhiyi)]

There are various errors in the GitLab CI. Please address them as well.

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