Skip null data (fix NRE) in output data received handler #11448

iSazonov · 2019-12-28T16:55:44Z

PR Summary

Add null check in OnOutputDataReceived() method.

PR Context

Fix #11445. The NRE comes from #11380.

PR Checklist

src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs

PaulHigin · 2020-01-06T16:34:23Z

src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs

@@ -1217,6 +1219,11 @@ protected override void CleanupConnection()

        private void OnOutputDataReceived(object sender, DataReceivedEventArgs e)
        {
+            if (string.IsNullOrEmpty(e.Data))


Please remove this code, and add this null check to line 749 above.

What should GetMessageGuid() return in the case? Guid.Empty?

Yes. Ensuing data processing will check for null or bad packets and do the appropriate thing. Thanks!

PaulHigin

LGTM

iSazonov · 2020-01-06T18:48:46Z

@PaulHigin Make sense to optimize GetMessageGuid() method? Is it on hot path? I could exclude allocations (we don't use parsed GUID) and try-catch.

PaulHigin · 2020-01-06T19:00:44Z

@iSazonov
My first thought was to let later packet processing check for null/empty packet string. But that is an invalid PSRP packet and nothing can be done with it. So now I am thinking we should do the null/empty check at protected void HandleOutputDataReceived() and simply return if string is null or empty.

I don't believe there will be any noticeable perf impact, but it feels cleaner.

To summarize, I think we should move the null check from line 749 to line 773.

Do you agree?

iSazonov · 2020-01-06T19:38:37Z

invalid PSRP packet

If we silently drop the packet it seems we hide a bug. If I undenstand right the packet is created on server before the send code - and if we created a broken package this looks like a bug.
Although the original design allow this. Actually, it worked like that before #11380 - we sent such a empty package. The PR brings back this behavior. This is the best minimal change.
If we do not want to send empty packets, then we should rather eliminate the reason for their appearance. This is much harder to do. I don’t really know where this is going. If this is not critical, then perhaps we could stop on the current fix.

iSazonov · 2020-01-06T19:50:53Z

I added an optimization commit to avoid extra allocations. If you don't like it I will revert.

PaulHigin · 2020-01-06T20:26:15Z

@iSazonov
Packet processing already detects malformed packets and emits a proper error. But a null/empty data string indicates a problem in the transport, e.g., named pipe emitting a null packet because it closed or some reason. In this case the code simply ignores the packet and is the correct thing to do. I feel this check and return should be done in HandleOutputDataReceived() as I mentioned above.

PaulHigin · 2020-01-06T20:30:29Z

src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs


-        private Guid GetMessageGuid(string data)
+        private bool ContainsMessageGuid(ReadOnlySpan<char> data)
        {
            // Perform quick scan for data packet for a GUID, ignoring any errors.


Please add try/catch. This method should not throw.

With the new implementation the method can not throw. But I added explicit try-catch to avoid a regression if anybody change the method.

Both Slice() and IndexOf() methods can throw.

It seems ReadOnlySpan.IndexOf() never throw. Slice() could be but IndexOf() before it protects from this.
In any case I already added try-catch to exclude a regression in future..

PaulHigin · 2020-01-06T20:31:15Z

src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs

@@ -775,15 +766,15 @@ protected void HandleOutputDataReceived(string data)
            {
                // Route protocol message based on whether it is a session or command message.


Please move string null/empty check to here.

I added the explicit check.

src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs

PaulHigin · 2020-01-07T17:16:54Z

src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs

-                    // Ignore any malformed packet errors here and return an empty Guid.
-                    // Packet errors will be reported later during message processing.
+                    var psGuidString = data.Slice(iTag + GUIDTAG.Length);
+                    return Guid.TryParse(psGuidString, out _);


Using Span.Slice() is a good optimization. But this code is actually incorrect as is. All PSRP packets contain a Guid. But a session message guid is always an empty guid (which is why the original code compares to an empty Guid). A non-empty guid means the PSRP packet is a command message. This method is used to route the message to the appropriate processing thread. To fix this, please make the following changes;

private bool IsCommandMessage(ReadOnlySpan<char> data) { try { // Perform quick scan for data packet GUID. var iTag = data.Index(GUIDTAG, StringComparison.OrdinalIgnoreCase); if (iTag > -1) { // An empty GUID value means this is a session message, otherwise it is a command message. var psGuidString = data.Slice(iTag + GUIDTag.Length, GUID_STR_LEN); if (Guid.TryParse(psGuidString out Guid psGuid)) { return !Guid.Equals(psGuid, Guid.Empty); } } } catch { // The method should never throw. } // Missing Guid means an invalid packet. Continue processing as session message. return false; }

Ah, I did know. Thanks!

Do we know that the package integrity already was checked and GUID always present? If yes we could only search the const without parsing:

Suggested change

return Guid.TryParse(psGuidString, out _);

private const string GUIDTAG = "PSGuid=00000000-0000-0000-0000-000000000000'";

Yes, good point. We can simply search for an empty PSGuid element. All valid packets will have a PSGuid element, and if invalid we just treat as a session message.

I think we can make it even simpler. I now agree that the method won't throw so we can remove the try/catch. Also empty Guid equates to session message so we should name the method correctly. I was thinking:

private const string SESSIONGUID = "PSGuid='00000000-0000-0000-0000-000000000000'"; private bool IsSessionMessage(string data) { // Perform quick scan for an empty GUID element, which identifies a session message. return data.IndexOf(SESSIONGUID, StringComparison.OrdinalIgnoreCase) > -1; }

Since this is only used for routing, any invalid packet can be routed to command thread ... it really shouldn't matter.

PaulHigin · 2020-01-07T17:17:09Z

src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs

                // Route protocol message based on whether it is a session or command message.
                // Session messages have empty Guid values.
-                if (Guid.Equals(GetMessageGuid(data), Guid.Empty))
+                if (ContainsMessageGuid(data))


Please change to use new IsCommandMessage().

PaulHigin · 2020-01-07T17:17:17Z

src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs

+                    // In this case we simply ignore the packet.
+                    return;
+                }
+
                // Route protocol message based on whether it is a session or command message.
                // Session messages have empty Guid values.


Please remove this comment.
I mean please just remove this comment:
// Session messages have empty Guid values.
because it seems unnecessary now.

PaulHigin · 2020-01-07T17:43:32Z

src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs

@@ -773,17 +771,25 @@ protected void HandleOutputDataReceived(string data)
        {
            try
            {
+                if (string.IsNullOrEmpty(data))


Minor comment. The null check does not need to be within the try/catch.

PaulHigin

LGTM

PaulHigin · 2020-01-07T18:38:12Z

src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs


-        private Guid GetMessageGuid(string data)
+        private bool IsSessionMessage(ReadOnlySpan<char> data)


Just curious if we still need to pass as Span type. We can just scan a normal string type, but I am not sure if it really makes a difference.

:-) Yes, but we again would care about null to exclude a regression in future.

I think if you worry that a null value may be passed to this method, the intent is more clear by having a null check here:

private bool IsSessionMessage(string data) { if (data == null) { return false; } return data.IndexOf(SESSIONDMESSAGETAG, StringComparison.OrdinalIgnoreCase) > -1; }

Or, maybe this method should just be directly inlined in HandleOutputDataReceived given it's just one line of code and not used anywhere else.

I prefer having the private method since it makes clear what the purpose is. Null check is not needed.

Inlined the method.

Oops, @PaulHigin sorry I did not see your comment. Perhaps SESSIONDMESSAGETAG well says that the purpose is and inlining is goog? Thoughts?

@PaulHigin Would it be OK to add a comment to the inlined code to make the purpose clear?
I don't mind having it a separate private method, just thought using string data should be sufficient.

I assumed the compiler would inline but I am fine either way. These are minor issues and overall I am happy with the changes.

iSazonov · 2020-01-10T03:36:11Z

@PaulHigin Thanks for help!

PaulHigin · 2020-01-10T16:12:04Z

@iSazonov Thanks for fixing this!

…eException` (#11448)

ghost · 2020-01-16T23:38:52Z

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

Handy links:

Release Notes

Skip null data in output data received handler

5335323

iSazonov added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Dec 28, 2019

iSazonov added this to the GA-consider milestone Dec 28, 2019

iSazonov requested review from daxian-dbw, SteveL-MSFT, adityapatwardhan and PaulHigin December 28, 2019 16:55

iSazonov assigned daxian-dbw Dec 28, 2019

PoshChan reviewed Dec 28, 2019

View reviewed changes

src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs Show resolved Hide resolved

PaulHigin suggested changes Jan 6, 2020

View reviewed changes

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 6, 2020

Move fix to GetMessageGuid()

a087950

PaulHigin approved these changes Jan 6, 2020

View reviewed changes

Refactor GetMessageGuid() to avoid extra allocations

c893e62

daxian-dbw requested a review from PaulHigin January 6, 2020 20:28

PaulHigin suggested changes Jan 6, 2020

View reviewed changes

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 6, 2020

Address feedback

69522d5

PoshChan reviewed Jan 7, 2020

View reviewed changes

src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs Show resolved Hide resolved

PaulHigin suggested changes Jan 7, 2020

View reviewed changes

ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jan 7, 2020

SteveL-MSFT added the MustHave label Jan 7, 2020

PaulHigin suggested changes Jan 7, 2020

View reviewed changes

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jan 7, 2020

iSazonov added 4 commits January 7, 2020 23:23

Implement IsCommandMessage()

bedfef8

Remove comment

0cc9c70

Move if block out of try-catch

7e2a097

Implement IsSessionMessage()

20edc1e

PaulHigin approved these changes Jan 7, 2020

View reviewed changes

Inline IsSessionMessage()

6f49088

daxian-dbw approved these changes Jan 10, 2020

View reviewed changes

daxian-dbw merged commit 397756a into PowerShell:master Jan 10, 2020

iSazonov deleted the remoting-nre-in-handler branch January 10, 2020 03:34

adityapatwardhan modified the milestones: GA-consider, GA-approved Jan 14, 2020

daxian-dbw pushed a commit that referenced this pull request Jan 14, 2020

Skip null data in output data received handler to fix a `NullReferenc…

8059342

…eException` (#11448)

daxian-dbw modified the milestones: GA-approved, 7.0.0-rc.2 Jan 14, 2020

ghost mentioned this pull request Jan 16, 2020

PowerShell crash on Windows Server 2012 R2 with release tests: OutOfProcessClientSessionTransportManagerBase.GetMessageGuid #11445

Closed

TravisEz13 mentioned this pull request Oct 12, 2022

Back-port to 7.0: Update list of PowerShell team members in release tools #18266

Merged

22 tasks

		@@ -775,15 +766,15 @@ protected void HandleOutputDataReceived(string data)
		{
		// Route protocol message based on whether it is a session or command message.

	return Guid.TryParse(psGuidString, out _);
	private const string GUIDTAG = "PSGuid=00000000-0000-0000-0000-000000000000'";


		private Guid GetMessageGuid(string data)
		private bool IsSessionMessage(ReadOnlySpan<char> data)

Skip null data (fix NRE) in output data received handler #11448

Skip null data (fix NRE) in output data received handler #11448

Conversation

iSazonov commented Dec 28, 2019 • edited Loading

PR Summary

PR Context

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulHigin left a comment

Choose a reason for hiding this comment

iSazonov commented Jan 6, 2020

PaulHigin commented Jan 6, 2020

iSazonov commented Jan 6, 2020

iSazonov commented Jan 6, 2020

PaulHigin commented Jan 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov Jan 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulHigin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov Jan 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov Jan 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov commented Jan 10, 2020

PaulHigin commented Jan 10, 2020

ghost commented Jan 16, 2020

iSazonov commented Dec 28, 2019 •

edited

Loading

iSazonov Jan 7, 2020 •

edited

Loading

iSazonov Jan 7, 2020 •

edited

Loading

iSazonov Jan 7, 2020 •

edited

Loading