Skip to content

Fix UDP ENOBUFS handling and multicast interface quoting (#5486)#5680

Merged
pepone merged 6 commits into
zeroc-ice:mainfrom
pepone:fix/5486-udp
Jun 24, 2026
Merged

Fix UDP ENOBUFS handling and multicast interface quoting (#5486)#5680
pepone merged 6 commits into
zeroc-ice:mainfrom
pepone:fix/5486-udp

Conversation

@pepone

@pepone pepone commented Jun 23, 2026

Copy link
Copy Markdown
Member

Addresses #5486 (items 9, 10). One of a set of PRs splitting the grouped transport audit.

  • Item 9 (C++/Unix): a transient ENOBUFS on a UDP send is routine on macOS/BSD during bursts — a transient local condition, not a connection failure. It was treated as fatal and closed the connection; since UDP is best-effort, UdpTransceiver::write now drops the datagram instead of throwing. Limited to non-Windows, because noBuffers() also matches WSAEFAULT there, which must not be silently dropped.
  • Item 10 (C++, C#, Java): a multicast --interface value was quoted only when it contained :, so a Windows adapter friendly name with a space (e.g. "Ethernet 2") was emitted unquoted and failed to re-parse. It is now quoted whenever it contains a separator the parser splits on (whitespace or :). The same bug existed in all three mappings; JS has no UdpEndpointI (N/A).

Item 10 has a red→green round-trip test in Ice/proxy for C++, C#, and Java; Ice/udp passes with no regression.

No changelog: with the catastrophic server-adapter case (bidirectional UDP, slated for removal in #5362) out of scope, item 9's remaining impact — a transient ENOBUFS burst on UDP sends no longer closing the connection — is minor, consistent with how the #5679 fixes were treated. Codex-audited.

A transient ENOBUFS on a datagram send (routine on macOS/BSD during
bursts) was treated as a fatal SocketException, closing the connection.
For an incoming datagram adapter this destroyed the single, never
recreated shared connection. UdpTransceiver::write now drops the
datagram on ENOBUFS instead, consistent with UDP best-effort semantics.

The multicast --interface value was quoted only when it contained ':',
so a Windows adapter friendly name with a space (e.g. "Ethernet 2") was
emitted unquoted and failed to re-parse. It is now quoted whenever it
contains a separator the endpoint parser would split on.

Addresses zeroc-ice#5486 (items 9, 10).
Copilot AI review requested due to automatic review settings June 23, 2026 12:52
@pepone pepone added the cpp label Jun 23, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses two transport-layer correctness issues in the C++ implementation related to UDP behavior and proxy string round-tripping, as tracked in #5486 (items 9 and 10).

Changes:

  • Treat transient UDP ENOBUFS (non-Windows) as a best-effort drop rather than a fatal error that closes the UDP connection.
  • Quote --interface in UDP endpoint stringification when it contains parser separators (e.g., whitespace), fixing proxy ice_toString() round-trip for Windows-friendly adapter names.
  • Add a proxy round-trip test covering an interface name containing a space.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
cpp/test/Ice/proxy/AllTests.cpp Adds a round-trip test ensuring --interface "Ethernet 2" survives ice_toString() re-parse.
cpp/src/Ice/UdpTransceiver.cpp Drops a datagram on non-Windows ENOBUFS instead of throwing/closing the UDP connection.
cpp/src/Ice/UdpEndpointI.cpp Updates quoting logic for --interface to include whitespace and other separators.

Comment thread cpp/src/Ice/UdpEndpointI.cpp Outdated
Comment thread cpp/src/Ice/UdpTransceiver.cpp
Comment thread cpp/src/Ice/UdpTransceiver.cpp
// connection, and a server's UDP adapter uses a single datagram socket that Ice never re-creates, so
// closing it would permanently stop the adapter from receiving datagrams. Limited to non-Windows: on
// Windows noBuffers() also matches WSAEFAULT, which must not be silently dropped.
if (noBuffers())

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is not clear why Windows include WSAEFAULT in the noBuffers, seems unrelated to the use we have in StreamSocket for it.,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

recv doesn't return WSAENOBUFS — only WSAEFAULT (send returns both). So in noBuffers() (which drives the StreamSocket read/write halve-and-retry), WSAEFAULT is the recv-side half of the pair. But retrying it isn't actually required: halving the length can't validate a bad pointer, and we always pass a correctly-sized buffer. Looks defensive/legacy (no rationale in git, predates 2012), and it's harmless only because it degrades to a throw. Unrelated to this UDP send path, which is why the drop stays non-Windows for now.

@pepone pepone requested a review from bernardnormier June 23, 2026 16:58
Reword to attribute the separators correctly: whitespace is split by the
endpoint parser, ':' by the proxy-string parser (review feedback).

@bernardnormier bernardnormier left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The two fixes are correct. A few requests before approval:

1. The ENOBUFS comment is confusing. It's in write() (sending), but justifies the change via a server's UDP adapter receiving datagrams. The only way a server reaches write() is bidirectional UDP (the sendto(_peerAddr) reply path) over the socket it also receives on — and that feature is undocumented, half-implemented, and slated for removal (#5362). So the rationale shouldn't lean on it. The best-effort argument stands on its own (and covers the common client-send case too). Suggestion inline.

2. PR description — same issue: please drop the "destroying the never-recreated shared incoming-datagram connection" framing (that's the bidir/server-adapter story being removed in #5362) and justify item 9 on the client best-effort send path instead.

3. No changelog needed. With the catastrophic server-adapter case going away, item 9's remaining impact (a transient ENOBUFS burst on UDP sends no longer closing the connection) is minor, consistent with how we treated the #5679 fixes.

4. Cross-mapping: item 10 is duplicated in C# and Java — both quote the multicast interface on : only and have the identical round-trip bug for a name with a space:

  • csharp/src/Ice/Internal/UdpEndpointI.cs: _mcastInterface.Contains(':', ...)
  • java/.../com/zeroc/Ice/UdpEndpointI.java: _mcastInterface.indexOf(':') != -1

Please fix both in this same PR (JS has no UdpEndpointI, so it's N/A). Item 9 is C++/Unix-specific and doesn't need a parallel change here.

5. Test nit — drop the (issue #5486 item 10) tag; tests should stand on their own. Suggestion inline.

Comment thread cpp/src/Ice/UdpTransceiver.cpp Outdated
Comment thread cpp/test/Ice/proxy/AllTests.cpp Outdated
pepone and others added 3 commits June 24, 2026 19:25
Co-authored-by: Bernard Normier <bernard@zeroc.com>
Co-authored-by: Bernard Normier <bernard@zeroc.com>
The C++ fix quoted the interface only on ':'; C# and Java had the identical
round-trip bug, where a Windows adapter friendly name with a space was emitted
unquoted by options() and failed to re-parse. Quote the interface whenever it
contains a separator the parser splits on (whitespace or ':'), matching C++,
and add the parallel "Ethernet 2" round-trip test in Ice/proxy.

JS has no UdpEndpointI (N/A).
@pepone pepone requested a review from bernardnormier June 24, 2026 17:42

@bernardnormier bernardnormier left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All review points addressed: the ENOBUFS comment is now justified on best-effort UDP grounds (no bidir/server-adapter narrative), the description is reworked, item 10 is fixed in C# and Java alongside C++ (matching separator sets, with round-trip tests in all three), the issue-number test tag is gone, and the no-changelog rationale is sound. Verified the C#/Java quoting matches the C++ set. LGTM.

@pepone pepone merged commit de324b4 into zeroc-ice:main Jun 24, 2026
46 checks passed
pepone added a commit that referenced this pull request Jul 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants