[PATCH] atl: Fix the ATL_WNDCLASSINFOW::m_szAutoName member definition and construction.
Hermès BÉLUSCA-MAÏTO
hermes.belusca at sfr.fr
Thu Jan 16 11:10:10 CST 2020
Dear Zebediah, please find some answers to the comments.
> -----Original Message-----
> From: wine-devel <wine-devel-bounces at winehq.org> On Behalf Of
> Zebediah Figura
> Sent: 15. siječnja 2020. 23:32
> To: wine-devel at 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
More information about the wine-devel
mailing list