Skip to content

Fix globbing for native commands #4489

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

Closed
BrucePay wants to merge 1 commit into from
Closed

Fix globbing for native commands #4489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 104 additions & 8 deletions src/System.Management.Automation/engine/CommandParameter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,14 @@ internal sealed class CommandParameterInternal
{
private class Parameter
{
internal Ast ast;
internal IScriptExtent extent;
Copy link
Contributor

@lzybkr lzybkr Aug 4, 2017

Choose a reason for hiding this comment

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

ast makes storing the extent no longer necessary, right?

Copy link
Contributor

@lzybkr lzybkr Aug 4, 2017

Choose a reason for hiding this comment

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

I mean - extent should always be ast.Extent, so we could remove the field and use a property instead.

Copy link
Collaborator Author

@BrucePay BrucePay Aug 15, 2017

Choose a reason for hiding this comment

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

This API is used in many places where the AST is not available e.g. the PowerShell API, F&O. In those cases dummy extents are explicitly passed in. So both the Ast and extent members are required.

Copy link
Contributor

@lzybkr lzybkr Aug 15, 2017

Choose a reason for hiding this comment

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

We could introduce a dummy ast for the dummy position.

My thinking - if the normal case is a script, we optimize for that case. If the field is typically redundant, we use less memory, generate less code (the push of the extra parameter), and reduce the number of fields gc needs to visit.

Copy link
Collaborator Author

@BrucePay BrucePay Aug 19, 2017

Choose a reason for hiding this comment

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

I think a dummy AST is overkill. Reworking the API to use dummy extents if the AST is null, etc. is probably the best solution but that's out of scope for this issue.

Copy link
Contributor

@lzybkr lzybkr Aug 19, 2017

Choose a reason for hiding this comment

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

Adding a new parameter will bloat the generated code significantly - this is an extra parameter for every argument on every command invocation. We should avoid that if there are alternatives.

Copy link
Contributor

@lzybkr lzybkr Sep 19, 2017

Choose a reason for hiding this comment

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

I'm still not happy with storing 2 references a typical script has a ton of these instances.

What if we use something like:

    private object context;  // Instead of storing Extent and Ast
    // ...
    internal Ast ParameterAst { return _parameter?.context as Ast; }
    internal IScriptExtent ParameterExtent {
        return _parameter?.context as IScriptExtent ?? (_parameter?.context as Ast)?.Extent;
    }
    // etc.

internal string parameterName;
internal string parameterText;
}
private class Argument
{
internal Ast ast;
internal IScriptExtent extent;
Copy link
Contributor

@lzybkr lzybkr Aug 4, 2017

Choose a reason for hiding this comment

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

The same applies here regarding ast and extent.

Copy link
Collaborator Author

@BrucePay BrucePay Aug 15, 2017

Choose a reason for hiding this comment

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

Same response as above - the API is used in places where there is no ast.

internal object value;
internal bool splatted;
Expand Down Expand Up @@ -73,6 +75,10 @@ internal IScriptExtent ParameterExtent
get { return _parameter != null ? _parameter.extent : PositionUtilities.EmptyExtent; }
}

internal Ast ParameterAst {
get { return _parameter != null ? _parameter.ast : null; }
}

/// <summary>
/// The extent of the optional argument, if one was specified.
/// </summary>
Expand All @@ -81,6 +87,11 @@ internal IScriptExtent ArgumentExtent
get { return _argument != null ? _argument.extent : PositionUtilities.EmptyExtent; }
}

internal Ast ArgumentAst
{
get { return _argument != null ? _argument.ast : null; }
}

