Dear Zebediah, please find some answers to the comments.
-----Original Message----- From: wine-devel <wine-devel-bounces(a)winehq.org> On Behalf Of Zebediah Figura Sent: 15. siječnja 2020. 23:32 To: wine-devel(a)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