[PATCH] comctl32: Avoid an unneeded lstrlenW() call.

[Francois Gouget]

Signed-off-by: Francois Gouget 
---

It seems wasteful to compute the length of the string when all we care 
about is the first character. Plus the 'if (*str)' pattern is pretty 
common and used extensively in Wine already.

dlls/comctl32/toolbar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dlls/comctl32/toolbar.c b/dlls/comctl32/toolbar.c
index 6a51ad7fc17..ec3b49890fd 100644
--- a/dlls/comctl32/toolbar.c
+++ b/dlls/comctl32/toolbar.c
@@ -2435,7 +2435,7 @@ TOOLBAR_CustomizeDialogProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
                     btnInfo->btn = nmtb.tbButton;
                     if (!(nmtb.tbButton.fsStyle & BTNS_SEP))
                     {
-                        if (lstrlenW(nmtb.pszText))
+                        if (*nmtb.pszText)
                             lstrcpyW(btnInfo->text, nmtb.pszText);
                         else if (nmtb.tbButton.iString >= 0 && 
                             nmtb.tbButton.iString < infoPtr->nNumStrings)

[Zhiyi Zhang]

On 10/22/18 9:16 AM, Francois Gouget wrote:
> Signed-off-by: Francois Gouget
>
> It seems wasteful to compute the length of the string when all we care 
> about is the first character. Plus the 'if (*str)' pattern is pretty 
> common and used extensively in Wine already.
>
> dlls/comctl32/toolbar.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/dlls/comctl32/toolbar.c b/dlls/comctl32/toolbar.c
> index 6a51ad7fc17..ec3b49890fd 100644
> --- a/dlls/comctl32/toolbar.c
> +++ b/dlls/comctl32/toolbar.c
> @@ -2435,7 +2435,7 @@ TOOLBAR_CustomizeDialogProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
>                      btnInfo->btn = nmtb.tbButton;
>                      if (!(nmtb.tbButton.fsStyle & BTNS_SEP))
>                      {
> -
>                    if (lstrlenW(nmtb.pszText))
> -
>                    if (*nmtb.pszText)]

There is a behavior change with this modification. lstrlenW won't crash even if applications somehow
replaced pszText with null. So we might want to check that pszText is not null before deferencing it.
>                          lstrcpyW(btnInfo->text, nmtb.pszText);
>                      else if (nmtb.tbButton.iString >= 0 && 
>                          nmtb.tbButton.iString < infoPtr->nNumStrings)

[Dmitry Timoshkov]

Zhiyi Zhang  wrote:
> > @@ -2435,7 +2435,7 @@ TOOLBAR_CustomizeDialogProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
> >                      btnInfo->btn = nmtb.tbButton;
> >                      if (!(nmtb.tbButton.fsStyle & BTNS_SEP))
> >                      {
> > -
> >                    if (lstrlenW(nmtb.pszText))
> > -
> >                    if (*nmtb.pszText)]
>
> There is a behavior change with this modification. lstrlenW won't crash even if applications somehow
> replaced pszText with null. So we might want to check that pszText is not null before deferencing it.

Have a look at include/winbase.h: lstr* APIs in Wine code are silently
replaced by inline versions without an exception handler.

[Francois Gouget]

On Mon, 22 Oct 2018, Dmitry Timoshkov wrote:
> Zhiyi Zhang  wrote:
> > > @@ -2435,7 +2435,7 @@ TOOLBAR_CustomizeDialogProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
> > >                      btnInfo->btn = nmtb.tbButton;
> > >                      if (!(nmtb.tbButton.fsStyle & BTNS_SEP))
> > >                      {
> > > -
> > >                    if (lstrlenW(nmtb.pszText))
> > > -
> > >                    if (*nmtb.pszText)]
> >
> > There is a behavior change with this modification. lstrlenW won't crash even if applications somehow
> > replaced pszText with null. So we might want to check that pszText is not null before deferencing it.
>
> Have a look at include/winbase.h: lstr* APIs in Wine code are silently
> replaced by inline versions without an exception handler.

Right. In the patch I forgot to mention that I checked Wine's 
lstrlen{A,W}() implementation in winbase.h but I forgot to check what 
the MSDN says (or the kernel32/string.c implementation). Given the 
inconsistency that's all the more reason to avoid using lstrlen{A,W}() 
in such a case (or at all actually).

In this case nmtb.pszText is set to Buffer which is a WCHAR Buffer[256]. 
So it cannot be NULL and MSDN's lstrlenW() exception handler would not 
be needed anyway.