Re: [PATCH v2] combase: Reorder hstring_private elements.

[Nikolay Sivov]

On 12/24/21 15:25, Bernhard wrote:
> Thanks for your review Nikolay,
>
> Nikolay Sivov  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

[PATCH v2] combase: Reorder hstring_private elements.

[Bernhard]

On Sat, Dec 25, 2021 at 1:34 PM Nikolay Sivov  wrote:
> > 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.
> From WinRT the base.h file. On Windows they are installed in

C:\Program Files (x86)\Windows
Kits\10\Include\10.0.19041.0\cppwinrt\winrt