Use stricter rule to unwrap a PSObject that wraps a COM object (#4614)

GetMember/SetMember/InvokeMember operations on a COM object will generate code to unwrap the COM object if it's wrapped in PSObject. Due to a loose restriction, if you have a string wrapped to a PSObject with an ETS member of the same name, then the string PSObject will be unwrapped too and the ETS member will be lost. The fix is to use a more restricted rule by checking if the base object is a COM object.

src/System.Management.Automation/engine/Utils.cs

@@ -1465,6 +1465,46 @@ namespace System.Management.Automation
             // String.WhitespaceChars will trim aggressively than what the underlying FS does (for ex, NTFS, FAT).
             internal static readonly char[] PathSearchTrimEnd = { (char)0x9, (char)0xA, (char)0xB, (char)0xC, (char)0xD, (char)0x20, (char)0x85, (char)0xA0 };
         }
+
+#if !UNIX
+        // This is to reduce the runtime overhead of the feature query
+        private static readonly Type ComObjectType = typeof(object).Assembly.GetType("System.__ComObject");
+#endif
+
+        internal static bool IsComObject(PSObject psObject)
+        {
+#if UNIX
+            return false;
+#else
+            if (psObject == null) { return false; }
+
+            object obj = PSObject.Base(psObject);
+            return IsComObject(obj);
+#endif
+        }
+
+        internal static bool IsComObject(object obj)
+        {
+#if UNIX
+            return false;
+#else
+            // We can't use System.Runtime.InteropServices.Marshal.IsComObject(obj) since it doesn't work in partial trust.
+            //
+            // There could be strongly typed RWCs whose type is not 'System.__ComObject', but the more specific type should
+            // derive from 'System.__ComObject'. The strongly typed RWCs can be created with 'new' operation via the Primay
+            // Interop Assembly (PIA).
+            // For example, with the PIA 'Microsoft.Office.Interop.Excel', you can write the following code:
+            //    var excelApp = new Microsoft.Office.Interop.Excel.Application();
+            //    Type type = excelApp.GetType();
+            //    Type comObjectType = typeof(object).Assembly.GetType("System.__ComObject");
+            //    Console.WriteLine("excelApp type: {0}", type.FullName);
+            //    Console.WriteLine("Is __ComObject assignable from? {0}", comObjectType.IsAssignableFrom(type));
+            // and the results are:
+            //    excelApp type: Microsoft.Office.Interop.Excel.ApplicationClass
+            //    Is __ComObject assignable from? True
+            return obj != null && ComObjectType.IsAssignableFrom(obj.GetType());
+#endif
+        }
     }
 }
 

src/System.Management.Automation/engine/parser/Compiler.cs

@@ -504,6 +504,9 @@ namespace System.Management.Automation.Language
         internal static readonly MethodInfo VariableOps_SetVariableValue =
             typeof(VariableOps).GetMethod(nameof(VariableOps.SetVariableValue), staticFlags);
 
+        internal static readonly MethodInfo Utils_IsComObject =
+            typeof(Utils).GetMethod(nameof(Utils.IsComObject), staticFlags, binder: null, types: new Type[] {typeof(object)}, modifiers: null);
+
         internal static readonly MethodInfo ClassOps_ValidateSetProperty =
             typeof(ClassOps).GetMethod(nameof(ClassOps.ValidateSetProperty), staticPublicFlags);
         internal static readonly MethodInfo ClassOps_CallBaseCtor =

src/System.Management.Automation/engine/runtime/Binding/Binders.cs

