Don't use TranslateDotNet in TaskParameter#9111
Conversation
…th and without the changes
|
(May be easier to review by commit.) |
rokonec
left a comment
There was a problem hiding this comment.
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.
|
@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:
As a consequence, custom
Since this functionality is somewhat obscure, I did my best to cover it with tests, doing some refactoring along the way. For example, The |
No longer the case |
Fixes #8923
Context
Moving
TaskParameterinstances between processes usesTranslateDotNetfor serializing/deserializing value types and value type arrays. This functionality is implemented on top ofBinaryFormatterand as such problematic from compliance perspective.Changes Made
Reworked
TaskParameterso now it operates on the following types:PrimitiveTypeandPrimitiveTypeArray, which are converted withConvert.ChangeType(o, typeof(T))to/from string or serialized natively if the type is supported byITranslator. These types additionally useSystem.TypeCodeof the specific type as a discriminator.ValueTypeandValueTypeArray, which are converted only to string because they can only be used in task outputs. The requirement for these to implementIConvertibleis not new. Without the changes in this PR a serializable but non-IConvertibletask output parameter can be moved from the task host process but fails later because the sameConvert.ChangeTypeconversion is attempted by the engine.ITaskItem, ITaskItemArray, Invalid, Null- no change, same behavior as before.Testing
Existing unit tests (extended, enhanced, and refactored).
Notes
BinaryFormatter. This is an unfortunate limitation but it seems to be done elsewhere already (TranslateExceptionfor example) so I'm assuming it's been thought though.