Skip to content

Fix NRE in ActionPreference parameters #11779

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
Closed
iSazonov wants to merge 1 commit into from
Closed

Fix NRE in ActionPreference parameters #11779

iSazonov wants to merge 1 commit into from

Conversation

@iSazonov
Copy link
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
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
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
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
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
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