Repository navigation
Conversation
|
There was a problem hiding this comment.
🟡 Changes recommended
The regression test lacks a client PSK callback, causing PSK-only DTLS 1.2 builds with TLS 1.3 enabled to return SUITES_ERROR instead of the expected WOLFSSL_ERROR_WANT_READ.
1 open issue
What changed in this PR
This PR corrects wolfSSL’s default I/O contexts so TLS objects without file descriptors fail rather than access stdin, while preserving DTLS and platform-specific contexts.
Changes:
- Select default I/O contexts according to the protocol and platform.
- Delegate OpenSSL-compatible read/write descriptor setters to the DTLS-aware implementations.
- Add regression tests for unset TLS descriptors and DTLS descriptor setters.
| File | Description |
|---|---|
tests/api/test_tls.h |
Registers the unset-descriptor regression test. |
tests/api/test_tls.c |
Checks default contexts and prevents unintended stdin I/O. |
tests/api/test_dtls.h |
Registers the DTLS descriptor-setter test. |
tests/api/test_dtls.c |
Tests DTLS handshake initiation through compatibility setters. |
src/ssl.c |
Reuses DTLS-aware descriptor setters. |
src/internal.c |
Selects protocol-appropriate default I/O contexts. |
Review effort: Balanced
|
@claude review |
|
@wolfSSL-Fenrir-bot review |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11956
Scan targets checked: wolfssl-src, wolfssl-bugs
Not reviewed (no scan target covers them): tests/api/test_dtls.h, tests/api/test_tls.h
Coverage: 2 of 4 in-scope changed file(s) opened by the reviewer; not opened: tests/api/test_dtls.c, tests/api/test_tls.c
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It changes the default I/O context for every WOLFSSL object in DTLS-enabled builds, which includes --enable-all, so a maintainer should still look at it before merge.
What was reviewed:
InitSSL: NetX, Mynewt and GNRC still get their port contexts. Without those ports, TLS objects default to&ssl->rfd/&ssl->wfd, both initialized to -1 just above, and only methods withDTLS_MAJORgetdtlsCtx.wolfSSL_set_rfd/wolfSSL_set_wfdnow wrapwolfSSL_set_read_fd/wolfSSL_set_write_fd. On DTLS objects,set_wfdnow updatesdtlsCtx.wfd. Both setters now also NULL-check and resetdtlsCtx.connected, the same aswolfSSL_set_fd.- Both new tests restore fd 0, close their sockets on every path, and use C90 declarations.
Extended reasoning...
The change touches the default I/O callback context setup in InitSSL (src/internal.c), the OpenSSL-compat fd setters in src/ssl.c, and adds two API tests. It doesn't touch crypto, auth or parsing, but it changes the behavior of the core I/O path for every SSL object in DTLS-enabled builds. The diff is small, and in the configurations I traced it looks correct. I deferred because this changes a library-wide default. There is also an inline Copilot comment on the new DTLS test whose content and resolution I can't see.
|
Jenkins retest this please |
| ssl->IOCB_WriteCtx = &ssl->wfd; /* correctly set */ | ||
| #ifdef WOLFSSL_DTLS | ||
| if (ctx->method->version.major == DTLS_MAJOR) { | ||
| ssl->IOCB_ReadCtx = &ssl->buffers.dtlsCtx; |
There was a problem hiding this comment.
Aren't these the same as above?
#elif defined(WOLFSSL_DTLS)
...
#else
....
#endif
There was a problem hiding this comment.
Flattened into the #elif chain. The DTLS branch still has to check the method, since giving TLS objects dtlsCtx here is what made an unset-fd TLS object do its I/O on fd 0.
|
Conflicts to be resolved as well. |
2047f43 to
2e73597
Compare
With WOLFSSL_DTLS compiled in, InitSSL pointed IOCB_ReadCtx and IOCB_WriteCtx at ssl->buffers.dtlsCtx for every WOLFSSL, TLS ones included. EmbedReceive and EmbedSend read the socket as *(int*)ctx, and the first member of WOLFSSL_DTLS_CTX is the zeroed peer address, so a TLS object that never had an fd set did its I/O on fd 0 instead of failing on -1. When stdin is an idle socket, an accept or connect without an fd blocks forever; test_tls13_apis hits this through wolfSSL_read_early_data on a server with no fd. Choose the default IO context in one place for builds with and without DTLS. NetX and GNRC keep their port context, and the socket default is &ssl->rfd/&ssl->wfd, or the DTLS context when the method is DTLS. GNRC objects in builds with DTLS now get the port context their I/O callbacks expect instead of the DTLS one. The new test checks the default contexts of a TLS and a DTLS object, and puts a socket on fd 0 to check that a client with no fd, and one with only a write fd, fail with SOCKET_ERROR_E without writing to or reading from fd 0.
The OpenSSL compat wolfSSL_set_wfd only stored the fd in ssl->wfd and pointed IOCB_WriteCtx at it, also for DTLS objects. EmbedSendTo casts the write context to WOLFSSL_DTLS_CTX, so it read the socket, the peer address and the sendto callback from the WOLFSSL fields that follow wfd. wolfSSL_set_rfd handled DTLS but did not reset dtlsCtx.connected, and neither checked ssl for NULL. Make both wrappers around wolfSSL_set_read_fd and wolfSSL_set_write_fd, which select the DTLS context and fill it in.
Flatten the default IO context selection in InitSSL into one #elif chain instead of nesting the DTLS check inside the #else branch.
2e73597 to
7c8123c
Compare
|
Jenkins retest this please |
With
WOLFSSL_DTLScompiled in,InitSSLset the default IO context of everyWOLFSSL, TLS included, to&ssl->buffers.dtlsCtx.EmbedReceive/EmbedSendread that as*(int*)ctx, and the first member of the DTLS context is zero, so a TLS object with no fd did its I/O on fd 0 (stdin) instead of failing. With stdin as an idle socket,test_tls13_apishangs inwolfSSL_read_early_data.&ssl->rfd/&ssl->wfd, or the DTLS context only for DTLS methods. NetX, Mynewt and GNRC keep their port context, also in DTLS builds.wolfSSL_set_rfd/wolfSSL_set_wfdnow wrapwolfSSL_set_read_fd/wolfSSL_set_write_fd.set_wfdon a DTLS object pointed the write context atssl->wfd, whichEmbedSendTothen read as aWOLFSSL_DTLS_CTX.Testing
test_tls_unset_fd_no_stdin_io: puts a socket on fd 0 and checks a client with no fd, and one with only a write fd, fail withSOCKET_ERROR_Ewithout touching fd 0; checks the default contexts of TLS and DTLS objects.test_dtls_set_rfd_wfd: DTLS handshake start overSSL_set_rfd/SSL_set_wfdon a datagram socketpair.make checkpasses with default,--enable-dtls,--enable-all, and--enable-all --enable-cryptocb --enable-asynccrypt-sw.