Skip to content

dtls: defer oversized poll outputs#138

Open
zRedShift wants to merge 1 commit into
algesten:mainfrom
zRedShift:fix/poll-output-small-buffer
Open

dtls: defer oversized poll outputs#138
zRedShift wants to merge 1 commit into
algesten:mainfrom
zRedShift:fix/poll-output-small-buffer

Conversation

@zRedShift
Copy link
Copy Markdown
Contributor

poll_output currently assumes the caller-provided buffer is large enough for
queued packets, queued application data, and peer-certificate events. If the
buffer is too small, those paths can panic instead of letting the caller grow
the buffer and retry.

This makes oversized output return Output::BufferTooSmall { needed } without
dropping the pending item. A later call with a large enough buffer emits the
same packet/event/data.

Line delta:

area added removed
non-test code 178 76
tests 334 22
docs/changelog 5 0
total 517 98

Validation:

  • cargo fmt --check
  • git diff --check
  • git diff --check upstream/main...HEAD
  • /home/ronen/.codex/skills/dimpl/scripts/check-snowflake-local.pl upstream/main
  • focused DTLS 1.2 / DTLS 1.3 / auto small-buffer regressions
  • cargo test --all-targets --features rcgen
  • cargo clippy --all-targets --features rcgen -- -D warnings
  • cargo test --no-default-features --features rust-crypto
  • cargo clippy --no-default-features --features rust-crypto -- -D warnings
  • cargo test --doc --features rcgen

Copy link
Copy Markdown
Owner

@algesten algesten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying we shouldn't land this, but I also wonder if it unnecessarily complicates the protocol.

Since it's UDP we are going to be hard limited by MTU, and it would be a strange setup to set MTU to one value and then hope to use a smaller output buffer only to run into this error straight away in the handshake (since a certificate is almost always split over multiple packets).

Are there reasonable scenarios where the output buffer is smaller than MTU?

@zRedShift
Copy link
Copy Markdown
Contributor Author

@algesten yeah, this is trying to address a panic, but I agree it’s basically misuse/user error.

My preference would be a type-protection pattern instead: make poll_output take a dimpl-validated output buffer newtype wrapping &mut [u8], where construction verifies the buffer is at least MTU-sized. Then we don’t need an extra Output variant that most callers probably won’t handle anyway.

That is another API break though. I can rework this PR that way for inspection, or open a separate one.

@algesten
Copy link
Copy Markdown
Owner

algesten commented May 31, 2026

As a personal preference, I don't particularly mind panics for broken invariants. Setting one MTU and using a smaller sized buffer, is a user error - not even a runtime error.

We could even go the other way and make an explicit assert! (or panic!) on this to ensure a developer catches it earlier with a better error message.

Of course making errors impossible through type system is even better, but I don't find the gain so big here that I think it warrants the "type noise" in signatures.

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.

2 participants