@@ -338,34 +338,74 @@ namespace System.Management.Automation.Language
 
     internal static class DynamicMetaObjectBinderExtensions
     {
-        internal static DynamicMetaObject DeferForPSObject(this DynamicMetaObjectBinder binder,
-                                                           params DynamicMetaObject[] args)
+        internal static DynamicMetaObject DeferForPSObject(this DynamicMetaObjectBinder binder, DynamicMetaObject target, bool targetIsComObject = false)
         {
+            Diagnostics.Assert(target.Value is PSObject, "target must be a psobject");
+
+            BindingRestrictions restrictions = BindingRestrictions.Empty;
+            Expression expr = ProcessOnePSObject(target, ref restrictions, argIsComObject: targetIsComObject);
+            return new DynamicMetaObject(DynamicExpression.Dynamic(binder, binder.ReturnType, expr), restrictions);
+        }
+
+        internal static DynamicMetaObject DeferForPSObject(this DynamicMetaObjectBinder binder, DynamicMetaObject target, DynamicMetaObject arg, bool targetIsComObject = false)
+        {
+            Diagnostics.Assert(target.Value is PSObject || arg.Value is PSObject, "At least one arg must be a psobject");
+
+            BindingRestrictions restrictions = BindingRestrictions.Empty;
+            Expression expr1 = ProcessOnePSObject(target, ref restrictions, argIsComObject: targetIsComObject);
+            Expression expr2 = ProcessOnePSObject(arg, ref restrictions, argIsComObject: false);
+            return new DynamicMetaObject(DynamicExpression.Dynamic(binder, binder.ReturnType, expr1, expr2), restrictions);
+        }
+
+        internal static DynamicMetaObject DeferForPSObject(this DynamicMetaObjectBinder binder, DynamicMetaObject[] args, bool targetIsComObject = false)
+        {
+            Diagnostics.Assert(args != null && args.Length > 0, "args should not be null or empty");
             Diagnostics.Assert(args.Any(mo => mo.Value is PSObject), "At least one arg must be a psobject");
 
             Expression[] exprs = new Expression[args.Length];
             BindingRestrictions restrictions = BindingRestrictions.Empty;
-            for (int i = 0; i < args.Length; i++)
+
+            // Target maps to arg[0] of the binder.
+            exprs[0] = ProcessOnePSObject(args[0], ref restrictions, targetIsComObject);
+            for (int i = 1; i < args.Length; i++)
             {
-                var baseValue = PSObject.Base(args[i].Value);
-                if (baseValue != args[i].Value)
-                {
-                    exprs[i] = Expression.Call(CachedReflectionInfo.PSObject_Base,
-                                               args[i].Expression.Cast(typeof(object)));
-                    restrictions = restrictions
-                        .Merge(args[i].GetSimpleTypeRestriction())
-                        .Merge(BindingRestrictions.GetExpressionRestriction(Expression.NotEqual(exprs[i], args[i].Expression)));
-                }
-                else
-                {
-                    exprs[i] = args[i].Expression;
-                    restrictions = restrictions.Merge(args[i].PSGetTypeRestriction());
-                }
+                exprs[i] = ProcessOnePSObject(args[i], ref restrictions, argIsComObject: false);
             }
 
             return new DynamicMetaObject(DynamicExpression.Dynamic(binder, binder.ReturnType, exprs), restrictions);
         }
 
+        private static Expression ProcessOnePSObject(DynamicMetaObject arg, ref BindingRestrictions restrictions, bool argIsComObject = false)
+        {
+            Expression expr = null;
+            object baseValue = PSObject.Base(arg.Value);
+            if (baseValue != arg.Value)
+            {
+                expr = Expression.Call(CachedReflectionInfo.PSObject_Base, arg.Expression.Cast(typeof(object)));
+
+                if (argIsComObject)
+                {
+                    // The 'base' is a COM object, so bake that in the rule.
+                    restrictions = restrictions
+                        .Merge(arg.GetSimpleTypeRestriction())
+                        .Merge(BindingRestrictions.GetExpressionRestriction(Expression.Call(CachedReflectionInfo.Utils_IsComObject, expr)));
+                }
+                else
+                {
+                    // Use a more general condition for the rule: 'arg' is a PSObject and 'base != arg'.
+                    restrictions = restrictions
+                        .Merge(arg.GetSimpleTypeRestriction())
+                        .Merge(BindingRestrictions.GetExpressionRestriction(Expression.NotEqual(expr, arg.Expression)));
+                }
+            }
+            else
+            {
+                expr = arg.Expression;
+                restrictions = restrictions.Merge(arg.PSGetTypeRestriction());
+            }
+            return expr;
+        }
+
         internal static DynamicMetaObject UpdateComRestrictionsForPsObject(this DynamicMetaObject binder, DynamicMetaObject[] args)
         {
             // Add a restriction that prevents PSObject arguments (so that they get based)
@@ -582,7 +622,7 @@ namespace System.Management.Automation.Language
                     GetRestrictions(target))).WriteToDebugLog(this);
             }
 