/// <summary>
/// The value of the optional argument, if one was specified, otherwise UnboundParameter.Value.
/// </summary>
Expand Down Expand Up @@ -141,30 +152,56 @@ internal IScriptExtent ErrorExtent
/// <summary>
/// Create a parameter when no argument has been specified.
/// </summary>
/// <param name="ast">The ast for this parameter</param>
/// <param name="extent">The extent in script of the parameter.</param>
/// <param name="parameterName">The parameter name (with no leading dash).</param>
/// <param name="parameterText">The text of the parameter, as it did, or would, appear in script.</param>
internal static CommandParameterInternal CreateParameter(
internal static CommandParameterInternal CreateParameterWithAst(
Ast ast,
IScriptExtent extent,
string parameterName,
string parameterText)
{
Diagnostics.Assert(extent != null, "Caller to verify extent argument");
return new CommandParameterInternal
{
_parameter =
new Parameter { extent = extent, parameterName = parameterName, parameterText = parameterText }
_parameter = new Parameter
{
ast = ast,
extent = extent,
parameterName = parameterName,
parameterText = parameterText
}
};
}

internal static CommandParameterInternal CreateParameter(
IScriptExtent extent,
string parameterName,
string parameterText)
{
return CreateParameterWithAst(null, extent, parameterName, parameterText);
}

internal static CommandParameterInternal CreateParameterAstOnly(
Ast ast,
string parameterName,
string parameterText
)
{
return CreateParameterWithAst(ast, ast.Extent, parameterName, parameterText);
}

/// <summary>
/// Create a positional argument to a command.
/// </summary>
/// <param name="ast">The ast for this argument</param>
/// <param name="extent">The extent of the argument value in the script.</param>
/// <param name="value">The argument value.</param>
/// <param name="splatted">True if the argument value is to be splatted, false otherwise.</param>
/// <param name="arrayIsSingleArgumentForNativeCommand">If the command is native, pass the string with commas instead of multiple arguments</param>
internal static CommandParameterInternal CreateArgument(
internal static CommandParameterInternal CreateArgumentWithAst(
Ast ast,
IScriptExtent extent,
object value,
bool splatted = false,
Expand All @@ -175,6 +212,7 @@ internal static CommandParameterInternal CreateArgument(
{
_argument = new Argument
{
ast = ast,
extent = extent,
value = value,
splatted = splatted,
Expand All @@ -183,6 +221,24 @@ internal static CommandParameterInternal CreateArgument(
};
}

internal static CommandParameterInternal CreateArgument(
IScriptExtent extent,
object value,
bool splatted = false,
bool arrayIsSingleArgumentForNativeCommand = false)
{
return CreateArgumentWithAst(null, extent, value, splatted, arrayIsSingleArgumentForNativeCommand);
}

internal static CommandParameterInternal CreateArgumentAstOnly(
Ast ast,
object value,
bool splatted = false,
bool arrayIsSingleArgumentForNativeCommand = false)
{
return CreateArgumentWithAst(ast, ast.Extent, value, splatted, arrayIsSingleArgumentForNativeCommand);
}

/// <summary>
/// Create an named argument, where the parameter name is known. This can happen when:
/// * The user uses the ':' syntax, as in
Expand All @@ -193,14 +249,16 @@ internal static CommandParameterInternal CreateArgument(
/// * In the parameter binder when it resolves a positional argument
/// * Other random places that manually construct command processors and know their arguments.
/// </summary>
/// <param name="ast">The ast for this parameter/argument pair</param>
/// <param name="parameterExtent">The extent in script of the parameter.</param>
/// <param name="parameterName">The parameter name (with no leading dash).</param>
/// <param name="parameterText">The text of the parameter, as it did, or would, appear in script.</param>
/// <param name="argumentExtent">The extent of the argument value in the script.</param>
/// <param name="value">The argument value.</param>
/// <param name="spaceAfterParameter">Used in native commands to correctly handle -foo:bar vs. -foo: bar</param>
/// <param name="arrayIsSingleArgumentForNativeCommand">If the command is native, pass the string with commas instead of multiple arguments</param>
internal static CommandParameterInternal CreateParameterWithArgument(
internal static CommandParameterInternal CreateParameterWithArgumentWithAst(
Ast ast,
IScriptExtent parameterExtent,
string parameterName,
string parameterText,
Expand All @@ -213,13 +271,51 @@ internal static CommandParameterInternal CreateParameterWithArgument(
Diagnostics.Assert(argumentExtent != null, "Caller to verify argumentExtent argument");
return new CommandParameterInternal
{
_parameter = new Parameter { extent = parameterExtent, parameterName = parameterName, parameterText = parameterText },
_argument = new Argument { extent = argumentExtent, value = value, arrayIsSingleArgumentForNativeCommand = arrayIsSingleArgumentForNativeCommand },
_parameter = new Parameter
{
ast = ast,
extent = parameterExtent,
parameterName = parameterName,
parameterText = parameterText
},
_argument = new Argument
{
ast = ast != null && ast is CommandParameterAst ? ((CommandParameterAst)ast).Argument : null,
extent = argumentExtent,
value = value,
arrayIsSingleArgumentForNativeCommand = arrayIsSingleArgumentForNativeCommand
},
_spaceAfterParameter = spaceAfterParameter
};
}

#endregion ctor
internal static CommandParameterInternal CreateParameterWithArgument(
IScriptExtent parameterExtent,
string parameterName,
string parameterText,
IScriptExtent argumentExtent,
object value,
bool spaceAfterParameter,
bool arrayIsSingleArgumentForNativeCommand = false)
{
return CreateParameterWithArgumentWithAst(null, parameterExtent, parameterName, parameterText, argumentExtent, value,
spaceAfterParameter, arrayIsSingleArgumentForNativeCommand);
}

internal static CommandParameterInternal CreateParameterWithArgumentAstOnly(
Ast ast,
string parameterName,
string parameterText,
object value,
bool spaceAfterParameter,
bool arrayIsSingleArgumentForNativeCommand = false
)
{
var pAst = (CommandParameterAst) ast;
return CreateParameterWithArgumentWithAst(pAst, pAst.Extent, parameterName, parameterText, pAst.Argument.Extent, value,
spaceAfterParameter, arrayIsSingleArgumentForNativeCommand);
}
#endregion ctor

internal bool IsDashQuestion()
{
Expand Down
21 changes: 18 additions & 3 deletions src/System.Management.Automation/engine/NativeCommandParameterBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,23 @@ internal void BindParameters(Collection<CommandParameterInternal> parameters)
// windbg -k com:port=\\devbox\pipe\debug,pipe,resets=0,reconnect
// The parser produced an array of strings but marked the parameter so we
// can properly reconstruct the correct command line.
bool bareWord = false;
var pAst = parameter.ArgumentAst;
if (pAst != null)
{
if (pAst is System.Management.Automation.Language.StringConstantExpressionAst sce)
{
bareWord = sce.StringConstantType == System.Management.Automation.Language.StringConstantType.BareWord;
}
else if (pAst is System.Management.Automation.Language.ExpandableStringExpressionAst ese)
{
bareWord = ese.StringConstantType == System.Management.Automation.Language.StringConstantType.BareWord;
}
}
appendOneNativeArgument(Context, argValue,
parameter.ArrayIsSingleArgumentForNativeCommand ? ',' : ' ',
sawVerbatimArgumentMarker);
sawVerbatimArgumentMarker,
bareWord);
}
}
}
Expand Down Expand Up @@ -141,7 +155,8 @@ internal String Arguments
/// <param name="obj">The object to append</param>
/// <param name="separator">A space or comma used when obj is enumerable</param>
/// <param name="sawVerbatimArgumentMarker">true if the argument occurs after --%</param>
private void appendOneNativeArgument(ExecutionContext context, object obj, char separator, bool sawVerbatimArgumentMarker)
/// <param name="bareWord">True if the argument was a bare (unquoted) string</param>
private void appendOneNativeArgument(ExecutionContext context, object obj, char separator, bool sawVerbatimArgumentMarker, bool bareWord)
{
IEnumerator list = LanguagePrimitives.GetEnumerator(obj);
bool needSeparator = false;
Expand Down Expand Up @@ -204,7 +219,7 @@ private void appendOneNativeArgument(ExecutionContext context, object obj, char
#if UNIX
// On UNIX systems, we expand arguments containing wildcard expressions against
// the file system just like bash, etc.
if (System.Management.Automation.WildcardPattern.ContainsWildcardCharacters(arg))
if (System.Management.Automation.WildcardPattern.ContainsWildcardCharacters(arg) && bareWord)
{
// See if the current working directory is a filesystem provider location
// We won't do the expansion if it isn't since native commands can only access the file system.
Expand Down
25 changes: 12 additions & 13 deletions src/System.Management.Automation/engine/parser/Compiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,12 @@ internal static class CachedReflectionInfo
internal static readonly MethodInfo CharOps_CompareStringIne =
typeof(CharOps).GetMethod(nameof(CharOps.CompareStringIne), staticFlags);

internal static readonly MethodInfo CommandParameterInternal_CreateArgument =
typeof(CommandParameterInternal).GetMethod(nameof(CommandParameterInternal.CreateArgument), staticFlags);
internal static readonly MethodInfo CommandParameterInternal_CreateParameter =
typeof(CommandParameterInternal).GetMethod(nameof(CommandParameterInternal.CreateParameter), staticFlags);
internal static readonly MethodInfo CommandParameterInternal_CreateParameterWithArgument =
typeof(CommandParameterInternal).GetMethod(nameof(CommandParameterInternal.CreateParameterWithArgument), staticFlags);
internal static readonly MethodInfo CommandParameterInternal_CreateArgumentAstOnly =
typeof(CommandParameterInternal).GetMethod(nameof(CommandParameterInternal.CreateArgumentAstOnly), staticFlags);
internal static readonly MethodInfo CommandParameterInternal_CreateParameterAstOnly =
typeof(CommandParameterInternal).GetMethod(nameof(CommandParameterInternal.CreateParameterAstOnly), staticFlags);
internal static readonly MethodInfo CommandParameterInternal_CreateParameterWithArgumentAstOnly =
typeof(CommandParameterInternal).GetMethod(nameof(CommandParameterInternal.CreateParameterWithArgumentAstOnly), staticFlags);

internal static readonly MethodInfo CommandRedirection_UnbindForExpression =
typeof(CommandRedirection).GetMethod(nameof(CommandRedirection.UnbindForExpression), instanceFlags);
Expand Down Expand Up @@ -3401,8 +3401,8 @@ public object VisitCommand(CommandAst commandAst)

bool arrayIsSingleArgumentForNativeCommand = ArgumentIsNotReallyArrayIfCommandIsNative(element);
elementExprs[i] =
Expression.Call(CachedReflectionInfo.CommandParameterInternal_CreateArgument,
Expression.Constant(element.Extent),
Expression.Call(CachedReflectionInfo.CommandParameterInternal_CreateArgumentAstOnly,
Expression.Constant(element),
Expression.Convert(GetCommandArgumentExpression(element), typeof(object)),
ExpressionCache.Constant(splatted),
ExpressionCache.Constant(arrayIsSingleArgumentForNativeCommand));
Expand Down Expand Up @@ -3509,18 +3509,17 @@ public object VisitCommandParameter(CommandParameterAst commandParameterAst)
bool spaceAfterParameter = (errorPos.EndLineNumber != arg.Extent.StartLineNumber ||
errorPos.EndColumnNumber != arg.Extent.StartColumnNumber);
bool arrayIsSingleArgumentForNativeCommand = ArgumentIsNotReallyArrayIfCommandIsNative(arg);
return Expression.Call(CachedReflectionInfo.CommandParameterInternal_CreateParameterWithArgument,
Expression.Constant(errorPos),
return Expression.Call(CachedReflectionInfo.CommandParameterInternal_CreateParameterWithArgumentAstOnly,
Expression.Constant(commandParameterAst),
Expression.Constant(commandParameterAst.ParameterName),
Expression.Constant(errorPos.Text),
Expression.Constant(arg.Extent),
Expression.Convert(GetCommandArgumentExpression(arg), typeof(object)),
ExpressionCache.Constant(spaceAfterParameter),
ExpressionCache.Constant(arrayIsSingleArgumentForNativeCommand));
}

return Expression.Call(CachedReflectionInfo.CommandParameterInternal_CreateParameter,
Expression.Constant(errorPos),
return Expression.Call(CachedReflectionInfo.CommandParameterInternal_CreateParameterAstOnly,
Expression.Constant(commandParameterAst),
Expression.Constant(commandParameterAst.ParameterName),
Expression.Constant(errorPos.Text));
}
Expand Down
6 changes: 6 additions & 0 deletions test/powershell/Language/Scripting/NativeExecution/NativeUnixGlobbing.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ Describe 'Native UNIX globbing tests' -tags "CI" {
It 'Should return the original pattern if there are no matches' {
/bin/echo $TESTDRIVE/*.nosuchfile | Should Match "\*\.nosuchfile$"
}
It 'Should not expand double-quoted strings' {
/bin/echo "*" | Should BeExactly "*"
}
It 'Should not expand single-quoted strings' {
/bin/echo '*' | Should BeExactly "*"
}
Copy link
Collaborator

@vors vors Aug 4, 2017

Choose a reason for hiding this comment

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

Please add a test for something along the lines of echo '11:1' | grep '.*:.'. The : part is important too to avoid interpreting it as a drive latter

Copy link
Collaborator Author

@BrucePay BrucePay Aug 4, 2017

Choose a reason for hiding this comment

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

This change only impacts expansion of wildcard characters. ':' isn't a wildcard character so I don't see a reason to add this test.

Copy link
Member

@SteveL-MSFT SteveL-MSFT Aug 4, 2017

Choose a reason for hiding this comment

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

I think the colon is an issue in the FileSystem Provider and should probably be in those tests

Copy link
Collaborator Author

@BrucePay BrucePay Aug 15, 2017

Choose a reason for hiding this comment

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

Steve - I'm not sure i understand your comment. I'm not testing the file provider here. I'm just making sure that quoting suppresses globbing. Specific issues with the filesystem provider are orthogonal.

Copy link
Member

@SteveL-MSFT SteveL-MSFT Aug 16, 2017

Choose a reason for hiding this comment

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

Didn't notice that the test didn't involve cmdlets

# Test the behavior in non-filesystem drives
It 'Should not expand patterns on non-filesystem drives' {
/bin/echo env:ps* | Should BeExactly "env:ps*"
Expand Down