Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use vanilla limit for KnownPacks #1425

Closed
wants to merge 1 commit into from

Conversation

Outfluencer
Copy link
Contributor

@Outfluencer Outfluencer commented Sep 6, 2024

vanilla has not limit for it in latest, its limit is the limit of the collection decode so its 65536

vanilla has not limit for it in latest, its limit is the limit of the collection decode so its 65535
@Outfluencer
Copy link
Contributor Author

Outfluencer commented Sep 6, 2024

Proofs
image

image (1)

image (2)

@Outfluencer Outfluencer changed the title change default limit of KnownPacks change default limit of KnownPacks to vanilla limit Sep 6, 2024
@Outfluencer Outfluencer changed the title change default limit of KnownPacks to vanilla limit use vanilla limit for KnownPacks Sep 6, 2024
@Timongcraft
Copy link
Contributor

This is also limited as a security measure and not only to match vanilla behaviour. View #1303 for details.

@Outfluencer
Copy link
Contributor Author

Outfluencer commented Sep 6, 2024

This is also limited as a security measure and not only to match vanilla behaviour. View #1303 for details.

65536 is as secure as 64

You cant make the server go out of memory with an array of the size 65536.

Every sting in velocity can be that size

the limit is only about preventing sending Integer.MAX_VALUE as size, but such small values are secure anyway

@Timongcraft
Copy link
Contributor

Timongcraft commented Sep 6, 2024

it is about the packs and not the size of packs.

@Outfluencer
Copy link
Contributor Author

Outfluencer commented Sep 6, 2024

okay what risks do i encounter if the limit is at 65536, compared to 64?

@Outfluencer
Copy link
Contributor Author

Outfluencer commented Sep 6, 2024

as the data is limited to 2 mb or 6 mb with compression anyways there is litrally no security risk with using the vanilla limit here

@Outfluencer
Copy link
Contributor Author

Outfluencer commented Sep 6, 2024

it is about the packs and not the size of packs.

Also i think you don't understand its about the array allocation and not the KnownPacks amount itself
The problem was that you could allocate the following: new KnownPack[2000000000] in that case the jvm would throw an OutOfMemoryException or crash.

Just so you can understand better 65536 is 30517,57 times smaller, so it cant cause any out of memory errors or performace issues

@UserNugget
Copy link
Contributor

vanilla has not limit for it in latest, its limit is the limit of the collection decode so its 65536

Its still in the latest vanilla version. You are looking at clientbound limit, but there are also a serverbound one.
And it seems that i forgot to add clientbound limit in #1303 👍

@Outfluencer
Copy link
Contributor Author

vanilla has not limit for it in latest, its limit is the limit of the collection decode so its 65536

Its still in the latest vanilla version. You are looking at clientbound limit, but there are also a serverbound one. And it seems that i forgot to add clientbound limit in #1303 👍

You are right, my friend send me the wrong packet.

    STREAM_CODEC = StreamCodec.composite(KnownPack.STREAM_CODEC.apply(ByteBufCodecs.list(64)), ServerboundSelectKnownPacks::knownPacks, ServerboundSelectKnownPacks::new);

@Outfluencer Outfluencer closed this Sep 6, 2024
@Outfluencer Outfluencer deleted the patch-2 branch September 6, 2024 16:31
@Timongcraft
Copy link
Contributor

At least I understand it correctly now, thanks

@UserNugget
Copy link
Contributor

vanilla has not limit for it in latest, its limit is the limit of the collection decode so its 65536

Its still in the latest vanilla version. You are looking at clientbound limit, but there are also a serverbound one. And it seems that i forgot to add clientbound limit in #1303 👍

You are right, my friend send me the wrong packet.

    STREAM_CODEC = StreamCodec.composite(KnownPack.STREAM_CODEC.apply(ByteBufCodecs.list(64)), ServerboundSelectKnownPacks::knownPacks, ServerboundSelectKnownPacks::new);

No need to close this PR, KnownPacksPacket is used for both clientbound and serverbound directions, so this change is still useful (but should be changed to affect only clientbound direction).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants