Skip to content

Don't use TranslateDotNet in TaskParameter#9111

Merged
ladipro merged 16 commits intodotnet:mainfrom
ladipro:task-parameter-bf
Aug 14, 2023
Merged

Don't use TranslateDotNet in TaskParameter#9111
ladipro merged 16 commits intodotnet:mainfrom
ladipro:task-parameter-bf

Conversation

@ladipro
Copy link
Copy Markdown
Member

@ladipro ladipro commented Aug 8, 2023

Fixes #8923

Context

Moving TaskParameter instances between processes uses TranslateDotNet for serializing/deserializing value types and value type arrays. This functionality is implemented on top of BinaryFormatter and as such problematic from compliance perspective.

Changes Made

Reworked TaskParameter so now it operates on the following types:

  • PrimitiveType and PrimitiveTypeArray, which are converted with Convert.ChangeType(o, typeof(T)) to/from string or serialized natively if the type is supported by ITranslator. These types additionally use System.TypeCode of the specific type as a discriminator.
  • ValueType and ValueTypeArray, which are converted only to string because they can only be used in task outputs. The requirement for these to implement IConvertible is not new. Without the changes in this PR a serializable but non-IConvertible task output parameter can be moved from the task host process but fails later because the same Convert.ChangeType conversion is attempted by the engine.
  • ITaskItem, ITaskItemArray, Invalid, Null - no change, same behavior as before.

Testing

Existing unit tests (extended, enhanced, and refactored).

Notes

  • The new code is under a change wave check so it can be disabled if needed.
  • If the user disables the 17.8 change wave, they will have to make sure that MSBuild can call BinaryFormatter. This is an unfortunate limitation but it seems to be done elsewhere already (TranslateException for example) so I'm assuming it's been thought though.

@ladipro ladipro requested review from JanKrivanek and rokonec August 8, 2023 12:41
@ladipro
Copy link
Copy Markdown
Member Author

ladipro commented Aug 8, 2023

(May be easier to review by commit.)

Copy link
Copy Markdown
Member

@rokonec rokonec left a comment

Choose a reason for hiding this comment

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

Current changes leave us open for deserialization malicious type instantiation kind of security attack. Please consider if it is possible to fix by mine recommendations or otherwise.

@ladipro
Copy link
Copy Markdown
Member Author

ladipro commented Aug 11, 2023

@rokonec @JanKrivanek I ended up reworking the PR, please take another look when you get a chance.

Here is the key revelation that led to the current approach:

IConvertible provides methods to convert the implementing type to primitives, most importantly to string. It does not include the other direction (I had assumed it was a convention with a string-taking constructor, for example), i.e. Convert.ChangeType(o, typeof(T)) fails for custom types. It works only for "built-in" convertibles, of which there is a limited set. They are enumerated in the System.TypeCode enum.

As a consequence, custom IConvertible implementations are allowed to be used only as task outputs, as the engine can convert from the output value to the internal string representation. The set of allowed task inputs types is therefore limited to built-in convertibles and there is no need to communicate the parameter type as part of the payload or infer it from the signature of the corresponding .NET property.

TaskParameter now distinguishes:

  1. PrimitiveType and PrimitiveTypeArray types, which are converted with Convert.ChangeType(o, typeof(T)) to/from string or serialized natively if the type is supported by ITranslator.
  2. ValueType and ValueTypeArray types, which are converted only to string because they can only be task outputs.
  3. ITaskItem, ITaskItemArray, Invalid, Null - same as before.

PrimitiveType and PrimitiveTypeArray additionally serializes the TypeCode of the specific type. I did this to avoid exploding TaskParameterType enum with many more types.

Since this functionality is somewhat obscure, I did my best to cover it with tests, doing some refactoring along the way. For example, TaskParameter_Tests could be easily parameterized using [Theory] instead of many [Fact]s with duplicated code.

The ValueType and ValueTypeArray conversion code is still under a change wave check. At this point I am more confident that the new logic is correct so I'll defer to your opinion whether it's needed or not.

@ladipro ladipro requested a review from rokonec August 11, 2023 10:33
@ladipro
Copy link
Copy Markdown
Member Author

ladipro commented Aug 11, 2023

(May be easier to review by commit.)

No longer the case ☹️

Copy link
Copy Markdown
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

I like it!!

@ladipro ladipro merged commit 0cf89d3 into dotnet:main Aug 14, 2023
@ladipro ladipro deleted the task-parameter-bf branch August 14, 2023 07:43
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.

[BinFmt] ValueType and ValueTypeArray

3 participants