From: "Hermès BÉLUSCA-MAÏTO" Subject: RE: [PATCH] atl: Fix the ATL_WNDCLASSINFOW::m_szAutoName member definition and construction. Message-Id: <003801d5cc8f$cfbf7b80$6f3e7280$@sfr.fr> Date: Thu, 16 Jan 2020 18:10:10 +0100 In-Reply-To: <980bd036-c04a-95c8-0f87-44a2c5d3508b@gmail.com> References: <013e01d5cbef$198c3920$4ca4ab60$@sfr.fr> <980bd036-c04a-95c8-0f87-44a2c5d3508b@gmail.com> Dear Zebediah, please find some answers to the comments. > -----Original Message----- > From: wine-devel On Behalf Of > Zebediah Figura > Sent: 15. siječnja 2020. 23:32 > To: wine-devel@winehq.org > Subject: Re: [PATCH] atl: Fix the ATL_WNDCLASSINFOW::m_szAutoName > member definition and construction. > [...] > > 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. > What do you mean? Is it possible to directly use ' swprintf(wci->m_szAutoName, ARRAY_SIZE(wci->m_szAutoName), L"ATL:%p", wci); ' ? I thought Wine tried not to use this construct since it might not correctly be supported/be defined with correct character size by older versions of GCC? Or has this changed since? [...] > > + 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. > Yes that could be helpful (as part of self-documenting code), but I'm also using this code in another project (ReactOS) where we don't by default support the complete set of C99 features (including the designaters); though that may not be a big deal - I can use conditional compilation in ReactOS, while in the submitted code for Wine I use the designaters. [...] > > + /* 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. > My original idea was that if for whatever reason the string buffer was filled but not NULL-terminated, the next test that calls strlen could trigger a read-beyond-array-boundary problem and so I forced the NULL-termination. After thoughts I think the better would be to combine the check-for-NULL-termination with the strlen thing, and then only if the buffer is NULL-terminated I can do the strlen test, otherwise I fail the test. > > + > > + /* 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. > If you are talking about the comments like " /* Verify its length */ " etc.. then yes I agree and I will remove them. [...] > > 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. > I think at MS the person who wrote that code doesn't completely understand what sizeof() does ^^ Because: - sizeof("ATL:") would return the number of bytes taken by the ansi string (--> will be equal to its number of characters for ansi), including its NULL terminator (and you get a total of 4+1 == 5); - When adding this to the sizeof(void*) * 2 you indeed obtain the maximum possible string character count including the NULL terminator (counted above); AND: - In my version of Windows 7 DDK and the one in MS Visual Studio 2010, the declaration was instead: " WCHAR m_szAutoName[5 + sizeof(void *)*CHAR_BITS]; " (yes!! The CHAR_BITS that is equal to 8 !!), showing that they completely mistook bytes / bits / hexadecimal digits .... Hermes