RE: [PATCH] atl: Fix the ATL_WNDCLASSINFOW::m_szAutoName member definition and construction.

[Hermès BÉLUSCA-MAÏTO]

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: [e-mail]
> 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