-            if (IsComObject(targetValue))
+            if (Utils.IsComObject(targetValue))
             {
                 // Pretend that all com objects are enumerable, even if they aren't.  We do this because it's technically impossible
                 // to know if a com object is enumerable without just trying to cast it to IEnumerable.  We could generate a rule like:
@@ -594,8 +634,8 @@ namespace System.Management.Automation.Language
                 // EnumerableOps.NonEnumerableObjectEnumerator for more comments on how this works.
 
                 var bindingRestrictions = BindingRestrictions.GetExpressionRestriction(
-                    Expression.Call(typeof(PSEnumerableBinder).GetMethod("IsComObject", BindingFlags.Static | BindingFlags.NonPublic),
-                                    target.Expression));
+                    Expression.Call(CachedReflectionInfo.Utils_IsComObject,
+                                    Expression.Call(CachedReflectionInfo.PSObject_Base, target.Expression)));
                 return new DynamicMetaObject(
                     Expression.Call(CachedReflectionInfo.EnumerableOps_GetCOMEnumerator, target.Expression), bindingRestrictions).WriteToDebugLog(this);
             }
@@ -669,25 +709,6 @@ namespace System.Management.Automation.Language
             return (result == DynamicMetaObjectExtensions.FakeError) ? null : result;
         }
 
-        // This is to reduce the runtime overhead of the feature query
-        private static readonly TypeInfo s_comObjectTypeInfo = GetComObjectType();
-
-        private static TypeInfo GetComObjectType()
-        {
-#if UNIX
-            return null;
-#else
-            return typeof(object).GetTypeInfo().Assembly.GetType("System.__ComObject").GetTypeInfo();
-#endif
-        }
-
-        internal static bool IsComObject(object obj)
-        {
-            // we can't use System.Runtime.InteropServices.Marshal.IsComObject(obj) since it doesn't work in partial trust
-            obj = PSObject.Base(obj);
-            return obj != null && s_comObjectTypeInfo != null && s_comObjectTypeInfo.IsAssignableFrom(obj.GetType().GetTypeInfo());
-        }
-
         private static IEnumerator AutomationNullRule(CallSite site, object obj)
         {
             return obj == AutomationNull.Value
@@ -697,7 +718,7 @@ namespace System.Management.Automation.Language
 
         private static IEnumerator NotEnumerableRule(CallSite site, object obj)
         {
-            if (!(obj is PSObject) && !(obj is IEnumerable) && !(obj is IEnumerator) && !(obj is DataTable) && !IsComObject(obj))
+            if (!(obj is PSObject) && !(obj is IEnumerable) && !(obj is IEnumerator) && !(obj is DataTable) && !Utils.IsComObject(obj))
             {
                 return null;
             }
@@ -4975,9 +4996,13 @@ namespace System.Management.Automation.Language
             if (target.Value is PSObject && (PSObject.Base(target.Value) != target.Value))
             {
                 Object baseObject = PSObject.Base(target.Value);
-                if ((baseObject != null) && (baseObject.GetType().FullName.Equals("System.__ComObject")))
+                if (baseObject != null && Utils.IsComObject(baseObject))
                 {
-                    return this.DeferForPSObject(target).WriteToDebugLog(this);
+                    // We unwrap only if the 'base' is a COM object. It's unnecessary to unwrap in other cases,
+                    // especially in the case of strings, we would lose instance members on the PSObject.
+                    // Therefore, we need to use a stricter restriction to make sure PSObject 'target' with other
+                    // base types doesn't get unwrapped.
+                    return this.DeferForPSObject(target, targetIsComObject: true).WriteToDebugLog(this);
                 }
             }
 
@@ -5837,9 +5862,13 @@ namespace System.Management.Automation.Language
                 (value.Value is PSObject && (PSObject.Base(value.Value) != value.Value)))
             {
                 Object baseObject = PSObject.Base(target.Value);
-                if ((baseObject != null) && (baseObject.GetType().FullName.Equals("System.__ComObject")))
+                if (baseObject != null && Utils.IsComObject(baseObject))
                 {
-                    return this.DeferForPSObject(target, value).WriteToDebugLog(this);
+                    // We unwrap only if the 'base' of 'target' is a COM object. It's unnecessary to unwrap in other cases,
+                    // especially in the case that 'target' is a string, we would lose instance members on the PSObject.
+                    // Therefore, we need to use a stricter restriction to make sure PSObject 'target' with other base types
+                    // doesn't get unwrapped.
+                    return this.DeferForPSObject(target, value, targetIsComObject: true).WriteToDebugLog(this);
                 }
             }
 
@@ -6354,9 +6383,13 @@ namespace System.Management.Automation.Language
                 args.Any(mo => mo.Value is PSObject && (PSObject.Base(mo.Value) != mo.Value)))
             {
                 Object baseObject = PSObject.Base(target.Value);
-                if ((baseObject != null) && (baseObject.GetType().FullName.Equals("System.__ComObject")))
+                if (baseObject != null && Utils.IsComObject(baseObject))
                 {
-                    return this.DeferForPSObject(args.Prepend(target).ToArray()).WriteToDebugLog(this);
+                    // We unwrap only if the 'base' of 'target' is a COM object. It's unnecessary to unwrap in other cases,
+                    // especially in the case that 'target' is a string, we would lose instance members on the PSObject.
+                    // Therefore, we need to use a stricter restriction to make sure other type of PSObject 'target'
+                    // doesn't get unwrapped.
+                    return this.DeferForPSObject(args.Prepend(target).ToArray(), targetIsComObject: true).WriteToDebugLog(this);
                 }
             }
 

