Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use stricter rule to avoid unnecessary unwrapping of PSObject when operating on a COM object #4614

Merged
merged 10 commits into from Aug 28, 2017
Merged

Use stricter rule to avoid unnecessary unwrapping of PSObject when operating on a COM object #4614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4d48d06
Use more restricted rule in general for DeferForPSObject
daxian-dbw Aug 18, 2017
8e5392a
Use stricter rule only when needed
daxian-dbw Aug 18, 2017
665e439
Add tests and touch up the comments
daxian-dbw Aug 18, 2017
0ef2048
[Feature] Change '%' to 'ForEach-Object'
daxian-dbw Aug 18, 2017
f6ccde0
Address comments
daxian-dbw Aug 22, 2017
2a90178
correct parameter names
daxian-dbw Aug 22, 2017
772b3c8
Fix test -- setup test only if on windows desktop
daxian-dbw Aug 22, 2017
b2482a0
Address another comment
daxian-dbw Aug 24, 2017
027c1d2
Make IsComObject a fater test
daxian-dbw Aug 25, 2017
598e0b4
Remove unnecessary parenthesis
daxian-dbw Aug 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 40 additions & 0 deletions src/System.Management.Automation/engine/Utils.cs
View file
Open in desktop
Expand Up @@ -1465,6 +1465,46 @@ internal static class Separators
// 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
}
}
}

Expand Down
3 changes: 3 additions & 0 deletions src/System.Management.Automation/engine/parser/Compiler.cs
View file
Open in desktop
Expand Up @@ -504,6 +504,9 @@ internal static class CachedReflectionInfo
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 =
Expand Down
117 changes: 75 additions & 42 deletions src/System.Management.Automation/engine/runtime/Binding/Binders.cs
View file
Open in desktop
Expand Up @@ -338,32 +338,72 @@ internal static string ToDebugString(this BindingRestrictions restrictions)

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++)
{
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)
{
var baseValue = PSObject.Base(args[i].Value);
if (baseValue != args[i].Value)
expr = Expression.Call(CachedReflectionInfo.PSObject_Base, arg.Expression.Cast(typeof(object)));

if (argIsComObject)
{
exprs[i] = Expression.Call(CachedReflectionInfo.PSObject_Base,
args[i].Expression.Cast(typeof(object)));
// The 'base' is a COM object, so bake that in the rule.
restrictions = restrictions
.Merge(args[i].GetSimpleTypeRestriction())
.Merge(BindingRestrictions.GetExpressionRestriction(Expression.NotEqual(exprs[i], args[i].Expression)));
.Merge(arg.GetSimpleTypeRestriction())
.Merge(BindingRestrictions.GetExpressionRestriction(Expression.Call(CachedReflectionInfo.Utils_IsComObject, expr)));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here expr is not a PSObject, and IsComObject calls PSObject.BaseObject.

A clean fix is to introduce an overload on IsComObject, one accepting PSObject and one that accepts object, calling the later here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IsComObject method is also used in FallbackConvert, and that target.Expression there could represent either
a COM object or a PSObject wrapping a COM object. Are you suggesting to make the expression restriction a bit complex by changing it to '(target.expression is PSObject AND IsComObject(PSObject target.Expression)) OR (IsComObject(Object target.Expression))'?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to avoid the more complex version of 'expression restriction' for FallbackConvert. We can do one of the following two:

internal static bool IsComObject(object obj)
{
    if (obj is PSObject) { obj = PSObject.Base(obj); }
    return obj != null && ComObjectType.IsAssignableFrom(obj.GetType());
}

OR

internal static bool IsComObject(object obj, bool mayBeWrapped)
{
    if (mayBeWrapped) { obj = PSObject.Base(obj); }
    return obj != null && ComObjectType.IsAssignableFrom(obj.GetType());
}

I vote for the first one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, I vote for neither - I'd leave it or introduce another helper with a different name.

I tend to err on the side of less general code to get a faster test, but it probably doesn't matter much anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I will leave the PR as is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we can have IsComObject(PSObject) and IsComObject(object), and make the 'expression restriction' in FallbackConvert look like this: IsComObject(PSObject.Base(target.Expression)), it makes the restriction a little bit more complex, but not much. I will go with it, let me know what you think.

else
{
exprs[i] = args[i].Expression;
restrictions = restrictions.Merge(args[i].PSGetTypeRestriction());
// 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)));
}
}

return new DynamicMetaObject(DynamicExpression.Dynamic(binder, binder.ReturnType, exprs), restrictions);
else
{
expr = arg.Expression;
restrictions = restrictions.Merge(arg.PSGetTypeRestriction());
}
return expr;
}

internal static DynamicMetaObject UpdateComRestrictionsForPsObject(this DynamicMetaObject binder, DynamicMetaObject[] args)
Expand Down Expand Up @@ -582,7 +622,7 @@ public override DynamicMetaObject FallbackConvert(DynamicMetaObject target, Dyna
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:
Expand All @@ -594,8 +634,8 @@ public override DynamicMetaObject FallbackConvert(DynamicMetaObject target, Dyna
// 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);
}
Expand Down Expand Up @@ -669,25 +709,6 @@ internal static DynamicMetaObject IsEnumerable(DynamicMetaObject target)
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
Expand All @@ -697,7 +718,7 @@ private static IEnumerator AutomationNullRule(CallSite site, object obj)

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;
}
Expand Down Expand Up @@ -4975,9 +4996,13 @@ public override DynamicMetaObject FallbackGetMember(DynamicMetaObject target, Dy
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);
}
}

Expand Down Expand Up @@ -5837,9 +5862,13 @@ public override DynamicMetaObject FallbackSetMember(DynamicMetaObject target, Dy
(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);
}
}

Expand Down Expand Up @@ -6354,9 +6383,13 @@ public override DynamicMetaObject FallbackInvokeMember(DynamicMetaObject target,
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);
}
}

Expand Down
51 changes: 51 additions & 0 deletions test/powershell/engine/COM/COM.Basic.Tests.ps1
View file
Open in desktop
Expand Up @@ -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" {
Expand All @@ -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
}

Expand All @@ -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
}