Skip to content

Fix NRE in ActionPreference parameters#11779

Closed
iSazonov wants to merge 1 commit intoPowerShell:masterfrom
iSazonov:fix-nre-common-parameters
Closed

Fix NRE in ActionPreference parameters#11779
iSazonov wants to merge 1 commit intoPowerShell:masterfrom
iSazonov:fix-nre-common-parameters

Conversation

@iSazonov
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov commented Feb 5, 2020 •
edited
Loading

PR Summary

Fix #11692

Add ValidateNotNull attribute to ErrorAction, WarningAction and InformationAction common parameters.

PR Context

PR Checklist

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Feb 5, 2020
@iSazonov iSazonov added this to the 7.1.0-preview.1 milestone Feb 5, 2020
@ghost ghost assigned adityapatwardhan Feb 5, 2020
PaulHigin
PaulHigin approved these changes Feb 7, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

This looks right to me. I can't think of any negative consequences in making this change, since it currently throws NRE.

@iSazonov iSazonov force-pushed the fix-nre-common-parameters branch from edf1f50 to 6abd5c4 Compare March 9, 2020 17:34
@daxian-dbw
Copy link
Copy Markdown
Member

daxian-dbw commented Mar 13, 2020

I think the right fix should be in the parameter binding code, where the NullReferenceException is thrown.
This could happen to any parameter of a enum type, not just the ActionPreference ones.

@iSazonov
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Mar 13, 2020

@daxian-dbw My initial thoughts was the same. But after I look the binding code I think the code is too sensitive (we can break something) and the simple fix is more reliable.

The code is in ReflectionParameterBinder()

PowerShell/src/System.Management.Automation/engine/ReflectionParameterBinder.cs

Line 192 in e9152a5

s_setterMethods.TryAdd(Tuple.Create(typeof(CommonParameters), "ErrorAction"), (o, v) => ((CommonParameters)o).ErrorAction = (ActionPreference)v);

Can we safely replace (ActionPreference)v with v as ActionPreference?

@daxian-dbw
Copy link
Copy Markdown
Member

daxian-dbw commented Mar 13, 2020 •
edited
Loading

The fix should be doing the same as in here:

PowerShell/src/System.Management.Automation/engine/ReflectionParameterBinder.cs

Lines 233 to 249 in e9152a5

Expression expr = Expression.Assign(propertyExpr, Expression.Convert(value, propertyExpr.Type));
if (propertyExpr.Type.IsValueType && Nullable.GetUnderlyingType(propertyExpr.Type) == null)
{
var throwInvalidCastExceptionExpr =
Expression.Call(Language.CachedReflectionInfo.LanguagePrimitives_ThrowInvalidCastException,
Language.ExpressionCache.NullConstant,
Expression.Constant(propertyExpr.Type, typeof(Type)));
// The return type of 'ThrowInvalidCastException' is System.Object, but the method actually always
// throws 'PSInvalidCastException' when it's executed. So converting 'throwInvalidCastExceptionExpr'
// to 'propertyExpr.Type' is fine, because the conversion will never be hit.
expr = Expression.Condition(Expression.Equal(value, Language.ExpressionCache.NullConstant),
Expression.Convert(throwInvalidCastExceptionExpr, propertyExpr.Type),
expr);
}
return Expression.Lambda<Action<object, object>>(expr, new[] { target, value }).Compile();

I will submit a PR shortly. See #12124. I think it can supersede this PR. @iSazonov please let me know if you have any concerns. I assigned the new PR to you for review.

@iSazonov
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Mar 16, 2020

@daxian-dbw #12124 returns better error message so I close current PR.

@iSazonov iSazonov closed this Mar 16, 2020
@iSazonov iSazonov deleted the fix-nre-common-parameters branch March 16, 2020 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@PoshChan PoshChan PoshChan left review comments

@PaulHigin PaulHigin PaulHigin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@adityapatwardhan adityapatwardhan

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

7.1.0-preview.1

Development

Successfully merging this pull request may close these issues.

ErrorAction for Test-Path throw an NullReferenceException

5 participants

@iSazonov @daxian-dbw @PaulHigin @PoshChan @adityapatwardhan