test/powershell/engine/COM/COM.Basic.Tests.ps1

@@ -16,7 +16,11 @@ try {
             $items = $folder.Items()
 
             ## $items is a collection of all items belong to the folder, and it should be enumerated.
+            $items.Count | Should Be 3
             $items | Measure-Object | ForEach-Object Count | Should Be $items.Count
+
+            $names = $items | ForEach-Object { $_.Name }
+            $names -join "," | Should Be "file1,file2,file3"
         }
 
         It "Should enumerate IEnumVariant interface object without exception" {
@@ -26,6 +30,7 @@ try {
 
             ## $enumVariant is an IEnumVariant interface of all items belong to the folder, and it should be enumerated.
             $enumVariant = $items._NewEnum()
+            $items.Count | Should Be 3
             $enumVariant | Measure-Object | ForEach-Object Count | Should Be $items.Count
         }
 
@@ -43,6 +48,52 @@ try {
         }
     }
 
+    Describe 'GetMember/SetMember/InvokeMember binders should have more restricted rule for COM object' -Tags "CI" {
+        BeforeAll {
+            if ([System.Management.Automation.Platform]::IsWindowsDesktop) {
+                $null = New-Item -Path $TESTDRIVE/bar -ItemType Directory -Force
+
+                $shell = New-Object -ComObject "Shell.Application"
+                $folder = $shell.Namespace("$TESTDRIVE")
+                $item = $folder.Items().Item(0)
+                $item = [psobject]::AsPSObject($item)
+
+                ## Create a PSObject that has an instance member 'Name' and a script method 'Windows'
+                $str = Add-Member -InputObject "abc" -MemberType NoteProperty -Name Name -Value "Hello" -PassThru
+                $str = Add-Member -InputObject $str -MemberType ScriptMethod -Name Windows -Value { "Windows" } -PassThru
+            }
+        }
+
+        It "GetMember binder should differentiate PSObject that wraps COM object from other PSObjects" {
+            ## GetMember on the member name 'Name'.
+            $entry1 = ($item, "bar")
+            $entry2 = ($str, "Hello")
+
+            foreach ($pair in ($entry1, $entry2, $entry2, $entry1, $entry1, $entry2)) {
+                $pair[0].Name | Should Be $pair[1]
+            }
+        }
+
+        It "SetMember binder should differentiate PSObject that wraps COM object from other PSObjects" {
+            ## SetMember on the member name 'Name'
+            $entry1 = ($item, "foo")
+            $entry2 = ($str, "World")
+
+            foreach ($pair in ($entry1, $entry2)) {
+                $pair[0].Name = $pair[1]
+                $pair[0].Name | Should Be $pair[1]
+            }
+        }
+
+        It "InvokeMember binder should differentiate PSObject that wraps COM object from other PSObjects" {
+            ## InvokeMember on the member name 'Windows'
+            $shell | ForEach-Object { $_.Windows() } > $null
+
+            ## '$str' is a PSObject that wraps a string, but with ScriptMethod 'Windows'
+            $str.Windows() | Should Be "Windows"
+        }
+    }
+
 } finally {
     $global:PSdefaultParameterValues = $defaultParamValues
 }