[PATCH v2] combase: Reorder hstring_private elements.

[Nikolay Sivov]

On 12/24/21 15:25, Bernhard wrote:
> Thanks for your review Nikolay,
>
> Nikolay Sivov <nsivov at codeweavers.com> schrieb am Fr., 24. Dez. 2021, 
> 09:03:
>
>
>
>     On 12/24/21 01:48, Bernhard Kölbl wrote:
>     > +#define HSTRING_REFERENCE_FLAG 1
>     > +
>     >   struct hstring_private
>     >   {
>     > -    LPWSTR buffer;
>     > +    UINT32 flags;
>     >       UINT32 length;
>     > -    BOOL   reference;
>     > -    LONG   refcount;
>     > +    LONG refcount;
>     > +    LPWSTR ptr;
>     >   };
>      From what I can tell this still doesn't match native layout. It also
>     might be slightly different for references.
>
>
> Yes it actually doesn't match but I don't know if it really matters in 
> this case, because HSTRINGs seem to be passed with this data only. 
> E.g. in the provided bug, the RoGetActivationFactory function takes a 
> HSTRING as parameter, but WinRT actually passes a HSTRING_HEADER.
> The first field in the HSTRING struct seems to only point at this 
> header struct. WinRT dereferences this pointer when passing the String.

I don't follow. What do you mean by "WinRT passing a header" and "WinRT 
dereferencing this pointer" ? Where is this code?

>
>
>     >   static BOOL alloc_string(UINT32 len, HSTRING *out)
>     >   {
>     >       struct hstring_private *priv;
>     > -    priv = HeapAlloc(GetProcessHeap(), 0, sizeof(*priv) + (len
>     + 1) * sizeof(*priv->buffer));
>     > +    priv = HeapAlloc(GetProcessHeap(), 0, sizeof(*priv));
>     >       if (!priv)
>     >           return FALSE;
>     > -    priv->buffer = (LPWSTR)(priv + 1);
>     > +    priv->ptr = (LPWSTR)HeapAlloc(GetProcessHeap(), 0, (len +
>     1) * sizeof(*priv->ptr));
>     I don't see this behaviour on Windows.
>
>
> Could explain this one a bit more? (I'm not much into how Windows 
> libraries allocate memory.) Maybe I'm overseeing a obvious mistake.

I don't think it's justified to have separate allocation for string buffer.

>
>
>     But anyway, do you have any idea why application would care? Is it
>     some
>     statically linked code depending on it, or some native module
>     bundled or
>     installed separately?
>
>
> Well, the programs (also WinRT) from the Wine-bug suffer from the same 
> issue, which to me looks like is caused by the misarranged struct, 
> which I changed in this patch. They try to dereference the flags + 
> length field, which causes a crash or invalid data to be read.
> I actually don't know why it worked in Wine before. Maybe by luck?
> (Also: The code that arranges the HSTRING struct members in memory 
> this way, is compiled into the binary from the WinRT headers)

Which headers in particular? I'd like to read through that, maybe we can 
easily match it.

>
>
>     If the layout is stable across Windows releases, we'll need tests
>     for it.
>
>
> I think test are the best solution for our open questions.
>
> Happy holidays
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20211225/fa663e65/attachment.htm>