From: Zebediah Figura Subject: Re: [PATCH] atl: Fix the ATL_WNDCLASSINFOW::m_szAutoName member definition and construction. Message-Id: <293f2b08-657f-ce68-8d0b-d802ba5a3128@gmail.com> Date: Thu, 16 Jan 2020 11:26:48 -0600 In-Reply-To: <003801d5cc8f$cfbf7b80$6f3e7280$@sfr.fr> References: <013e01d5cbef$198c3920$4ca4ab60$@sfr.fr> <980bd036-c04a-95c8-0f87-44a2c5d3508b@gmail.com> <003801d5cc8f$cfbf7b80$6f3e7280$@sfr.fr> On 1/16/20 11:10 AM, Hermès BÉLUSCA-MAÏTO wrote: > 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? Yes, but only in modules compiled with msvcrt. As far as I understand the concern isn't about older versions of GCC, but rather about incompatibility with standard library routines which expect 32-bit wchar_t. > > [...] > >>> + 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. > Personally I wouldn't do either, although others might disagree. In general, if I write an ok() message, I think it makes sense for all following code to implicitly assume the test passed. My reasoning is that if the first test didn't pass, then subsequent failures don't need debugging, as we already know what went wrong, and also that whether a test unit passes is pretty much a binary state—it doesn't largely matter if it fails once or a dozen times or crashes; it needs to be fixed in any case. >>> + >>> + /* 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 .... Sure, it definitely seems wrong. But it seems likely we'd want our structure to be at least as large as Microsoft's, lest further Windows bugs cause memory corruption. Of course, if even they aren't consistent... > > > Hermes > >