Skip to content

Replace binary serialization of encoding with a custom serializer.#45374

Merged
tmat merged 1 commit intodotnet:masterfrom
tmat:EncodingSerialization
Jun 26, 2020
Merged

Replace binary serialization of encoding with a custom serializer.#45374
tmat merged 1 commit intodotnet:masterfrom
tmat:EncodingSerialization

Conversation

@tmat
Copy link
Member

@tmat tmat commented Jun 23, 2020

@tmat tmat requested review from a team as code owners June 23, 2020 01:41
@jaredpar
Copy link
Member

The referenced bug says that we need to either use a well known safe serialization library or essentially get buy off from the security team on the implementation. Has the latter been done here as this is a custom serializer?

@tmat
Copy link
Member Author

tmat commented Jun 23, 2020

I don't think we need a buy off on this. It only requires one if built-in .NET serializers or third party libraries are used, which is not the case here. We simply serialize the name of the encoding or a well-known id.

@tmat
Copy link
Member Author

tmat commented Jun 26, 2020

@dotnet/roslyn-compiler @dotnet/roslyn-ide PTAL

Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

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

I think we need explicit sign off from security but changes look okay

Copy link
Member

@chsienki chsienki left a comment

Choose a reason for hiding this comment

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

:shipit:

@tmat tmat merged commit bd545a6 into dotnet:master Jun 26, 2020
@ghost ghost added this to the Next milestone Jun 26, 2020
@tmat tmat deleted the EncodingSerialization branch June 26, 2020 18:57
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

LGTM.

[Fact]
public void EncodingSerialization_AllAvailable()
{
foreach (var info in Encoding.GetEncodings())
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 It would be nice to have this as a Theory that uses MemberData to provide each encoding by name.

333fred added a commit to 333fred/roslyn that referenced this pull request Jun 29, 2020
* upstream/master: (1226 commits)
  Remove unnecessary Clone() (dotnet#45469)
  Align addition of a synthesized override of object.Equals(object? obj) in records with the latest design. (dotnet#45475)
  Move SymbolSearch down to EditorFeatures (dotnet#45505)
  VisitType in MethodToClassRewriter for function pointers.
  Fix up nondeterminism in serializing naming style preferences
  Update dependencies from https://github.com/dotnet/arcade build 20200626.2 (dotnet#45482)
  Fix typo
  Move to vnext
  Add constant inerpolated strings to the test plan, update status for records.
  Don't emit ldftn when the result is unused.
  PR Feedback: * Additional tests for nested function contexts. * Override VisitFunctionPointerLoad in MethodToClassRewriter. * Adjust debug asserts.
  Add records to compiler test plan (dotnet#45434)
  Expand comment in CreateRecoverableText
  Replace binary serialization of encoding with a custom serializer. (dotnet#45374)
  LangVersion 9 (dotnet#44911)
  Avoid loading document text in AbstractObjectBrowserLibraryManager.DocumentChangedAsync
  Allow TryGetTextVersion to pass through to the initial source
  Ensure recoverable text is in temporary storage
  Fix test
  Updates the option page type GUID to match the one in pkgdef
  ...
333fred added a commit that referenced this pull request Jun 30, 2020
…e_168

* upstream/master: (102 commits)
  Change contrast ratio to get close to 1.5:1 (#45526)
  Revert "Move SymbolSearch down to EditorFeatures (#45505)"
  Delay accessibility checks to avoid cycles (#45441)
  Prevent trying to convert metadata references into circular project references
  Remove unnecessary Clone() (#45469)
  Align addition of a synthesized override of object.Equals(object? obj) in records with the latest design. (#45475)
  Move SymbolSearch down to EditorFeatures (#45505)
  VisitType in MethodToClassRewriter for function pointers.
  Fix up nondeterminism in serializing naming style preferences
  Update dependencies from https://github.com/dotnet/arcade build 20200626.2 (#45482)
  Fix typo
  Move to vnext
  Add constant inerpolated strings to the test plan, update status for records.
  Don't emit ldftn when the result is unused.
  PR Feedback: * Additional tests for nested function contexts. * Override VisitFunctionPointerLoad in MethodToClassRewriter. * Adjust debug asserts.
  Add records to compiler test plan (#45434)
  Expand comment in CreateRecoverableText
  Replace binary serialization of encoding with a custom serializer. (#45374)
  LangVersion 9 (#44911)
  Avoid loading document text in AbstractObjectBrowserLibraryManager.DocumentChangedAsync
  ...
@dibarbet dibarbet modified the milestones: Next, 16.7.P4 Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants