Enforce 64KB event payload size limit on EventPipe #50600
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
josalem
reviewed
Apr 1, 2021
| @@ -897,6 +897,9 @@ ep_buffer_manager_write_event ( | |||
| // Before we pick a buffer, make sure the event is enabled. | |||
| ep_return_false_if_nok (ep_event_is_enabled (ep_event)); | |||
|
|
|||
| // Check that the payload size is less than 64 KB (max size for ETW events) | |||
| ep_return_false_if_nok (ep_event_payload_get_size (payload) <= 64 * 1024); | |||
There was a problem hiding this comment.
ETW might limit its payloads to 64KB, but does EventPipe need to? EventSource doesn't explicitly prevent you from having payloads this large, e.g., your test.
There was a problem hiding this comment.
It doesn't. But ETW will drop the event if it's greater than 64KB and I'd like to enforce the same behavior here since:
- We need some kind of limit regardless. Currently >100KB payloads crash the app. We could enforce a 100KB limit instead of 64KB.
- But if we do enforce 100KB limit, that leads to a discrepancy between ETW/EventPipe behavior which we then need to go and figure out how customers can find out whether big events are getting dropped on ETW but not on EventPipe and document why such behavior differences exist.
- Another approach is to enforce 1MB payload (which we currently do - just not explicitly but because we just fail to allocate buffers greater than that) and increase the max flushing block size to 1MB but that's a pretty inefficient use of space for sessions listening on much smaller-sized payloads (i.e. counters).
There was a problem hiding this comment.
Fair point. Let me mull this over, but I think 64KB is fine. Should we remove the DebugBreak in the EP_UNLIKELY macro expansion?
There was a problem hiding this comment.
I'd actually like to keep that. It lets us catch exceptions like this rather than silently failing and us never noticing the failures.
iSazonov
reviewed
Apr 2, 2021
noahfalk
approved these changes
Apr 3, 2021
josalem
approved these changes
Apr 5, 2021
|
I'm merging this since CI failure is due to #50753. |
thaystg
added a commit
to thaystg/runtime
that referenced
this pull request
Apr 6, 2021
…shim_mono # By Aaron Robinson (10) and others # Via GitHub * upstream/main: (108 commits) [mbr] Add Apple sample (dotnet#50740) make EstablishProxyTunnelAsync throw on failure status code from proxy (dotnet#50763) Improve RGB Min Max evaluation performance by using 2 or 3 comparison… (dotnet#50622) [mono] More domain cleanups (dotnet#50479) Fix Crossgen2 of PlatformDefaultMemberFunction methods and calls. (dotnet#50754) Disable EventSource generator in design-time builds (dotnet#50741) Fix X509 test failures on Android (dotnet#50301) Do not confuse fgDispBasicBlocks in fgMorphBlocks (dotnet#50703) Enforce 64KB event payload size limit on EventPipe (dotnet#50600) Reorganize CoreCLR native build to reduce CMake reconfigures when the build system is untouched (dotnet#49906) [mbr] Turn on hot reload for iOS, tvOS and MacCatalyst (dotnet#50458) improve connection scavenge logic by doing zero-byte read (dotnet#50545) Resolve call mdtokens when making tier 1 inline observations (dotnet#50675) Annotate APIs in System.Private.Xml (dotnet#49682) Support compiling against OpenSSL 3 headers Change Configuration.Json to use a regular Dictionary. (dotnet#50611) Remove unused BigNumFromBinary P/Invoke (dotnet#50670) Make Ninja the default CMake generator on Windows for the repo (dotnet#49715) [AppleAppBuilder] Entitlements to run tests on catalyst using the JIT (dotnet#50637) [mono] Fix delegate invokes to dynamic methods in mixed mode. (dotnet#50547) ... # Conflicts: # src/mono/dlls/mscordbi/CMakeLists.txt
ghost
locked as resolved and limited conversation to collaborators
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.
ETW has a maximum of 64KB event payload size limit that is not enforced on EventPipe. This causes issues like #50515 to occur where an event that's greater than 100KB can cause the flushing thread's event block to be too small to fit a single event payload, which crashes the application.