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

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

- Document the m_szAutoName size.

- Make the format actually MS-compatible: "ATL" followed by colon,
  followed by hexadecimal digits of pointer (4*2 digits on 32-bit platforms,
  8*2 digits on 64-bit platforms). This correctly fix x64 compilation
  and still make the autogenerated name "unique".

- Add a test that checks the name prefix.

Signed-off-by: Hermes Belusca-Maito

[Zebediah Figura]

Hello Hermès, a few comments.

On 1/15/20 3:59 PM, Hermès BÉLUSCA-MAÏTO wrote:
> From 0e829de8bd04d42fe2dca688ebd1799ee5ffe048 Mon Sep 17 00:00:00 2001
> From: Hermes Belusca-Maito 
> Date: Wed, 15 Jan 2020 02:09:33 +0100
> Subject: [PATCH] atl: Fix the ATL_WNDCLASSINFOW::m_szAutoName member
>  definition and construction.
> -
>
> Document the m_szAutoName size.
> -
>
> Make the format actually MS-compatible: "ATL" followed by colon,
> followed by hexadecimal digits of pointer (4*2 digits on 32-bit platforms,
> 8*2 digits on 64-bit platforms). This correctly fix x64 compilation
> and still make the autogenerated name "unique".
> -
>
> Add a test that checks the name prefix.
> Signed-off-by: Hermes Belusca-Maito
>
> dlls/atl/atl30.c        | 10 +++----
>  dlls/atl/tests/module.c | 59 +++++++++++++++++++++++++++++++++++++++++
>  include/atlwin.h        |  4 +--
>  3 files changed, 66 insertions(+), 7 deletions(-)
>
> diff --git a/dlls/atl/atl30.c b/dlls/atl/atl30.c
> index 29b29d4cbe..ee1b09cdac 100644
> --- a/dlls/atl/atl30.c
> +++ b/dlls/atl/atl30.c
> @@ -313,7 +313,7 @@ ATOM WINAPI AtlModuleRegisterWndClassInfoA(_ATL_MODULEA *pm, _ATL_WNDCLASSINFOA
>      if (!wci->m_wc.lpszClassName)
>      {
> -
>        sprintf(wci->m_szAutoName, "ATL%08x", PtrToUint(wci));
> -
>        sprintf(wci->m_szAutoName, "ATL:%p", wci);
>        TRACE("auto-generated class name %s\n", wci->m_szAutoName);
>        wci->m_wc.lpszClassName = wci->m_szAutoName;
>    }
>
> @@ -350,8 +350,8 @@ ATOM WINAPI AtlModuleRegisterWndClassInfoA(_ATL_MODULEA *pm, _ATL_WNDCLASSINFOA
> - NOTES
> - Can be called multiple times without error, unlike RegisterClassEx().
> -
> -
> - If the class name is NULL, then a class with a name of "ATLxxxxxxxx" is
> -
> - registered, where the 'x's represent a unique value.
> -
> - If the class name is NULL, then a class with a name of "ATL:xxxxxxxx" is
> -
> - registered, where 'xxxxxxxx' represents a unique hexadecimal value.
> -
> */
>
> ATOM WINAPI AtlModuleRegisterWndClassInfoW(_ATL_MODULEW *pm, _ATL_WNDCLASSINFOW *wci, WNDPROC *pProc)
> @@ -372,8 +372,8 @@ ATOM WINAPI AtlModuleRegisterWndClassInfoW(_ATL_MODULEW *pm, _ATL_WNDCLASSINFOW
>      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.
>          TRACE("auto-generated class name %s\n", debugstr_w(wci->m_szAutoName));
>          wci->m_wc.lpszClassName = wci->m_szAutoName;
>      }
>
> diff --git a/dlls/atl/tests/module.c b/dlls/atl/tests/module.c
> index 192b23ef1e..021846ccaf 100644
> --- a/dlls/atl/tests/module.c
> +++ b/dlls/atl/tests/module.c
> @@ -25,6 +25,7 @@
>  #define COBJMACROS
>
> #include <atlbase.h>
> +#include <atlwin.h>
>
> #include <wine/test.h>
>
> @@ -113,6 +114,63 @@ static void test_winmodule(void)
>      ok(winmod.m_pCreateWndList == create_data+1, "winmod.m_pCreateWndList != create_data\n");
>  }
>
> +static void test_winclassinfo(void)
> +{
> - _ATL_MODULEA winmod;
> - HRESULT hres;
> - size_t len, expectedLen;
> - ATOM atom;
> - WNDPROC wndProc;
> - _ATL_WNDCLASSINFOA wci =
> - {
> -
>    /* WNDCLASSEXA m_wc; */
> -
>    {
> -
>        sizeof(WNDCLASSEXA),
> -
>        CS_VREDRAW | CS_HREDRAW,
> -
>        DefWindowProcA,
> -
>        0,
> -
>        0,
> -
>        NULL,
> -
>        NULL,
> -
>        LoadCursorA(NULL, IDC_ARROW),
> -
>        (HBRUSH)(COLOR_BTNFACE + 1),
> -
>        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.
> -
> - winmod.cbSize = sizeof(winmod);
> - winmod.m_pCreateWndList = (void*)0xdeadbeef;
> - hres = AtlModuleInit(&winmod, NULL, NULL);
> - ok(hres == S_OK, "AtlModuleInit failed: %08x\n", hres);
> - ok(!winmod.m_pCreateWndList, "winmod.m_pCreateWndList = %p\n", winmod.m_pCreateWndList);
> -
> - atom = AtlModuleRegisterWndClassInfoA(&winmod, &wci, &wndProc);
> - ok(atom != NULL, "AtlModuleRegisterWndClassInfoA failed: %08x\n", atom);
> - ok(atom == wci.m_atom, "(atom = %08x) is != than (wci.m_atom = %08x)\n", atom, wci.m_atom);
> -
> - /* Check for the prefix */
> - ok(strncmp(wci.m_szAutoName, "ATL:", 4) == 0, "wci.m_szAutoName = '%s', expected starting with 'ATL:'\n", wci.m_szAutoName);
> -
> - /* 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.
> -
> - /* 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.
> +}
> -
>
> static DWORD cb_val;
>
> static void WINAPI term_callback(DWORD dw)
> @@ -156,5 +214,6 @@ START_TEST(module)
>  {
>      test_StructSize();
>      test_winmodule();
> - test_winclassinfo();
> test_term();
>
> }
> diff --git a/include/atlwin.h b/include/atlwin.h
> index 386177ba61..2ad812b201 100644
> --- a/include/atlwin.h
> +++ b/include/atlwin.h
> @@ -29,7 +29,7 @@ typedef struct _ATL_WNDCLASSINFOA_TAG
>      LPCSTR m_lpszCursorID;
>      BOOL m_bSystemCursor;
>      ATOM m_atom;
> - CHAR m_szAutoName[14];
> - CHAR m_szAutoName[sizeof("ATL:") + sizeof(void *) * 2]; // == 4 characters + NULL + number of hexadecimal digits describing a pointer.
>
> } _ATL_WNDCLASSINFOA;
>
> 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.
> -- 
> 2.22.0.windows.1

[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

[Zebediah Figura]

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: [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?

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