Rubberduck: Parsing user COM projects crashes with custom types containing custom classes

[bclothier]

Originally reported in #5217 -- though this issue was symptomically fixed by restoring the conditional flags, we still need to know why it crashed and there was difficulty in creating a MVCE.

With gratitude to @SteveLaycock for going above and beyond in helping in creating a reproduction, I can verify that this given code will cause an access violation:

 Private Type foo
    bar As Class1
End Type

Private x As foo 

The crashing happens when we try to resolve the x type as a ComField:
https://github.com/rubberduck-vba/Rubberduck/blob/756763be946735c4f7bc804db1f902d730485c1c/Rubberduck.Parsing/ComReflection/ComField.cs#L54-L66

The problems emerges:

1) The type is Constant which is incorrect, since x is a field, and thus should be a variable, not a constant. This happens because we have this test:
https://github.com/rubberduck-vba/Rubberduck/blob/d53d42e1d508305d75704cb8ca688493179146fc/Rubberduck.Parsing/ComReflection/ComModule.cs#L61

2) The varDesc.desc.lpvarValue has a value of 0x0000000200000000 which makes it _seems_ non-null pointer but this is always the same and seems to be an invalid address.
3) It should be noted that the VBA does not conform to the MS-OAUT. It will describe this type as VAR_STATIC which is in violation of 2.2.43.
4) This simply do not happen on 32-bit Office applications. In the same codebase, the cVars returns 0 and thus we don't get any fields to create.

[bclothier]

Does anyone know how the union will behave in C#? According to the MS-OAUT, the union will be a different size between 32-bit and 64-bit platforms because of the union between a ULONG Windows data type which is 32-bit integer and the * VARIANT which is a pointer and thus scales with the platform:

 typedef struct tagVARDESC {
   MEMBERID memid;
   LPOLESTR lpstrReserved;
   [switch_type(VARKIND), switch_is(varkind)] 
     union {
     [case(VAR_PERINSTANCE, VAR_DISPATCH, VAR_STATIC)] 
       ULONG oInst;
     [case(VAR_CONST)] 
       VARIANT* lpvarValue;
   } _vdUnion;
   ELEMDESC elemdescVar;
   WORD wVarFlags;
   VARKIND varkind;
 } VARDESC,
  *LPVARDESC;

From reference source:

    [StructLayout(LayoutKind.Sequential, CharSet=CharSet.Unicode)]

    public struct VARDESC
    {
        public int memid;                   
        public String lpstrSchema;

        [System.Runtime.InteropServices.StructLayout(LayoutKind.Explicit, CharSet=CharSet.Unicode)]

        public struct DESCUNION
        {
            [FieldOffset(0)]
            public int oInst;
            [FieldOffset(0)]
            public IntPtr lpvarValue;
        };

        public DESCUNION desc;

        public ELEMDESC elemdescVar;
        public short wVarFlags;
        public VARKIND varkind;
    }

