Add BinaryFormatter auditing EventSource #39874
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GrabYourPitchforks
requested review from
blowdart,
noahfalk,
bartonjs,
tarekgh and
joperezr
tarekgh
reviewed
Jul 24, 2020
....Formatters/src/System/Runtime/Serialization/Formatters/Binary/BinaryFormatterEventSource.cs
Outdated
Show resolved
Hide resolved
tarekgh
reviewed
Jul 24, 2020
src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterEventSourceTests.cs
Outdated
Show resolved
Hide resolved
jkotas
reviewed
Jul 24, 2020
....Formatters/src/System/Runtime/Serialization/Formatters/Binary/BinaryFormatterEventSource.cs
Outdated
Show resolved
Hide resolved
jkotas
reviewed
Jul 24, 2020
....Formatters/src/System/Runtime/Serialization/Formatters/Binary/BinaryFormatterEventSource.cs
Outdated
Show resolved
Hide resolved
jkotas
reviewed
Jul 24, 2020
....Runtime.Serialization.Formatters/tests/System.Runtime.Serialization.Formatters.Tests.csproj
Outdated
Show resolved
Hide resolved
noahfalk
reviewed
Jul 28, 2020
....Formatters/src/System/Runtime/Serialization/Formatters/Binary/BinaryFormatterEventSource.cs
Outdated
Show resolved
Hide resolved
| [Event(EventId_SerializationStarted, Opcode = EventOpcode.Start, Keywords = Keywords.Serialization, Level = EventLevel.Informational)] | ||
| public void SerializationStarted() | ||
| { | ||
| if (IsEnabled(EventLevel.Informational, Keywords.Serialization) && !_writeInProgress.Value) |
There was a problem hiding this comment.
Nit: I typically suggest people not to worry about adding IsEnabled() checks unless you are on a very hot code-path trying to shave a few nanoseconds. The implementation within WriteEvent() will do an appropriate check.
There was a problem hiding this comment.
Followed up offline. Will leave them in for now since at least one of the operations (Type.get_AssemblyQualifiedName) guarded by the initial check might have side effects that we don't want to run if nobody is listening.
....Formatters/src/System/Runtime/Serialization/Formatters/Binary/BinaryFormatterEventSource.cs
Outdated
Show resolved
Hide resolved
....Formatters/src/System/Runtime/Serialization/Formatters/Binary/BinaryFormatterEventSource.cs
Outdated
Show resolved
Hide resolved
tarekgh
approved these changes
Jul 29, 2020
jkotas
approved these changes
Jul 29, 2020
Jacksondr5
pushed a commit
to Jacksondr5/runtime
that referenced
this pull request
Aug 10, 2020
ghost
locked as resolved and limited conversation to collaborators
jkotas
added
the
binaryformatter-migration
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the last of the 5.0 runtime changes per the
BinaryFormatterobsoletion document. This introduces a newEventSourceused by the serialization infrastructure to tell you when calls toBinaryFormatter.SerializeorBinaryFormatter.Deserializetake place. The are currently 6 events raised in total:Type.AssemblyQualifiedNameis provided as an arg)Type.AssemblyQualifiedNameis provided as an arg)This feature is not a "global
SerializationBinder" or a "global surrogate selector" and cannot be used to substitute types at runtime. Rather, as we begin to wind downBinaryFormatterwithin the runtime and libraries, it's meant to help app authors discover hiddenBinaryFormatterdependencies within their own code or within any assemblies they pull into their apps.There is an open question as to whether it would be useful to port this feature back to Full Framework as part of an overall defense-in-depth mechanism. I'm not considering that at the moment, but this code was designed such that it can be easily backported to Full Framework if needed. The code to hook up an
EventListenerwould look the same both in Full Framework and in .NET 5.0+.