Re: [PATCH] atl: Fix the ATL_WNDCLASSINFOW::m_szAutoName member definition and construction.

[Zebediah Figura]

Hello Hermès, a few comments.

On 1/15/20 3:59 PM, Hermès BÉLUSCA-MAÏTO wrote:
> From 0e829de8bd04d42fe2dca688ebd1799ee5ffe048 Mon Sep 17 00:00:00 2001
> From: Hermes Belusca-Maito 
> Date: Wed, 15 Jan 2020 02:09:33 +0100
> Subject: [PATCH] atl: Fix the ATL_WNDCLASSINFOW::m_szAutoName member
>  definition and construction.
> -
>
> Document the m_szAutoName size.
> -
>
> Make the format actually MS-compatible: "ATL" followed by colon,
> followed by hexadecimal digits of pointer (4*2 digits on 32-bit platforms,
> 8*2 digits on 64-bit platforms). This correctly fix x64 compilation
> and still make the autogenerated name "unique".
> -
>
> Add a test that checks the name prefix.
> Signed-off-by: Hermes Belusca-Maito
>
> dlls/atl/atl30.c        | 10 +++----
>  dlls/atl/tests/module.c | 59 +++++++++++++++++++++++++++++++++++++++++
>  include/atlwin.h        |  4 +--
>  3 files changed, 66 insertions(+), 7 deletions(-)
>
> diff --git a/dlls/atl/atl30.c b/dlls/atl/atl30.c
> index 29b29d4cbe..ee1b09cdac 100644
> --- a/dlls/atl/atl30.c
> +++ b/dlls/atl/atl30.c
> @@ -313,7 +313,7 @@ ATOM WINAPI AtlModuleRegisterWndClassInfoA(_ATL_MODULEA *pm, _ATL_WNDCLASSINFOA
>      if (!wci->m_wc.lpszClassName)
>      {
> -
>        sprintf(wci->m_szAutoName, "ATL%08x", PtrToUint(wci));
> -
>        sprintf(wci->m_szAutoName, "ATL:%p", wci);
>        TRACE("auto-generated class name %s\n", wci->m_szAutoName);
>        wci->m_wc.lpszClassName = wci->m_szAutoName;
>    }
>
> @@ -350,8 +350,8 @@ ATOM WINAPI AtlModuleRegisterWndClassInfoA(_ATL_MODULEA *pm, _ATL_WNDCLASSINFOA
> - NOTES
> - Can be called multiple times without error, unlike RegisterClassEx().
> -
> -
> - If the class name is NULL, then a class with a name of "ATLxxxxxxxx" is
> -
> - registered, where the 'x's represent a unique value.
> -
> - If the class name is NULL, then a class with a name of "ATL:xxxxxxxx" is
> -
> - registered, where 'xxxxxxxx' represents a unique hexadecimal value.
> -
> */
>
> ATOM WINAPI AtlModuleRegisterWndClassInfoW(_ATL_MODULEW *pm, _ATL_WNDCLASSINFOW *wci, WNDPROC *pProc)
> @@ -372,8 +372,8 @@ ATOM WINAPI AtlModuleRegisterWndClassInfoW(_ATL_MODULEW *pm, _ATL_WNDCLASSINFOW
>      if (!wci->m_wc.lpszClassName)
>      {
> -
>        static const WCHAR szFormat[] = {'A','T','L','%','0','8','x',0};
> -
>        swprintf(wci->m_szAutoName, ARRAY_SIZE(wci->m_szAutoName), szFormat, PtrToUint(wci));
> -
>        static const WCHAR szFormat[] = {'A','T','L',':','%','p',0};
> -
>        swprintf(wci->m_szAutoName, ARRAY_SIZE(wci->m_szAutoName), szFormat, wci);

While you're at it you might as well use a wide string constant here,
since this module is built with msvcrt.
>          TRACE("auto-generated class name %s\n", debugstr_w(wci->m_szAutoName));
>          wci->m_wc.lpszClassName = wci->m_szAutoName;
>      }
>
> diff --git a/dlls/atl/tests/module.c b/dlls/atl/tests/module.c
> index 192b23ef1e..021846ccaf 100644
> --- a/dlls/atl/tests/module.c
> +++ b/dlls/atl/tests/module.c
> @@ -25,6 +25,7 @@
>  #define COBJMACROS
>
> #include <atlbase.h>
> +#include <atlwin.h>
>
> #include <wine/test.h>
>
> @@ -113,6 +114,63 @@ static void test_winmodule(void)
>      ok(winmod.m_pCreateWndList == create_data+1, "winmod.m_pCreateWndList != create_data\n");
>  }
>
> +static void test_winclassinfo(void)
> +{
> - _ATL_MODULEA winmod;
> - HRESULT hres;
> - size_t len, expectedLen;
> - ATOM atom;
> - WNDPROC wndProc;
> - _ATL_WNDCLASSINFOA wci =
> - {
> -
>    /* WNDCLASSEXA m_wc; */
> -
>    {
> -
>        sizeof(WNDCLASSEXA),
> -
>        CS_VREDRAW | CS_HREDRAW,
> -
>        DefWindowProcA,
> -
>        0,
> -
>        0,
> -
>        NULL,
> -
>        NULL,
> -
>        LoadCursorA(NULL, IDC_ARROW),
> -
>        (HBRUSH)(COLOR_BTNFACE + 1),
> -
>        NULL,
> -
>        NULL,   /* LPCSTR lpszClassName; <-- We force ATL class name generation */
> -
>        NULL
> -
>    },
> -
>    NULL,       /* LPCSTR m_lpszOrigName;   */
> -
>    NULL,       /* WNDPROC pWndProc;        */
> -
>    IDC_ARROW,  /* LPCSTR m_lpszCursorID;   */
> -
>    TRUE,       /* BOOL m_bSystemCursor;    */
> -
>    0,          /* ATOM m_atom;             */
> -
>    ""          /* CHAR m_szAutoName[...];  */
> - };

I'm a strong proponent of designated initializers for cases like this.
> -
> - winmod.cbSize = sizeof(winmod);
> - winmod.m_pCreateWndList = (void*)0xdeadbeef;
> - hres = AtlModuleInit(&winmod, NULL, NULL);
> - ok(hres == S_OK, "AtlModuleInit failed: %08x\n", hres);
> - ok(!winmod.m_pCreateWndList, "winmod.m_pCreateWndList = %p\n", winmod.m_pCreateWndList);
> -
> - atom = AtlModuleRegisterWndClassInfoA(&winmod, &wci, &wndProc);
> - ok(atom != NULL, "AtlModuleRegisterWndClassInfoA failed: %08x\n", atom);
> - ok(atom == wci.m_atom, "(atom = %08x) is != than (wci.m_atom = %08x)\n", atom, wci.m_atom);
> -
> - /* Check for the prefix */
> - ok(strncmp(wci.m_szAutoName, "ATL:", 4) == 0, "wci.m_szAutoName = '%s', expected starting with 'ATL:'\n", wci.m_szAutoName);
> -
> - /* Be sure m_szAutoName is NULL-terminated as it should by checking for NULL-terminator at its end - the string in it should always have the same length */
> - ok(wci.m_szAutoName[ARRAY_SIZE(wci.m_szAutoName) - 1] == 0, "wci.m_szAutoName is not NULL-terminated!\n");
> -
> - /* Manually NULL-terminate it in case something bad happened */
> - wci.m_szAutoName[ARRAY_SIZE(wci.m_szAutoName) - 1] = 0;

I don't think there's much point working around failures we don't expect
to happen. If the string is not null-terminated the above test will fail
and we'll know.
> -
> - /* Verify its length */
> - len = strlen(wci.m_szAutoName);
> - expectedLen = sizeof("ATL:") + sizeof(void *) * 2 - 1;
> - ok(len == expectedLen, "wci.m_szAutoName has length %d, expected length %d\n", len, expectedLen);

Do you really need all of these comments? The code seems clear enough.
> +}
> -
>
> static DWORD cb_val;
>
> static void WINAPI term_callback(DWORD dw)
> @@ -156,5 +214,6 @@ START_TEST(module)
>  {
>      test_StructSize();
>      test_winmodule();
> - test_winclassinfo();
> test_term();
>
> }
> diff --git a/include/atlwin.h b/include/atlwin.h
> index 386177ba61..2ad812b201 100644
> --- a/include/atlwin.h
> +++ b/include/atlwin.h
> @@ -29,7 +29,7 @@ typedef struct _ATL_WNDCLASSINFOA_TAG
>      LPCSTR m_lpszCursorID;
>      BOOL m_bSystemCursor;
>      ATOM m_atom;
> - CHAR m_szAutoName[14];
> - CHAR m_szAutoName[sizeof("ATL:") + sizeof(void *) * 2]; // == 4 characters + NULL + number of hexadecimal digits describing a pointer.
>
> } _ATL_WNDCLASSINFOA;
>
> typedef struct _ATL_WNDCLASSINFOW_TAG
> @@ -40,7 +40,7 @@ typedef struct _ATL_WNDCLASSINFOW_TAG
>      LPCWSTR m_lpszCursorID;
>      BOOL m_bSystemCursor;
>      ATOM m_atom;
> - WCHAR m_szAutoName[14];
> - WCHAR m_szAutoName[sizeof("ATL:") + sizeof(void *) * 2]; // == 4 characters + NULL + number of hexadecimal digits describing a pointer.
>
> } _ATL_WNDCLASSINFOW;
>
> ATOM WINAPI AtlModuleRegisterWndClassInfoA(_ATL_MODULEA *pm, _ATL_WNDCLASSINFOA *wci, WNDPROC *pProc);

In my copy of atlwin.h (from the Windows 7 DDK) this is sizeof("ATL:") +
sizeof(void *) * 2 + 1, i.e. one more than you have here.
> -- 
> 2.22.0.windows.1