Cap array allocations in ProtocolUtils readers#1788
Open
AlexMelanFromRingo wants to merge 1 commit intoPaperMC:dev/3.0.0from
Open
Cap array allocations in ProtocolUtils readers#1788AlexMelanFromRingo wants to merge 1 commit intoPaperMC:dev/3.0.0from
AlexMelanFromRingo wants to merge 1 commit intoPaperMC:dev/3.0.0from
Conversation
06245ab to
61256fa
Compare
61256fa to
dfbeaa9
Compare
Member
|
Restricting these from strictly server-defined data is not a really concern. Why would you crash your own proxy or player? applying this limit unconditionally will also potentially hit real data at some point |
Author
|
Fair on the single-backend case. I was thinking more about proxies fronting multiple backends, where one bad backend shouldn't be able to take out players connected to the others. If you don't see value there, happy to close this. Otherwise I can rework it as an opt-in system property (like velocity.max-known-packs) so default behavior is unchanged. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
readByteArrayalready takes acapand rejects oversized counts, butreadKeyArray,readIntegerArray,readStringArrayandreadVarIntArrayonly check that the count fits in the remaining buffer bytes. Since each entry is 4 to 8 bytes (a primitive int or an Object reference) but the buffer check is in bytes, a single varint count of around the frame size lets a peer push us into a 4x to 8x larger heap allocation than the wire bytes.For example
readIntegerArraywas reachable fromAvailableCommandsPacket(clientbound). Setting the array length to ~2M passes theisReadable(length)check and we thennew int[~2M], an 8 MiB allocation per packet. Similar story for the other three.Adds a
capparameter to each of the four readers, defaults toDEFAULT_MAX_STRING_SIZE(the same constantreadByteArrayuses) and rejects with the existingcheckFrameif the count exceeds it. Tests for the cap path are added../gradlew :velocity-proxy:test :velocity-proxy:checkstyleMainclean.