Add UDP support to NetX sockets for DTLS sessions#10408
Conversation
|
Can one of the admins verify this patch? |
|
Okay to test. Contributor agreement on file. Thank you @hmohide |
|
#10414 Should fix the zephyr tests if you rebase to pull in the updated workflow |
|
|
@dgarskeI think above failure not triggered by the changes? Do i need to make some change? Please guide |
|
Hi @hmohide these would all be solved by a rebase against latest master and force push. Please go ahead and do that. Thanks |
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 9 total — 9 posted, 0 skipped
7 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] DTLS NetX receive maps timeouts to fatal error, breaking retransmission —
src/wolfio.c:2659-2665 - [Medium] wolfSSL_SetIO_NetX_Dtls stores caller pointers dereferenced much later (lifetime hazard) —
src/wolfio.c:2751-2759, wolfssl/internal.h:5719-5721 - [Medium] DTLS receive does not capture peer address (single fixed-peer limitation) —
src/wolfio.c:2644-2694 - [Medium] New public API and DTLS IO callbacks have no test coverage —
src/wolfio.c:2644-2759 - [Medium] New public API lacks doxygen; existing doxygen references renamed nxSocket field —
wolfssl/wolfio.h:798-801 - [Medium] DTLS NetX path requires NetX Duo APIs without a guard —
src/wolfio.c:2729-2745 - [Low] NetX_SendTo uses Allman braces and missing space after 'if' (style mismatch) —
src/wolfio.c:2729-2745 - [Low] Trailing whitespace will fail wolfSSL whitespace CI check —
src/wolfio.c:2640,2708,2736,2744,2754; wolfssl/internal.h:5718; wolfssl/wolfio.h:795,798-801 - [Low] Incorrect/misleading struct member comments (nxPort, nxUdpSocket) —
wolfssl/internal.h:5719-5721
Review generated by Skoll
dgarske
left a comment
There was a problem hiding this comment.
@hmohide Our internal AI review found several items to consider: #10408 (review)
|
@dgarske i have updated the review comments |
- Replace non-ASCII em-dash with ASCII hyphen in NetX_ReceiveFrom comment to pass wolfSSL encoding/whitespace CI check - Add WOLFSSL_NETX_DUO to .wolfssl_known_macro_extras so the macro checker no longer flags it as unrecognized
|
Jenkins retest this please "Build 'wolfSSL/PRB-fips-repo-and-harness-test-v3-part2' failed with result: FAILURE" |
There was a problem hiding this comment.
Pull request overview
This PR adds NetX Duo UDP I/O callback support so DTLS sessions can run over NetX sockets (in addition to existing NetX TCP/TLS support), and wires the appropriate callbacks/context initialization into the default wolfSSL initialization paths.
Changes:
- Add NetX Duo DTLS UDP I/O callbacks (
NetX_ReceiveFrom/NetX_SendTo) and a public configuration API (wolfSSL_SetIO_NetX_Dtls). - Update NetX context structure to track separate TCP vs UDP socket handles and UDP destination addressing.
- Select UDP callbacks automatically for DTLS methods in
InitSSL_Ctx()and ensure DTLS I/O contexts point atssl->nxCtxwhen NetX is enabled.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
wolfssl/wolfio.h |
Adds public API and local callback prototypes for NetX Duo DTLS UDP I/O. |
wolfssl/internal.h |
Extends NetX I/O context to distinguish TCP vs UDP and store DTLS destination addressing. |
src/wolfio.c |
Implements NetX Duo DTLS UDP send/receive callbacks and updates existing NetX TCP callbacks to use renamed context fields. |
src/internal.c |
Selects NetX DTLS UDP callbacks for DTLS contexts and fixes DTLS default I/O ctx initialization for NetX. |
doc/dox_comments/header_files/wolfio.h |
Updates NetX docs and adds Doxygen for the new DTLS NetX Duo UDP API. |
doc/dox_comments/header_files-ja/wolfio.h |
Japanese doc updates/additions corresponding to the NetX changes. |
.wolfssl_known_macro_extras |
Registers WOLFSSL_NETX_DUO as a known macro. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 5 total — 4 posted, 1 skipped
4 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] NetX_ReceiveFrom does not validate the datagram source peer —
src/wolfio.c:2645-2718 - [Medium] DTLS 1.3 quick-timeout can divide to zero and fall back to nxWait, disabling retransmission —
src/wolfio.c:2662-2675 - [Low] Zero-length datagram returns 0 instead of being ignored —
src/wolfio.c:2692-2717 - [Low] Redundant repeated wolfSSL_dtls_get_using_nonblock() calls —
src/wolfio.c:2662-2685
Skipped findings
- [Medium]
No test coverage for new NetX DTLS UDP callbacks
Review generated by Skoll
- Replace non-ASCII em-dash with ASCII hyphen in NetX_ReceiveFrom comment to pass wolfSSL encoding/whitespace CI check - Add WOLFSSL_NETX_DUO to .wolfssl_known_macro_extras so the macro checker no longer flags it as unrecognized
i have rebased the branch |
|
@dgarske i have rebased. can you please review and approve |
|
Jenkins retest this please. History lost |
|
The CI "Build 'wolfSSL/PRB-fips-repo-and-harness-test-v3-part2' failed with result: FAILURE" is failing only because you have this PR opened on master. We can accept with that known CI issue. We are in a release cycle, so this won't be merged for a few days. |
|
@dgarske any update on this PR. this one of the critical PR required for our relaese |
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 4 total — 3 posted, 1 skipped
3 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] NetX_ReceiveFrom does not reject negative sz before using it as an unsigned length —
src/wolfio.c:2668-2806 - [Low] Unreachable return at end of NetX_ReceiveFrom (dead code) —
src/wolfio.c:2821-2823 - [Low] Post-handshake invalid-peer/0-length flood is not rate-limited in blocking mode —
src/wolfio.c:2759-2795
Skipped findings
- [Low]
No test coverage for new DTLS NetX Duo I/O callbacks
Review generated by Skoll
Just a few minor items to review here -> #10408 (review) |
|
Okay to test. Thank you @hmohide |
|
Jenkins retest this please "ERROR: Error fetching remote repo 'origin'" |
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 4 total — 3 posted, 1 skipped
3 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] NetX_ReceiveFrom retains leftover datagram bytes, diverging from DTLS datagram semantics —
src/wolfio.c:2809-2832 - [Low] NetX_SendTo silently uses IPv6 path when destination address version is unset —
src/wolfio.c:2870-2878 - [Low] wolfSSL_SetIO_NetX_Dtls does not reset any buffered packet/offset state —
src/wolfio.c:2889-2899
Skipped findings
- [Medium]
No test coverage for new NetX Duo DTLS UDP callbacks
Review generated by Skoll
| continue; | ||
| } | ||
|
|
||
| if (nxCtx->nxOffset > total) { |
There was a problem hiding this comment.
🟠 [Medium] NetX_ReceiveFrom retains leftover datagram bytes, diverging from DTLS datagram semantics
When the caller's sz is smaller than the received datagram, the code copies sz bytes, advances nxCtx->nxOffset, keeps the packet, and returns copied without draining. The remainder of that datagram is then returned on the next call. This mirrors the TCP NetX_Receive byte-stream behavior, but for UDP/DTLS the standard reference EmbedReceiveFrom (recvfrom) delivers one datagram per call and discards any bytes beyond sz (datagram truncation semantics). Feeding the tail of an over-sized datagram back as a subsequent read could mis-frame DTLS records if the record layer ever requests fewer bytes than the datagram holds. In typical use wolfSSL passes a buffer large enough for a full record so this is an edge case, but the semantic difference from EmbedReceiveFrom is worth confirming.
Fix: Document/verify the leftover-retention choice for DTLS or switch to draining the packet each call so a single datagram is never split across reads.
| return WOLFSSL_CBIO_ERR_GENERAL; | ||
| } | ||
|
|
||
| if (nxCtx->nxdIp.nxd_ip_version == NX_IP_VERSION_V4) { |
There was a problem hiding this comment.
🔵 [Low] NetX_SendTo silently uses IPv6 path when destination address version is unset
The send path branches on nxCtx->nxdIp.nxd_ip_version == NX_IP_VERSION_V4, and the else falls through to nxd_udp_socket_send(). If a send is attempted before a peer address has been established (e.g. a server that sends before receiving a ClientHello, or an application that forgot to call wolfSSL_SetIO_NetX_Dtls with a valid peer), nxdIp.nxd_ip_version is 0 (zero-initialized), so the IPv6 branch runs with an all-zero NXD_ADDRESS. This fails gracefully (returns WOLFSSL_CBIO_ERR_GENERAL) rather than crashing, but the failure is opaque. In normal DTLS flows the peer is always locked before a send, so this is edge-case robustness only.
Fix: Optionally validate that nxdIp.nxd_ip_version is V4 or V6 before selecting the send path, to make misuse produce a clear diagnostic.
| } | ||
|
|
||
| /* like set_fd, but for default NetX Duo UDP context */ | ||
| void wolfSSL_SetIO_NetX_Dtls(WOLFSSL* ssl, NX_UDP_SOCKET* nxsocket, |
There was a problem hiding this comment.
🔵 [Low] wolfSSL_SetIO_NetX_Dtls does not reset any buffered packet/offset state
The setter assigns nxUdpSocket, nxdIp, nxPort, and nxWait but leaves nxPacket/nxOffset untouched. On a freshly allocated WOLFSSL the nxCtx is zero-initialized so this is harmless in the normal one-shot setup case. However, if the API is ever called to re-point IO mid-session while a partially-drained packet is buffered, that stale packet/offset would be applied to the new socket. This matches the existing wolfSSL_SetIO_NetX (TCP) behavior, so it is a consistency observation rather than a new defect.
Fix: Leave as-is for parity with wolfSSL_SetIO_NetX, or reset nxPacket/nxOffset if mid-session re-binding is intended to be supported.
Description
Add UDP support to NetX sockets for DTLS sessions
Fixes zd#
src/internal.c
src/wolfio.c
wolfssl/internal.h
wolfssl/wolfio.h
Testing
Yes, verified the DTLS communication with multiple session
Checklist