Move .NET method invocation logging to after the needed type conversion is done for method arguments (PowerShell#25022)

Author: daxian-dbw

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

@@ -6943,12 +6943,6 @@ internal static DynamicMetaObject InvokeDotNetMethod(
                     expr = Expression.Block(expr, ExpressionCache.AutomationNullConstant);
                 }
 
-                // Expression block runs two expressions in order:
-                //  - Log method invocation to AMSI Notifications (can throw PSSecurityException)
-                //  - Invoke method
-                string targetName = methodInfo.ReflectedType?.FullName ?? string.Empty;
-                expr = MaybeAddMemberInvocationLogging(expr, targetName, name, args);
-
                 // If we're calling SteppablePipeline.{Begin|Process|End}, we don't want
                 // to wrap exceptions - this is very much a special case to help error
                 // propagation and ensure errors are attributed to the correct code (the
@@ -7111,6 +7105,7 @@ internal static Expression InvokeMethod(MethodBase mi, DynamicMetaObject target,
                 invocationType != MethodInvocationType.NonVirtual;
             var parameters = mi.GetParameters();
             var argExprs = new Expression[parameters.Length];
+            var argsToLog = new List<Expression>(Math.Max(parameters.Length, args.Length));
 
             for (int i = 0; i < parameters.Length; ++i)
             {
@@ -7135,16 +7130,21 @@ internal static Expression InvokeMethod(MethodBase mi, DynamicMetaObject target,
 
                     if (expandParameters)
                     {
-                        argExprs[i] = Expression.NewArrayInit(
-                            paramElementType,
-                            args.Skip(i).Select(
-                                a => a.CastOrConvertMethodArgument(
+                        IEnumerable<Expression> elements = args
+                            .Skip(i)
+                            .Select(a =>
+                                a.CastOrConvertMethodArgument(
                                     paramElementType,
                                     paramName,
                                     mi.Name,
                                     allowCastingToByRefLikeType: false,
                                     temps,
-                                    initTemps)));
+                                    initTemps))
+                            .ToList();
+
+                        argExprs[i] = Expression.NewArrayInit(paramElementType, elements);
+                        // User specified the element arguments, so we log them instead of the compiler-created array.
+                        argsToLog.AddRange(elements);
                     }
                     else
                     {
@@ -7155,13 +7155,18 @@ internal static Expression InvokeMethod(MethodBase mi, DynamicMetaObject target,
                             allowCastingToByRefLikeType: false,
                             temps,
                             initTemps);
+
                         argExprs[i] = arg;
+                        argsToLog.Add(arg);
                     }
                 }
                 else if (i >= args.Length)
                 {
-                    Diagnostics.Assert(parameters[i].IsOptional,
+                    // We don't log the default value for an optional parameter, as it's not specified by the user.
+                    Diagnostics.Assert(
+                        parameters[i].IsOptional,
                         "if there are too few arguments, FindBestMethod should only succeed if parameters are optional");
+
                     var argValue = parameters[i].DefaultValue;
                     if (argValue == null)
                     {
@@ -7199,17 +7204,25 @@ internal static Expression InvokeMethod(MethodBase mi, DynamicMetaObject target,
                         var psRefValue = Expression.Property(args[i].Expression.Cast(typeof(PSReference)), CachedReflectionInfo.PSReference_Value);
                         initTemps.Add(Expression.Assign(temp, psRefValue.Convert(temp.Type)));
                         copyOutTemps.Add(Expression.Assign(psRefValue, temp.Cast(typeof(object))));
+
                         argExprs[i] = temp;
+                        argsToLog.Add(temp);
                     }
                     else
                     {
-                        argExprs[i] = args[i].CastOrConvertMethodArgument(
+                        var convertedArg = args[i].CastOrConvertMethodArgument(
                             parameterType,
                             paramName,
                             mi.Name,
                             allowCastingToByRefLikeType,
                             temps,
                             initTemps);
+
+                        argExprs[i] = convertedArg;
+                        // If the converted arg is a byref-like type, then we log the original arg.
+                        argsToLog.Add(convertedArg.Type.IsByRefLike
+                            ? args[i].Expression
+                            : convertedArg);
                     }
                 }
             }
@@ -7255,6 +7268,12 @@ internal static Expression InvokeMethod(MethodBase mi, DynamicMetaObject target,
                 }
             }
 
+            // We need to add one expression to log the .NET invocation before actually invoking:
+            //  - Log method invocation to AMSI Notifications (can throw PSSecurityException)
+            //  - Invoke method
+            string targetName = mi.ReflectedType?.FullName ?? string.Empty;
+            string methodName = mi.Name is ".ctor" ? "new" : mi.Name;
+
             if (temps.Count > 0)
             {
                 if (call.Type != typeof(void) && copyOutTemps.Count > 0)
@@ -7265,8 +7284,13 @@ internal static Expression InvokeMethod(MethodBase mi, DynamicMetaObject target,
                     copyOutTemps.Add(retValue);
                 }
 
+                AddMemberInvocationLogging(initTemps, targetName, methodName, argsToLog);
                 call = Expression.Block(call.Type, temps, initTemps.Append(call).Concat(copyOutTemps));
             }
+            else
+            {
+                call = AddMemberInvocationLogging(call, targetName, methodName, argsToLog);
+            }
 
             return call;
         }
@@ -7559,28 +7583,50 @@ internal static void InvalidateCache()
         }
 
 #nullable enable
-        private static Expression MaybeAddMemberInvocationLogging(
+        private static Expression AddMemberInvocationLogging(
             Expression expr,
             string targetName,
             string name,
-            DynamicMetaObject[] args)
+            List<Expression> args)
         {
-#if UNIX && !DEBUG
-            // For efficiency this is a no-op on non-Windows platforms in release builds.
+#if UNIX
+            // For efficiency this is a no-op on non-Windows platforms.
             return expr;
 #else
-            Expression[] invocationArgs = new Expression[args.Length];
-            for (int i = 0; i < args.Length; i++)
+            Expression[] invocationArgs = new Expression[args.Count];
+            for (int i = 0; i < args.Count; i++)
             {
-                invocationArgs[i] = args[i].Expression.Cast(typeof(object));
+                invocationArgs[i] = args[i].Cast(typeof(object));
             }
+
             return Expression.Block(
                 Expression.Call(
                     CachedReflectionInfo.MemberInvocationLoggingOps_LogMemberInvocation,
                     Expression.Constant(targetName),
                     Expression.Constant(name),
                     Expression.NewArrayInit(typeof(object), invocationArgs)),
                 expr);
+#endif
+        }
+
+        private static void AddMemberInvocationLogging(
+            List<Expression> exprs,
+            string targetName,
+            string name,
+            List<Expression> args)
+        {
+#if !UNIX
+            Expression[] invocationArgs = new Expression[args.Count];
+            for (int i = 0; i < args.Count; i++)
+            {
+                invocationArgs[i] = args[i].Cast(typeof(object));
+            }
+
+            exprs.Add(Expression.Call(
+                CachedReflectionInfo.MemberInvocationLoggingOps_LogMemberInvocation,
+                Expression.Constant(targetName),
+                Expression.Constant(name),
+                Expression.NewArrayInit(typeof(object), invocationArgs)));
 #endif
         }
 #nullable disable