Skip to content

Relay remoted exceptions#8779

Merged
JanKrivanek merged 24 commits intodotnet:mainfrom
JanKrivanek:proto/serialize-exc
Jun 20, 2023
Merged

Relay remoted exceptions#8779
JanKrivanek merged 24 commits intodotnet:mainfrom
JanKrivanek:proto/serialize-exc

Conversation

@JanKrivanek
Copy link
Member

@JanKrivanek JanKrivanek commented May 19, 2023

Fixes #8786

Context

BinaryFormatSerializer is going to be deprecated (https://aka.ms/binaryformatter) - as a result we should not be using our SerializeDotNet function.
This PR is tackling one of the remaining usages - (de)serialization of Exceptions.

Changes Made

Unrelated Issues Workarounded Or Addressed

Testing

  • Testing remoting of all internally supported types (iterating over their all public properties of primitive types)

+ manual test described in the bug

Prior fix:

binaryFormatterBrokenBuild.csproj(6,17): error MSB4113: Specified condition "''" evaluates to "" instead of a boolean.
MSBUILD : error MSB4166: Child node "2" exited prematurely. Shutting down. Diagnostic information may be found in files in
"C:\Users\jankrivanek\AppData\Local\Temp\MSBuildTempjankrivanek\" and will be named MSBuild_*.failure.txt. This location ca
n be changed by setting the MSBUILDDEBUGPATH environment variable to a different directory.
MSBUILD : error MSB4166: C:\Users\jankrivanek\AppData\Local\Temp\MSBuildTempjankrivanek\MSBuild_pid-25752_9a55230dad3b41b09
1eb48954483f835.failure.txt:
MSBUILD : error MSB4166: UNHANDLED EXCEPTIONS FROM PROCESS 25752:
MSBUILD : error MSB4166: =====================
MSBUILD : error MSB4166: 5/22/2023 11:10:07 AM
MSBUILD : error MSB4166: System.NotSupportedException: BinaryFormatter serialization and deserialization are disabled withi
n this application. See https://aka.ms/binaryformatter for more information.
MSBUILD : error MSB4166:    at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Serialize(Stream serializatio
nStream, Object graph)
MSBUILD : error MSB4166:    at Microsoft.Build.Execution.BuildResult.Microsoft.Build.BackEnd.ITranslatable.Translate(ITrans
lator translator)
MSBUILD : error MSB4166:    at Microsoft.Build.BackEnd.NodeEndpointOutOfProcBase.RunReadLoop(Stream localReadPipe, Stream l
ocalWritePipe, ConcurrentQueue`1 localPacketQueue, AutoResetEvent localPacketAvailable, AutoResetEvent localTerminatePacket
Pump)
MSBUILD : error MSB4166: ===================
MSBUILD : error MSB4166:
MSBUILD : error MSB4166:
    0 Warning(s)
    2 Error(s)

After fix:

binaryFormatterBrokenBuild.csproj(6,17): error MSB4113: Specified condition "''" evaluates to "" instead of a boolean.
    0 Warning(s)
    1 Error(s)

Edit 01:

  • Conditioned behind changewave (However TaskHost has this always enabled as it doesn't currently use ChangeWaves)
  • Message, Type nonullable now; HResult transfered if on NET4.5 and above (the constract still respect situation where one side is not on the version)
  • Tests added

Edit 02:

  • Moved remoting related utils to Framework

Edit 03:

  • Introduced internal exceptions hierarchy and type preserving de/serialization
  • This fixes the SDK regression repro

Edit 04:

  • Introduced more reflection - allowing to use exception types not inheriting from common root base
  • This fixes the failing nuget.exe test

Edit 05:

  • Introduced fixed serialization whitelist - this is needed to prevent deserialization exploit similar to BinaryFormatter attack

Edit 06:

  • Increased version of NuGet.CommandLine supported by MSBuild and removed all workaround code that was needed to facilitate it

Copy link
Contributor

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

Hello! I am rainersigwald. Tests? 🙂

@JanKrivanek JanKrivanek requested a review from danmoseley May 22, 2023 10:48
@JanKrivanek
Copy link
Member Author

@danmoseley Thanks for a detailed look! Second look would be very appreciated (I attempted to address all resolved comments; the unresolved one might need clarification)

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

This doesn't fully resolve the crash in the repro from #6215 (comment):

dotnet msbuild -nr:false .\error.proj
MSBuild version 17.7.0-dev-23272-01+2b0963cee for .NET
S:\repro\dotnet\msbuild\issues\6215\error.proj(2,20): error MSB4113: Specified condition "''" evaluates to "" instead of a boolean.
MSBUILD : error MSB1025: An internal failure occurred while running MSBuild.
This is an unhandled exception in MSBuild -- PLEASE OPEN A BUG AGAINST THE MSBUILD TEAM.
Microsoft.Build.Exceptions.InvalidProjectFileException->Microsoft.Build.BackEnd.BuildTransferredException: Specified condition "''" evaluates to "" instead of a boolean.  S:\repro\dotnet\msbuild\issues\6215\error.proj
   at Microsoft.Build.Shared.ProjectErrorUtilities.ThrowInvalidProject(String errorSubCategoryResourceName, IElementLocation elementLocation, String resourceName, Object[] args) in S:\msbuild\src\Shared\ProjectErrorUtilities.cs:line 373
...

I think our internal exception types need to be serialized in a type-preserving way.

@JanKrivanek
Copy link
Member Author

This doesn't fully resolve the crash in the repro from #6215 (comment):

❯ dotnet msbuild -nr:false .\error.proj
MSBuild version 17.7.0-dev-23272-01+2b0963cee for .NET
S:\repro\dotnet\msbuild\issues\6215\error.proj(2,20): error MSB4113: Specified condition "''" evaluates to "" instead of a boolean.
MSBUILD : error MSB1025: An internal failure occurred while running MSBuild.
This is an unhandled exception in MSBuild -- PLEASE OPEN A BUG AGAINST THE MSBUILD TEAM.
Microsoft.Build.Exceptions.InvalidProjectFileException->Microsoft.Build.BackEnd.BuildTransferredException: Specified condition "''" evaluates to "" instead of a boolean.  S:\repro\dotnet\msbuild\issues\6215\error.proj
   at Microsoft.Build.Shared.ProjectErrorUtilities.ThrowInvalidProject(String errorSubCategoryResourceName, IElementLocation elementLocation, String resourceName, Object[] args) in S:\msbuild\src\Shared\ProjectErrorUtilities.cs:line 373
...

I think our internal exception types need to be serialized in a type-preserving way.

Thanks for pointing!
On it

@JanKrivanek JanKrivanek marked this pull request as draft May 22, 2023 17:26
@JanKrivanek JanKrivanek force-pushed the proto/serialize-exc branch from 4e8698c to 6a5f6f8 Compare May 23, 2023 20:36
@JanKrivanek JanKrivanek marked this pull request as ready for review May 24, 2023 20:05
@ericstj
Copy link
Member

ericstj commented May 30, 2023

cc @ViktorHofer
Inserting a base type is allowed. I do recall ensuring we test for this, here's a test that covers it:
https://github.com/dotnet/sdk/blob/0ba7aa399bfc0f5394d0f0a2fabe7c1093e49825/src/Tests/Microsoft.DotNet.ApiCompatibility.Tests/Rules/CannotRemoveBaseTypeOrInterfaceTests.cs#L19-L52
Perhaps there's an issue with references being followed in this comparison where it isn't able to traverse the inheritance hierarchy. 🤔
Issues should be filed here: https://github.com/dotnet/sdk/issues?q=is%3Aopen+is%3Aissue+label%3AArea-ApiCompat

@JanKrivanek
Copy link
Member Author

Thanks @danmoseley and @ericstj for the help!
The new base exception type is not flagged in the assembly that is defining the new base type, t is only flagged in assembly that is referencing it - so your theory @ericstj might be right. The dependency is properly declared - si it still feels as a bug.
Issue created: dotnet/sdk#32922

@JanKrivanek JanKrivanek changed the title Relay remoted exceptions over to single squashed exception type Relay remoted exceptions May 31, 2023
@ViktorHofer
Copy link
Member

ViktorHofer commented May 31, 2023

I did a root cause investigation and shared my finding in dotnet/sdk#32922. Also submitted a fix for it: dotnet/sdk#32930. As we just branched, the fix will likely not be available before Preview 6. While you could use APICompat's nuget package to immediately receive the fix, IMO it's fine to keep this baselined until the repository uses the P6 SDK.

I tested my changes on top of your PR and the errors are gone. From that point of view, your changes are correct 👍

@JanKrivanek JanKrivanek force-pushed the proto/serialize-exc branch from d7ecbcb to f12e99a Compare May 31, 2023 14:27
Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Ok, this isn't a fully complete review but there's enough to ruin your weekend--don't look until Monday morning your time!

@JanKrivanek JanKrivanek force-pushed the proto/serialize-exc branch from b7c6d0b to 4c172dd Compare June 12, 2023 15:32
@JanKrivanek JanKrivanek force-pushed the proto/serialize-exc branch from 6f4315a to 80e0df6 Compare June 14, 2023 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Depart BinaryFormatter for Exceptions serialization

8 participants