-
Notifications
You must be signed in to change notification settings - Fork 7.2k
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.
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
Remove BinaryFormatter use in PSRP serialization #17133
Remove BinaryFormatter use in PSRP serialization #17133
Conversation
Thanks a lot @jborean93 for this pull request, I really appreciate it! |
I would love to get rid of the My main concern is how it might affect Exchange endpoints. I was not on the team when this property was included, but I seem to recall it was included for Exchange endpoints. One possible solution could be to serialize this object ourselves 'safely' so that it can be deserialized and used on older endpoints. But I don't know how important it is and if it is really needed. Marking for committee review. |
get; | ||
internal set; | ||
} | ||
public TimeZoneInfo ClientTimeZone => TimeZoneInfo.Local; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This violates the remoting protocol (PSRP). We either need to provide the actual client timezone or nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sending nothing would be the most straightforward solution then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it violate the protocol? PowerShell, since v3, has ignored whatever was sent to it from the client and did TimeZone.CurrentTimeZone as per
PowerShell/src/System.Management.Automation/engine/remoting/common/WireDataFormat/EncodeAndDecode.cs
Lines 2376 to 2392 in 7cc9c87
if (dataAsPSObject.Properties[RemoteDataNameStrings.TimeZone] != null) | |
{ | |
// Binary deserialization of timezone info via BinaryFormatter is unsafe, | |
// so don't deserialize any untrusted client data using this API. | |
// | |
// In addition, the binary data being sent by the client doesn't represent | |
// the client's current TimeZone unless they somehow accessed the | |
// StandardName and DaylightName. These properties are initialized lazily | |
// by the .NET Framework, and would be populated by the server with local | |
// values anyways. | |
// | |
// So just return the CurrentTimeZone. | |
#if !CORECLR // TimeZone Not In CoreCLR | |
result.TimeZone = TimeZone.CurrentTimeZone; | |
#endif | |
} |
The TimeZone
class is deprecated and further along PowerShell just changes it to TimeZoneInfo.Local
I can make this null
and just always return that if you wish, it just seems like having it always set to this value is more aligned with the current behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please leave it null, as that conforms to the protocol (which makes this capability field optional). Also I notice that currently this field is null now, with this previous code:
#if !CORECLR // TimeZone Not In CoreCLR
result.TimeZone = TimeZone.CurrentTimeZone;
#endif
I can change the code to omit the field altogether if you wish, the only reason why I send an empty byte value is that
I tested both a missing and empty value and decided on the latter to try and preserve as much existing behaviour as I could (
Yea I can understand this concern and ultimately was hoping that you, or someone else in the pwsh team, can get them to review this change. I know for sure that Exchange works with the property not present altogether as my Python client doesn't add this field at all, I'm just not sure if it will be able to handle an empty value that this PR has today. |
@PowerShell/powershell-committee reviewed this, we agree that given dotnet's deprecation and removal plan, we should remove binaryformatter completely and not replace the optional protocol property with incorrect information. We can get customer feedback on impact through 7.3, however, the only viable mitigation we can give them is to use 5.1 or 7.2 instead. |
@jborean93 and @PaulHigin, can you please help me understand if additional changes are needed given the committee's comments? |
The committee Ok'd removing the ClientTimeZone message since it is optional in the protocol. We want to see if there are any adverse affects. However, I believe this PR will have to change to not substitute the ClientTimeZone on the server with the current local timezone, as that does not reflect the actual client timezone and violates the protocol. |
This is currently what happens today, the server (since pwsh v3) will ignore the PowerShell/src/System.Management.Automation/engine/remoting/common/WireDataFormat/EncodeAndDecode.cs Lines 2376 to 2392 in 7cc9c87
Only PowerShell 2 actually read this value and parsed it. I have no idea what Exchange does though and whether they use this value for anything. I know it doesn't care if the property doesn't exist as per the docs it is optional but I'm unsure what it does with an empty value like |
How do you know? Do you have access to the code base? In any case, this is contrary to what the protocol states. So either the protocol must change or the code. This PR should change the code and we can evaluate its effects in preview. |
The only way I can, through tools like dotpeek and dnsspy. On Windows 7 with pwsh 2 installed the method there will actually deserialize the .NET binary blob. On pwsh v3 and newer (since 2012) it has the same comment you see today in the code.
That's fine with me, I don't know where you keep the docs, I'm just trying to remove a component that is going to be removed from .NET. My goal is to try and keep the same behaviour as is today. |
@jborean93 It is not just updating protocol docs (which requires approval) but also updating the protocol version since it will be (or has been, since the code is now doing this) changed (ClientTimeZone information is actually ServerTimeZone). That protocol change makes no sense and is not worth doing in my opinion and as the committee recommends, this PR should simply remove it, since it is optional. I am thinking the 'ClientTimeZone' property should be set to null. |
Considering the change happened in pwsh v3 the protocol version was already bumped from 2.1 to 2.2 I'm not sure it really warrants a new bump for something that has already happened. I'm happy to just omit the value altogether but it is strictly a breaking change; |
Since the property has been set to the server time zone since PowerShell v3, I think we should keep it that way to avoid potentially breaking anything. |
@daxian-dbw We cannot keep this behavior, regardless of how long it has been there, as it violates the protocol and we are non-compliant. We either need to change the protocol (which doesn't make sense) or adhere to the protocol which allows us to not send the message (since it is optional). |
OK, so it looks to me that means the server side timezone needs to be fixed regardless of the rest changes in this PR. |
@daxian-dbw |
get; | ||
internal set; | ||
} | ||
public TimeZoneInfo ClientTimeZone => TimeZoneInfo.Local; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please leave it null, as that conforms to the protocol (which makes this capability field optional). Also I notice that currently this field is null now, with this previous code:
#if !CORECLR // TimeZone Not In CoreCLR
result.TimeZone = TimeZone.CurrentTimeZone;
#endif
@jborean93 can you please address @PaulHigin's comments? |
@PaulHigin I just pushed a commit to address you comment (leave |
Sorry I've been slack in following it up, don't you also want to remove the line https://github.com/PowerShell/PowerShell/pull/17133/files#diff-24dd8a004527ebae1b7fb5796191281dc116c2f41ed925f4f12b38cab99a71dfR1604 so it doesn't add the property at all in the CLIXML. I believe that was desired rather than an empty byte array.
|
@PaulHigin Can you comment on @jborean93's message above? |
Hmm, yes thanks @jborean93. I think it is best to just not include the TimeZone data item in the message stream. |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
OK, that line was removed. Can both of you take another look? I will merge if all look good. |
The only breaking change I know off is that now is when using a client with this change, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you both. I will merge this PR today. |
🎉 Handy links: |
PR Summary
Removes the usage of
BinaryFormatter
in PSRP connections.PR Context
The
BinaryFormatter
class is marked as obsolete and is planned to be removed in a future .NET version https://github.com/dotnet/designs/blob/main/accepted/2020/better-obsoletion/binaryformatter-obsoletion.md. One location where it is used in pwsh is when serializing the client's time zone. On the server end PowerShell has ignored this property since v3 and even on v2 it has an exception handler to ignore invalid values. This means the client can either omit the property altogether or send an empty value and the server will still be fine.One minor difference between a missing property and an empty property value is that
$PSSenderInfo.ClientTimeZone
is$null
when it's missing and set to[TimeZone]::Current
when it's an empty value. So to preserve as much existing behaviour an empty value was used instead.The only concern I have with this approach that I haven't tested it with an Exchange endpoint to see if it has any special behaviour with this property.
Partially addressed #14054
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).