Skip to content

Fix NRE in ConsiceView#11435

Merged
iSazonov merged 3 commits intoPowerShell:masterfrom
iSazonov:consiceview-null-check
Jan 14, 2020
Merged

Fix NRE in ConsiceView#11435
iSazonov merged 3 commits intoPowerShell:masterfrom
iSazonov:consiceview-null-check

Conversation

@iSazonov
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov commented Dec 26, 2019 •
edited
Loading

PR Summary

Use LanguagePrimitives.TryConvertTo() as last resort to string conversion.

PR Context

Playing with AccountManagement module I catch an error where properties with disposed object reference present.

PS C:\Windows\System32> $e.psobject.properties

GetterScript    : & { Set-StrictMode -Version 1; $this.Exception.InnerException.PSMessageDetails }
SetterScript    :
MemberType      : ScriptProperty
IsSettable      : False
IsGettable      : True
Value           :
TypeNameOfValue : System.Object
Name            : PSMessageDetails
IsInstance      : False

MemberType      : Property
Value           : System.Management.Automation.MethodInvocationException: Exception calling "ToString" with "0"
                  argument(s): "Can�t delete an already deleted object"
                   ---> System.InvalidOperationException: Can�t delete an already deleted object
                     at System.DirectoryServices.AccountManagement.Principal.CheckDisposedOrDeleted()
                     at System.DirectoryServices.AccountManagement.Principal.HandleGet[T](T& currentValue, String
                  name, LoadState& state)
                     at System.DirectoryServices.AccountManagement.Principal.get_Name()
                     at System.DirectoryServices.AccountManagement.Principal.ToString()
                     at CallSite.Target(Closure , CallSite , Object )
                     --- End of inner exception stack trace ---
                     at System.Management.Automation.ExceptionHandlingOps.ConvertToMethodInvocationException(Exception
                  exception, Type typeToThrow, String methodName, Int32 numArgs, MemberInfo memberInfo) in C:\Users\sie
                  \Documents\GitHub\iSazonov\PowerShell\src\System.Management.Automation\engine\runtime\Operations\Misc
                  Ops.cs:line 2062
                     at CallSite.Target(Closure , CallSite , Object )
                     at System.Dynamic.UpdateDelegates.UpdateAndExecute1[T0,TRet](CallSite site, T0 arg0)
                     at CallSite.Target(Closure , CallSite , Object )
                     at System.Management.Automation.Interpreter.DynamicInstruction`2.Run(InterpretedFrame frame) in C:
                  \Users\sie\Documents\GitHub\iSazonov\PowerShell\src\System.Management.Automation\engine\interpreter\D
                  ynamicInstructions.Generated.cs:line 141
                     at System.Management.Automation.Interpreter.EnterTryCatchFinallyInstruction.Run(InterpretedFrame
                  frame) in C:\Users\sie\Documents\GitHub\iSazonov\PowerShell\src\System.Management.Automation\engine\i
                  nterpreter\ControlFlowInstructions.cs:line 357
IsSettable      : False
IsGettable      : True
TypeNameOfValue : System.Exception
Name            : Exception
IsInstance      : True

MemberType      : Property
Value           :
IsSettable      : False
IsGettable      : True
TypeNameOfValue : System.Object
Name            : TargetObject
IsInstance      : True

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 Dec 26, 2019
@iSazonov iSazonov added this to the GA-consider milestone Dec 26, 2019
@iSazonov iSazonov requested a review from SteveL-MSFT December 26, 2019 04:43
@iSazonov iSazonov self-assigned this Dec 26, 2019
SteveL-MSFT
SteveL-MSFT approved these changes Jan 3, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

Possible to add a test? Seems like we can create an ErrorRecord that hits this code path?

@iSazonov
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Jan 3, 2020

Possible to add a test? Seems like we can create an ErrorRecord that hits this code path?

@SteveL-MSFT I did not find how to do it :-(

@SteveL-MSFT
Copy link
Copy Markdown
Member

SteveL-MSFT commented Jan 7, 2020

@iSazonov seems like line 802 would have already covered this? Can you show how it looks before and after this change with your error?

@adityapatwardhan
Copy link
Copy Markdown
Member

adityapatwardhan commented Jan 9, 2020

@iSazonov Can you provide a repro with your module?

@iSazonov
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Jan 10, 2020 •
edited
Loading

My fix is not right.

$value = $prop.Value.ToString().Trim()

The line throws if ToString() throws ($prop.Value has a reference on an object with property with disposed object in my case)

I will update the PR.

SteveL-MSFT
SteveL-MSFT requested changes Jan 10, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

Ok, so it's calling a method on a disposed object.

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Jan 10, 2020
@SteveL-MSFT
Copy link
Copy Markdown
Member

SteveL-MSFT commented Jan 13, 2020

Tried to simulate the original issue of a disposed object, but couldn't find a way to do it.

@iSazonov
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Jan 14, 2020

Tried to simulate the original issue of a disposed object, but couldn't find a way to do it.

I too. I could port a code from AccountManagement module but I think it is not correct behavior for ToString() to throw and I will report the issue to Core team.

@iSazonov iSazonov merged commit ac06918 into PowerShell:master Jan 14, 2020
@iSazonov iSazonov deleted the consiceview-null-check branch January 14, 2020 03:33
daxian-dbw pushed a commit that referenced this pull request Jan 14, 2020
@iSazonov @daxian-dbw
Fix NRE in ConsiceView (#11435)
1ecaae2 
Use LanguagePrimitives.TryConvertTo() as last resort to string conversion.
@daxian-dbw daxian-dbw modified the milestones: GA-approved, 7.0.0-rc.2 Jan 14, 2020
@ghost
Copy link
Copy Markdown

ghost commented Jan 16, 2020

🎉v7.0.0-rc.2 has been released which incorporates this pull request.:tada:

Handy links:

@TravisEz13 TravisEz13 mentioned this pull request Oct 12, 2022
Merged
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

+1 more reviewer

@PoshChan PoshChan PoshChan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@iSazonov iSazonov

Labels

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

Projects

None yet

Milestone

7.0.0-rc.2

Development

Successfully merging this pull request may close these issues.

5 participants

@iSazonov @SteveL-MSFT @adityapatwardhan @PoshChan @daxian-dbw