Skip to content

Do not check first byte before handshake #5415

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

Closed
Forgind wants to merge 5 commits into from
Closed

Do not check first byte before handshake #5415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
65003ff
Remove check for 5F/60
Forgind Jun 9, 2020
f589e65
Remove outdated comment
Forgind Jun 9, 2020
e8eb6db
Merge https://github.com/microsoft/msbuild into uncheck-bytes
Forgind Jun 9, 2020
da3a1b9
Unmask change
Forgind Jun 9, 2020
0ab8794
Update src/Shared/NodeEndpointOutOfProcBase.cs
Forgind Jun 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 3 additions & 20 deletions src/Shared/CommunicationsUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,11 +311,7 @@ private static long GenerateHostHandshakeFromBase(long baseHandshake)
}
}
#endif

// Mask out the first byte. Modern builds expect the first byte to be zero to indicate that they are modern
// and should be treated as such. Older builds used a non-zero initial byte. See here:
// https://github.com/microsoft/msbuild/blob/584ca5f11b28971f5651b4b8de5f173ad1cb2786/src/Shared/NodeEndpointOutOfProcBase.cs#L403.
return baseHandshake & 0x00FFFFFFFFFFFFFF;
return baseHandshake;
}

/// <summary>
Expand Down Expand Up @@ -377,7 +373,7 @@ internal static long ReadLongForHandshake(this PipeStream stream
#endif
)
{
return stream.ReadLongForHandshake((byte[])null, 0
return stream.ReadLongForHandshake(0
#if NETCOREAPP2_1 || MONO
, handshakeReadTimeout
#endif
Expand All @@ -388,7 +384,7 @@ internal static long ReadLongForHandshake(this PipeStream stream
/// Extension method to read a series of bytes from a stream.
/// If specified, leading byte matches one in the supplied array if any, returns rejection byte and throws IOException.
/// </summary>
internal static long ReadLongForHandshake(this PipeStream stream, byte[] leadingBytesToReject,
internal static long ReadLongForHandshake(this PipeStream stream,
Copy link
Copy Markdown
Member

@danmoseley danmoseley Jun 15, 2020

Choose a reason for hiding this comment

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

Since this handshake has caused some pain, maybe consider in the exceptions (like the one below) including the hex form of the bytes that were received (and possibly what was expected, if it was different)

byte rejectionByteToReturn
#if NETCOREAPP2_1 || MONO
, int timeout
Expand Down Expand Up @@ -435,19 +431,6 @@ byte rejectionByteToReturn
throw new IOException(String.Format(CultureInfo.InvariantCulture, "Unexpected end of stream while reading for handshake"));
}

if (i == 0 && leadingBytesToReject != null)
{
foreach (byte reject in leadingBytesToReject)
{
if (read == reject)
{
stream.WriteByte(rejectionByteToReturn); // disconnect the host

throw new IOException(String.Format(CultureInfo.InvariantCulture, "Client: rejected old host. Received byte {0} but this matched a byte to reject.", bytes[i])); // disconnect and quit
}
}
}

bytes[i] = Convert.ToByte(read);
}
}
Expand Down
16 changes: 1 addition & 15 deletions src/Shared/NodeEndpointOutOfProcBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,23 +384,9 @@ private void PacketPumpProc()
// respond with another long. Once the handshake is complete, both sides can be assured the
// other is ready to accept data.
// To avoid mixing client and server builds, the long is the MSBuild binary timestamp.

// Compatibility issue here.
// Previous builds of MSBuild 4.0 would exchange just a byte.
// Host would send either 0x5F or 0x60 depending on whether it was the toolset or not respectively.
// Client would return either 0xF5 or 0x06 respectively.
// Therefore an old host on a machine with new clients running will hang,
// sending a byte and waiting for a byte until it eventually times out;
// because the new client will want 7 more bytes before it returns anything.
// The other way around is not a problem, because the old client would immediately return the (wrong)
// byte on receiving the first byte of the long sent by the new host, and the new host would disconnect.
// To avoid the hang, special case here:
// Make sure our handshakes always start with 00.
// If we received ONLY one byte AND it's 0x5F or 0x60, return 0xFF (it doesn't matter what as long as
// it will cause the host to reject us; new hosts expect 00 and old hosts expect F5 or 06).
try
{
long handshake = localReadPipe.ReadLongForHandshake(/* reject these leads */ new byte[] { 0x5F, 0x60 }, 0xFF /* this will disconnect the host; it expects leading 00 or F5 or 06 */
long handshake = localReadPipe.ReadLongForHandshake(0xFF /* this will disconnect a < 4.5 host; it expects leading 00 or F5 or 06 */
#if NETCOREAPP2_1 || MONO
, ClientConnectTimeout /* wait a long time for the handshake from this side */
#endif
Expand Down