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

Fix field length for command packets #3715

Closed
wants to merge 6 commits into from
Closed

Conversation

Kowaman
Copy link
Contributor

@Kowaman Kowaman commented Jul 23, 2024

Updates the field length for ClientCommand and UnsingedClientCommand packets.

Resolves OverflowPacketException being thrown, I imagine this is only encountered with usage of the run_command chat click event, as players can't send a command this length in the chat bar.

https://wiki.vg/Protocol#Chat_Command

@Kowaman Kowaman changed the title Fix field Fix field length for command packets Jul 23, 2024
@roeii
Copy link

roeii commented Jul 23, 2024

Change looks good. 256 might be a left over?

@Outfluencer
Copy link
Collaborator

please take a look into all minecraft versions

the protocol wiki is not guaranteed to be right

For the ClientCommand packet take a look into the networking for all versions that packet is registered for

@Outfluencer
Copy link
Collaborator

Outfluencer commented Jul 23, 2024

i took a look on my own
image
Here you can see the limit for 1.19 is 256

so you sould respect that

@Kowaman
Copy link
Contributor Author

Kowaman commented Jul 23, 2024

Thanks for looking. It looks like it was increased with PVN 766, ie 1.20.5/1.20.6 -- how should I go about handling the difference between versions?

@Outfluencer
Copy link
Collaborator

Outfluencer commented Jul 23, 2024

Use the protocolVersion argument, check the version, and use the matching value

Like this:

readString( buf, protocolVersion >= ProtocolConstants.MINECRAFT_1_20_5 ? 32767 : 256 );

@Kowaman
Copy link
Contributor Author

Kowaman commented Jul 24, 2024

Should be good now! Thanks for the tip @Outfluencer

@md-5
Copy link
Member

md-5 commented Jul 24, 2024

Should probably be a ternary like the example Outfluencer gave in his comment

@Kowaman
Copy link
Contributor Author

Kowaman commented Jul 28, 2024

Should probably be a ternary like the example Outfluencer gave in his comment

@md-5 Sounds good -- was trying to match the existing style but if ternary is preferred can do that

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.

4 participants