From: Zebediah Figura Subject: Re: [PATCH] atl: Fix the ATL_WNDCLASSINFOW::m_szAutoName member definition and construction. Message-Id: <980bd036-c04a-95c8-0f87-44a2c5d3508b@gmail.com> Date: Wed, 15 Jan 2020 16:31:57 -0600 In-Reply-To: <013e01d5cbef$198c3920$4ca4ab60$@sfr.fr> References: <013e01d5cbef$198c3920$4ca4ab60$@sfr.fr> 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 > +#include > > #include > > @@ -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 >