Skip to content

Fix OptionalNbt framing: VarInt byte length, not bool presence - #335

Open
etz wants to merge 2 commits into
azalea-rs:mainfrom
etz:fix/optional-nbt-varint-length-clean
Open

Fix OptionalNbt framing: VarInt byte length, not bool presence#335
etz wants to merge 2 commits into
azalea-rs:mainfrom
etz:fix/optional-nbt-varint-length-clean

Conversation

@etz

@etz etz commented Jun 4, 2026

Copy link
Copy Markdown

The serverbound custom_click_action payload is currently encoded as bare unnamed NBT, but vanilla expects a VarInt byte length followed by unnamed NBT (with 0 meaning absent). Empty payloads happen to round-trip because Nbt::None writes 0x00 and that's also the VarInt zero-length sentinel, which is probably why this hasn't surfaced before — any non-empty submission disconnects.

Repro on a vanilla 26.1.2 server with an OP'd offline user and a tiny minecraft:notice dialog datapack with a text input. Submitting the dialog from unpatched azalea:

nbtazaleaprobe lost connection: Internal Exception: io.netty.handler.codec.DecoderException:
java.io.IOException: Packet play/serverbound/minecraft:custom_click_action was larger than
I expected, found 145 bytes extra whilst reading packet serverbound/minecraft:custom_click_action

Outgoing payload after the action id, unpatched vs. patched:

unpatched: ... 10 61 7a 61 6c 65 61 74 65 73 74 3a 70 72 6f 62 65 0a 08 00 05 ...
patched:   ... 10 61 7a 61 6c 65 61 74 65 73 74 3a 70 72 6f 62 65 9c 01 0a 08 00 05 ...

9c 01 is the VarInt for the 156-byte NBT payload that follows.

The fix adds an OptionalNbt newtype whose AzBuf impl reads/writes the VarInt-length framing, and switches both s_custom_click_action.rs (config + game) to use it.

Regression tests live in azalea-protocol/tests/optional_nbt.rs (minimal present payload, absent payload, vanilla-shaped dialog payload with args / cmd / field keys).

The first commit also contains a few mechanical clippy fixes in unrelated existing code — without them cargo clippy -p azalea-protocol --all-targets -- -D warnings doesn't pass on the base, so the new tests can't be gated on it cleanly. Happy to drop that commit if you'd rather it land separately.

One thing worth a second pair of eyes: I only found two packets that use this framing today. If you know of others that should switch to OptionalNbt, point me at them.

@etz
etz force-pushed the fix/optional-nbt-varint-length-clean branch from a1858f3 to 4e972d9 Compare June 4, 2026 22:08
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.

1 participant