Fix UDP ENOBUFS handling and multicast interface quoting (#5486)#5680
Conversation
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).
There was a problem hiding this comment.
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
--interfacein UDP endpoint stringification when it contains parser separators (e.g., whitespace), fixing proxyice_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. |
| // 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()) |
There was a problem hiding this comment.
It is not clear why Windows include WSAEFAULT in the noBuffers, seems unrelated to the use we have in StreamSocket for it.,
There was a problem hiding this comment.
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.
Reword to attribute the separators correctly: whitespace is split by the endpoint parser, ':' by the proxy-string parser (review feedback).
bernardnormier
left a comment
There was a problem hiding this comment.
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.
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).
bernardnormier
left a comment
There was a problem hiding this comment.
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.
Addresses #5486 (items 9, 10). One of a set of PRs splitting the grouped transport audit.
ENOBUFSon 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::writenow drops the datagram instead of throwing. Limited to non-Windows, becausenoBuffers()also matchesWSAEFAULTthere, which must not be silently dropped.--interfacevalue 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 noUdpEndpointI(N/A).Item 10 has a red→green round-trip test in
Ice/proxyfor C++, C#, and Java;Ice/udppasses 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
ENOBUFSburst on UDP sends no longer closing the connection — is minor, consistent with how the #5679 fixes were treated. Codex-audited.