[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