Improve the handling around the crashing in both VBA and OLE sides by using an extension method

Author: bclothier

Rubberduck.JunkDrawer/Hacks/VarDescExtensions.cs

@@ -0,0 +1,28 @@
+using System.Runtime.InteropServices.ComTypes;
+
+namespace Rubberduck.JunkDrawer.Hacks
+{
+    public static class VarDescExtensions
+    {
+        /// <remarks>
+        /// Use only with VBA-supplied <see cref="ITypeInfo"/> which may return a <see cref="VARDESC"/> that do not conform to 
+        /// the MS-OAUT in describing the constants. See section 2.2.43 at: https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-oaut/ae7791d2-4399-4dff-b7c6-b0d4f3dce982
+        /// 
+        /// To further complicate the situation, on 64-bit platform, the <see cref="VARDESC.DESCUNION.oInst"/> is a 32-bit integer whereas
+        /// the <see cref="VARDESC.DESCUNION.lpvarValue"/> is a pointer. On 32-bit platform, the sizes of 2 members are exactly same so no
+        /// problem. But on 64-bit platform, setting the <c>oInst</c>to 0 does not necessarily zero-initialize the entire region. Thus, the 
+        /// upper 32-bit part of the <c>lpvarValue</c> can contain garbage which will confound the simple null pointer check. Thus to guard 
+        /// against this, we will check the <c>oInst</c> value to see if it's zero. 
+        /// 
+        /// There is a small but non-zero chance that there might be a valid pointer that happens to be only in high half of the address...
+        /// in that case, it'll be wrong but since VBA is always writing <see cref="VARKIND.VAR_STATIC"/> to the <see cref="VARDESC.varkind"/>
+        /// field, we're kind of stuck...
+        /// </remarks>
+        /// <param name="varDesc">The <see cref="VARDESC"/> from a VBA <see cref="ITypeInfo"/></param>
+        /// <returns>True if this is most likely a constant. False when it's definitely not.</returns>
+        public static bool IsValidVBAConstant(this VARDESC varDesc)
+        {
+            return varDesc.varkind == VARKIND.VAR_STATIC && varDesc.desc.oInst != 0;
+        }
+    }
+}

Rubberduck.Parsing/ComReflection/ComModule.cs

@@ -11,6 +11,8 @@
 using TYPEATTR = System.Runtime.InteropServices.ComTypes.TYPEATTR;
 using VARDESC = System.Runtime.InteropServices.ComTypes.VARDESC;
 using CALLCONV = System.Runtime.InteropServices.ComTypes.CALLCONV;
+using Rubberduck.VBEditor.ComManagement.TypeLibs.Abstract;
+using Rubberduck.JunkDrawer.Hacks;
 
 namespace Rubberduck.Parsing.ComReflection
 {
@@ -58,7 +60,15 @@ private void GetComFields(ITypeInfo info, TYPEATTR attrib)
                     info.GetNames(desc.memid, names, names.Length, out int length);
                     Debug.Assert(length == 1);
 
-                    var type = desc.desc.lpvarValue == IntPtr.Zero ? DeclarationType.Variable : DeclarationType.Constant;
+                    DeclarationType type;
+                    if(info is ITypeInfoWrapper wrapped && wrapped.HasVBEExtensions)
+                    {
+                        type = desc.IsValidVBAConstant() ? DeclarationType.Constant : DeclarationType.Variable;
+                    }
+                    else
+                    {
+                        type = desc.varkind == VARKIND.VAR_CONST ? DeclarationType.Constant : DeclarationType.Variable;
+                    }
 
                     _fields.Add(new ComField(this, info, names[0], desc, index, type));
                 }

Rubberduck.VBEEditor/ComManagement/TypeLibs/TypeInfoConstantsCollection.cs

@@ -1,6 +1,6 @@
-using System;
-using System.Collections.Generic;
+using System.Collections.Generic;
 using System.Runtime.InteropServices.ComTypes;
+using Rubberduck.JunkDrawer.Hacks;
 using Rubberduck.VBEditor.ComManagement.TypeLibs.Abstract;
 using Rubberduck.VBEditor.ComManagement.TypeLibs.Unmanaged;
 using TYPEATTR = System.Runtime.InteropServices.ComTypes.TYPEATTR;
@@ -45,33 +45,15 @@ public TypeInfoConstantsCollection(ITypeInfo parent, TYPEATTR attributes) :
                 parent.GetVarDesc(i, out var ppVarDesc);
                 var varDesc = StructHelper.ReadStructureUnsafe<VARDESC>(ppVarDesc);
 
-                // VBA constants are "static".... go figure. We can still infer it is a 
-                // constant rather than a field by checking the lpvarValue
-                if (varDesc.varkind == VARKIND.VAR_STATIC && IsValidPointer(varDesc.desc.lpvarValue))
+                // VBA constants are "static".... go figure.
+                if (varDesc.IsValidVBAConstant())
                 {
                     _mapper.Add(_mapper.Count, i);
                 }
                 parent.ReleaseVarDesc(ppVarDesc);
             }
         }
 
-        /// <remarks>
-        /// On 64-bit platform, VBA seems to like putting random values in the high 32-bit part of the lpvarValue. Sometimes it does give out
-        /// null pointer but when ti does has no apparent reason or rhyme. Thus, to help avoid erroneous resolution of lpvarValue for vars that 
-        /// shouldn't be resolved, we will need to mask the high word and assume that any valid constants will have a address that is also in the low
-        /// 32-bit part of the lpvarValue. That does not seem to exist on 32-bit VBA. Whee! 
-        /// Ref: https://github.com/rubberduck-vba/Rubberduck/issues/5241
-        /// </remarks>
-        private bool IsValidPointer(IntPtr ptr)
-        {
-            if(IntPtr.Size == 8)
-            {
-                return ((ulong)ptr & 0x00000000ffffffff) != 0;
-            }
-
-            return ptr != IntPtr.Zero;
-        }
-
         public override int Count => _mapper.Count;
 
         public override ITypeInfoVariable GetItemByIndex(int index) => 

Rubberduck.VBEEditor/Rubberduck.VBEditor.csproj

@@ -22,6 +22,7 @@
     <Reference Include="System.Windows.Forms" />
   </ItemGroup>
   <ItemGroup>
+    <ProjectReference Include="..\Rubberduck.JunkDrawer\Rubberduck.JunkDrawer.csproj" />
     <ProjectReference Include="..\Rubberduck.Resources\Rubberduck.Resources.csproj" />
   </ItemGroup>
   <ItemGroup>