Skip to content

Fix delivery of zero-byte UDP datagrams#5347

Open
mvanhorn wants to merge 2 commits into
ponylang:mainfrom
mvanhorn:fix/5289-udp-recvfrom-zero-byte-datagrams
Open

Fix delivery of zero-byte UDP datagrams#5347
mvanhorn wants to merge 2 commits into
ponylang:mainfrom
mvanhorn:fix/5289-udp-recvfrom-zero-byte-datagrams

Conversation

@mvanhorn
Copy link
Copy Markdown
Contributor

Summary

pony_os_recvfrom no longer treats a zero-byte POSIX recvfrom as an error. Zero-byte UDP datagrams are now delivered to UDPNotify.received with an empty payload, matching RFC 768. TCP pony_os_recv behavior is unchanged.

Why this matters

RFC 768 says UDP datagrams "may be of any length greater than or equal to zero." Before this change, applications using zero-byte UDP datagrams as heartbeats, keepalives, or presence pings saw the socket torn down because the runtime called pony_error() on recvd == 0.

Maintainer @SeanTAllen called out the fix shape in the issue thread:

...in POSIX pony_os_recvfrom, when received == 0, return PONY_SOCKET_OK with count_out = 0 instead of falling into the recv-style peer-closed branch.

A note on the implementation: pony_os_recvfrom still returns size_t in this tree (no PONY_SOCKET_* enum visible in src/libponyrt/lang/socket.c). To match the existing pony_os_recv calling convention used elsewhere, this patch uses the existing (size_t)-1 / SIZE_MAX sentinel for "would block / no data" and lets recvd == 0 fall through as a normal len == 0 read. udp_socket.pony checks len == USize.max_value() to mean "no data yet" (replacing the prior len == 0 check, which now legitimately means "empty datagram"). If the three-state enum design has actually landed somewhere I missed, happy to rewrite to use it.

Testing

New PonyTest case _TestUDPZeroByteDatagram round-trips a zero-byte datagram through a bound UDP socket and asserts the notifier receives data.size() == 0. Existing TCP suite is untouched.

Fixes #5289

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label May 16, 2026
@SeanTAllen
Copy link
Copy Markdown
Member

please stop editing the next release release notes file directly.

@SeanTAllen SeanTAllen added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label May 17, 2026
@ponylang-main
Copy link
Copy Markdown
Contributor

Hi @mvanhorn,

The changelog - fixed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 5347.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

@SeanTAllen
Copy link
Copy Markdown
Member

please give this PR a title that is a good entry in the CHANGELOG

@SeanTAllen SeanTAllen added the do not merge This PR should not be merged at this time label May 17, 2026
…lang#5289)

Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
@mvanhorn mvanhorn force-pushed the fix/5289-udp-recvfrom-zero-byte-datagrams branch from 6d7620f to 4fc8c47 Compare May 17, 2026 22:56
@mvanhorn mvanhorn changed the title [Fix] UDP recvfrom treats zero-byte datagrams as an error (issue #5289) Fix delivery of zero-byte UDP datagrams May 17, 2026
@mvanhorn
Copy link
Copy Markdown
Contributor Author

@SeanTAllen thanks, both fixed in 4fc8c47:

  1. Retitled the PR to "Fix delivery of zero-byte UDP datagrams" so it reads as a CHANGELOG entry directly.
  2. Moved the release-note prose out of .release-notes/next-release.md into a new .release-notes/5347.md per the bot's instructions. next-release.md is back to its pre-PR state.

Sorry for the direct edit, won't happen again.

Two small follow-ups after a self-review pass with ponylang/llm-skills'
pony-code-review skill:

- packages/net/udp_socket.pony: expand the _pending_reads docstring to
  describe the actual budget semantics (4 KB of read work, with empty
  datagrams charged 1 byte each) and add an inline comment explaining
  the .max(1) DoS guard.
- packages/net/_test.pony: add the "unreliable-appveyor-osx" label on
  _TestUDPZeroByteDatagram, matching _TestBroadcast precedent for UDP
  tests that bind to the wildcard address and rely on local_address()
  delivery.
@mvanhorn
Copy link
Copy Markdown
Contributor Author

@SeanTAllen - I ran pony-code-review against this PR per your ask. The review came back clean on the fix itself: adversarial couldn't construct a scenario where the bug still occurs, correctness found no blocking issues, the test passes the counterfactual check against the two most obvious reverts. Pushed 85473d0 with three small polishes the review surfaced:

  • _pending_reads docstring rewritten to describe the actual budget semantics (4 KB of read work, with empty datagrams charged 1 byte each).
  • Inline comment on the new .max(1) explaining the DoS guard for a zero-byte datagram flood.
  • unreliable-appveyor-osx label on _TestUDPZeroByteDatagram matching _TestBroadcast precedent (same wildcard-bind + local_address() pattern, known to be flaky on AppVeyor/macOS).

Two parked items the review surfaced that I left for your call:

  • The .max(1) rate-limit fix is not directly tested by the single-datagram test - a future drive-by len.max(1) -> len would pass. Worth a multi-datagram test?
  • The error path (recvd < 0 with non-EAGAIN errno still calls pony_error()) is also untested. Worth covering, or out of scope for this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge discuss during sync Should be discussed during an upcoming sync do not merge This PR should not be merged at this time

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UDP recvfrom treats zero-byte datagrams as an error

3 participants