The fact that we get 0x0000000200000000 makes me wonder if this is coming from somewhere, but I cannot see anything obvious that would explain why we have 0x00000002. The oInst is zero (correct), and the following member elemdescVar contains nothing similar to the value (in fact it's all zeroes)

[bclothier]

Addendum: I can confirm that even though the attrib.cVars is 0 on 32-bit, the ITypeInfoWrapper.Vars does indicate there is one so we do have that information about the x field but it's getting skipped over because cVars is reported to be zero. Furthermore, the VARDESC for the x seems to be all in the order; the lpvarValue is zero and all other members seems similar to what I'd see on the 64-bit platform.

So, it's simply not happening on 32-bit only because we aren't hitting that region of code, which obviously don't work on 64-bit because of non-null pointer due to .... something.

[bclothier]

Ok, the mystery of why cVars don't concide on 32-bit/64-bit platform is resolved:

https://github.com/rubberduck-vba/Rubberduck/blob/f606675a9ee8e06474d2b41efcd175882440b812/Rubberduck.VBEEditor/ComManagement/TypeLibs/TypeInfoConstantsCollection.cs#L50

https://github.com/rubberduck-vba/Rubberduck/blob/f606675a9ee8e06474d2b41efcd175882440b812/Rubberduck.VBEEditor/ComManagement/TypeLibs/TypeInfoWrapper.cs#L416-L424

Therefore, it's all down to the fact that on 64-bit platform, the lpvarValue has some non-zero value and that is throwing everything off.

[bclothier]

This is worse than I thought.

It seems that lpvarValue may change value when we read it. Sometime it does actually provide a null pointer but other times it provides a random value. However it is consistently in the high-order half of the value. Since I don't have anything better, the solution seems to be apply a bitmask on the high-order half for 64-bit Office.

[WaynePhillipsEA]

The problem looks to me to be in RDs code....

https://github.com/rubberduck-vba/Rubberduck/blob/d53d42e1d508305d75704cb8ca688493179146fc/Rubberduck.Parsing/ComReflection/ComModule.cs#L61

It should be checking the varkind for VAR_CONST, not checking the lpvarValue pointer against IntPtr.Zero.

When the VARDESC structure gets created (by OLE or VBA or whatever), only oInst or lparValue will be set. On 32-bit the lparValue and oInst overlap and thus occupy the same memory area, and so writing to one fully affects the other. On 64-bit they still overlap, but as oInst is only 32-bit, when the provider sets the oInst value, only the lower half of the 64-bit lparValue gets set and the upper half is uninitialized and might be whatever value was in the uninitialized memory block beforehand. Remember that in C, memory allocations are not automatically zero-initialised.

In other words, on 64-bit, when the varkind is not VAR_CONST, the upper 32-bits of the lparValue are undefined (uninitialized), and effectively it's just padding.

[bclothier]

Thanks for the technical explanation on why the random values in 64 bit. That reassures me that the fix was correct.

Yes, you would think we would get VAR_CONST for constants. Unfortunately, as documented in the code, it is not the case. VBA simply call all constants and types VAR_STATIC, thus the workaround above.

[WaynePhillipsEA]

Ah yes, I vaguely remember that.

Your fix on the VBA side does not address the ComModule incorrect lpvarValue pointer check. The OLE provided versions of VARDESC might be zero-initialized... but who knows if you're just being lucky.

[bclothier]

Just one question - is it conceivable that we would be handed a valid pointer that exists only in the high half of the 64-bit address space?

We could use ReadStructureSafe but that's going slow down things a bit.

[WaynePhillipsEA]

Yes, there's no guarantee the lower 32-bits being zero gives an invalid address.

Using ReadStructureSafe might help, but there's also no guarantee that the full 64-bit address [with an invalid upper half] would not point to _something else_ in memory and thus treating it as a Variant might still cause problems further down the line. For example, you might end up de-referencing an invalid BSTR inside a bad Variant picked up that way, and that could bring down the host.

But if varkind is not being set correctly by VBA... you're a bit stuck.

[WaynePhillipsEA]

I can't remember off the top off my head whether both versions of the VBA ITypeInfo had that same flaw with the varkind member not being set properly. It might be worth checking if the alternative ITypeInfo versions have it set correctly.

[bclothier]

Good idea - I'll check when I have a chance. Thanks!

[bclothier]

In my testing, it does not look like I can use the alternate ITypeInfo - an AV is thrown when I try to read the GetVarDesc off the alternate. Therefore, we don't really have a way of knowing whether a VARDESC is describing a mislabeled constant.

[bclothier]

Hmm. I just found out that there’s a reserved parameter, described in the MS-OAUT:

 HRESULT GetVarDesc(
   [in] UINT index,
   [out] LPVARDESC* ppVarDesc,
   [out] DWORD* pReserved
 );

Does anyone have old documentations for the last parameter and whether it would be helpful?

[WaynePhillipsEA]

That documentation is wrong - the pReserved parameter does not exist in the win SDK version.

With regards to the constants... again I might not be remembering 100%, but I seem to recall that constants come through the typelib API unnamed (with MEMBERID_NIL). Perhaps you could use that to determine whether the var is a constant vs field etc.

[bclothier]

facepalms You're absolutely right - I was so fixated on the whole varkind mess I didn't even think about memberID & names. The names are a bit inconvenient since that requires another API call to obtain the name. However, the MemberID should be within a certain range that you defined for user constants. I can adjust the check for that. I already adjusted the calling code to further check that a given ITypeInfo is a VBA's by checking the HasVBAExtensions as I am assume that problem only exists with VBA's and won't give non-VBA type infos any special treatment.