ssl.c split: cleanup

[SparkiDev]

Description

Clean up includes single return point from every function, consistent formatting function block comment and tests added.

ssl_api_rw.c: Cleanup; 6 helpers extracted from write_dup/shutdown; fixed a SendBuffered corner case that returned 0 instead of an error.

ssl_api_ext.c: Cleanup; extracted wolfssl_ticket_key_cb_process, wolfssl_rehandshake_prepare.

ssl_api_hs.c: Cleanup + de-indent; 9 helpers extracted, incl. shared wolfssl_handshake_flush/_done between connect and accept (~100 duplicated lines removed). wolfSSL_connect/accept left multi-exit.

ssl_api_cert.c: Cleanup of newer functions at end of file; extracted PushPeerCertToChain; fixed a double free in CreatePeerCertChain; fixed 5 wrong @param names.

ssl_api_crl_ocsp.c: Full cleanup of all 49 functions; wolfSSL_OCSP_parse_url rewritten; fixed a URL with no host returning success with the rest of the URL as the host; added IPv6 literal support; fixed 4 wrong WOLFSSL_ENTER names.

ssl.c: Moved the x509GetIssuerFromCM forward declaration here for clarity.

Testing

Maximal config.
OpenSSL API regression testing.

Tests added.

[github-actions]

MemBrowse Memory Report

gcc-arm-cortex-m3

• FLASH: .text +44 B (+0.0%, 122,987 B / 262,144 B, total: 47% used)

gcc-arm-cortex-m4-dtls13

• FLASH: .text +64 B (+0.0%, 181,988 B / 1,048,576 B, total: 17% used)

gcc-arm-cortex-m4-openssl-compat

• FLASH: .rodata +160 B (+0.0%, 773,068 B / 1,048,576 B, total: 74% used)

gcc-arm-cortex-m4-tls13

• FLASH: .text +64 B (+0.0%, 237,319 B / 262,144 B, total: 91% used)

gcc-arm-cortex-m7-tls13

• FLASH: .text +64 B (+0.0%, 237,383 B / 262,144 B, total: 91% used)

linuxkm-standard

• Data: __patchable_function_entries +8 B (+0.0%, 49,200 B)
  No memory changes detected for:
• gcc-arm-cortex-m0plus
• gcc-arm-cortex-m4
• gcc-arm-cortex-m4-baremetal
• gcc-arm-cortex-m4-crypto-only
• gcc-arm-cortex-m4-min-ecc
• gcc-arm-cortex-m4-pkcs7
• gcc-arm-cortex-m4-pq
• gcc-arm-cortex-m4-rsa-only
• gcc-arm-cortex-m4-sp-math
• gcc-arm-cortex-m4-tls12
• gcc-arm-cortex-m7
• gcc-arm-cortex-m7-pq
• linuxkm-pie
• stm32-sim-stm32h753

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #11022

Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

[SparkiDev]

Jenkins: retest this please

[Frauschi]

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 21 total — 14 posted, 7 skipped

Posted findings

• [High] New rw and hs tests call wolfTLSv1_2_*_method() without a !WOLFSSL_NO_TLS12 guard - build break in TLS 1.3-only builds — tests/api/test_ssl_rw.c:82
• [High] wolfSSL_OCSP_parse_url now rejects URLs with an explicit port and a ':' in the path — src/ssl_api_crl_ocsp.c:1339-1349
• [High] New !sentNotify branch in wolfSSL_shutdown overwrites WOLFSSL_SUCCESS and changes ssl->error on a pre-existing path — src/ssl_api_rw.c:960-976
• [High] Guard mismatches after the split leave three static functions unused or undefined under -Werror — src/ssl_api_hs.c:89-92
• [Medium] wolfSSL_writev: small-stack allocation regression, ssl dereferenced before validation, and stale guard indentation — src/ssl_api_rw.c:1077-1114
• [Medium] None of the three fixes this PR claims is covered by a test that could fail without it — src/ssl_api_rw.c:965-976
• [Medium] set0_verify_cert_store still leaks the handed-over reference on the sibling early-return path — src/ssl_api_cert.c:1740-1762
• [Medium] Doxygen blocks in ssl_api_crl_ocsp.c copy-pasted from neighbouring functions describe the wrong parameter — src/ssl_api_crl_ocsp.c:1172-1177
• [Medium] wolfSSL_UseMaxFragment overloads ret with three incompatible conventions — src/ssl_api_ext.c:247-278
• [Low] wolfSSL_SendUserCanceled doc omits WOLFSSL_FATAL_ERROR — doc/dox_comments/header_files/ssl.h:2542-2575
• [Low] UnloadIntermediateCerts NULL check hidden behind an unrelated WOLFSSL_TRUST_PEER_CERT guard — tests/api/test_ssl_cert.c:1179-1182
• [Low] Leftovers from the ssl_api_hs.c de-indent: an indented preprocessor block and three dead (void) casts — src/ssl_api_hs.c:1459-1478
• [Low] Comment in _Rehandshake describes the branch that is not taken — src/ssl_api_ext.c:988-993
• [Info] wolfSSL_get_ocsp_response is now the only unguarded function in a hardened file — src/ssl_api_crl_ocsp.c:780-793

Skipped findings

• [Medium] ChangeLog.md not updated for the user-visible behaviour changes in this PR
• [Low] Five bare scope blocks introduced in test_ssl_hs.c
• [Low] Three new tests carry comments that claim more than the assertions can prove
• [Low] ALPN test allocates the peer buffer with the wrong dynamic type
• [Low] Handshake state enums moved to file scope pollute the ssl.c amalgamation namespace with very generic names
• [Low] Flush progress messages switch from WOLFSSL_MSG to WOLFSSL_MSG_EX, dropping them where XVSNPRINTF is unavailable
• [Info] Inconsistent backslash alignment in the extended DECLS macros

Review generated by Skoll via Claude/Codex

[SparkiDev]

Jenkins: retest this please

FIPS aborted

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #11022

Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src

No new issues found in the changed files. ✅

[Frauschi]

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 17 total — 10 posted, 7 skipped

Posted findings

• [High] New !sentNotify tail in wolfSSL_shutdown can overwrite a WOLFSSL_SUCCESS result — src/ssl_api_rw.c:993-1004
• [High] Static test helpers guarded looser than their only callers: -Werror unused-function in TLS-1.3-only builds — tests/api/test_ssl_rw.c:40-41,482-483; tests/api/test_ssl_hs.c:408-409; tests/api/test_ssl_ext.c:228-230
• [Medium] Over-restrictive !WOLFSSL_NO_TLS12 guards silently drop TLS 1.3 test coverage — tests/api/test_ssl_ext.c:297-318; tests/api/test_ssl_cert.c:966-1006
• [Medium] Moved x509GetIssuerFromCM forward declaration loses the !NO_CERTS guard — src/ssl.c:424-429
• [Medium] Blind-private-key behaviour change shipped undeclared, and conflates OOM with "not an EC key" — src/ssl_api_hs.c:1462-1490
• [Low] producedDate accessors skipped by the file-wide NULL-hardening pass — src/ssl_api_crl_ocsp.c:846-871
• [Low] OCSP response test leaks the allocated buffer on the failure path — tests/api/test_ssl_crl_ocsp.c:310-321
• [Info] inject test never re-tries the call its comment says is accepted — tests/api/test_ssl_rw.c:656-671
• [Info] test reads ctx->CBClientCert under a wider guard than the field has — tests/api/test_ssl_cert.c:441-469
• [Info] wolfSSL_CTX_set_cert_store doc note does not match the guard it describes — src/ssl_api_cert.c:1632-1645

Skipped findings

• [Info] @param [in] ssl on two functions that mutate the object
• [Info] connect and accept compute advanceState by different rules for the same helper
• [Info] Vestigial ret = 0 and stale WANT_WRITE comment after the write-dup split
• [Info] myBuffer[0] = 0 makes the -Wmaybe-uninitialized pragma and its comment redundant
• [Info] endTime passed to the reset-timer helper while potentially uninitialized
• [Info] set0_verify_cert_store with the CTX's own store is now destructive rather than a no-op
• [Info] Dead (void)ssl in wolfSSL_negotiate

Review generated by Skoll via Claude/Codex

[SparkiDev]

Jenkins: retest this please

[Frauschi]

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 16 total — 9 posted, 7 skipped

Posted findings

• [High] New set0 free in wolfssl_set_verify_cert_store turns a safe no-op into a dangling pointer, and the new test cannot fail without it — src/ssl_api_cert.c:1748-1756
• [Medium] Two static helpers are guarded wider than their only callers, giving -Wunused-function under -Werror — tests/api/test_ssl_cert.c:416-419
• [Medium] wolfSSL_shutdown return value changes from 0 to WOLFSSL_FATAL_ERROR with no ChangeLog entry — src/ssl_api_rw.c:797-834
• [Medium] New shutdown branch reports SOCKET_PEER_CLOSED_E for locally-initiated closures, masking the real reason — src/ssl_api_rw.c:988-1006
• [Medium] wolfSSL_want_read/wolfSSL_want_write gained a "Must not be NULL" comment but no NULL check, unlike every other API in the file — src/ssl_api_rw.c:1203-1241
• [Low] CRL bad-argument test is over-gated on NO_FILESYSTEM and skips most of the CRL API — tests/api/test_ssl_crl_ocsp.c:47-61
• [Low] New ticket-key-callback test: gated on a macro the code under test does not depend on, and its callback documents a check it never performs — tests/api/test_ssl_ext.c:1190-1195
• [Low] Rewritten OCSP URL parser: unvalidated port string and userinfo folded into the host or port — src/ssl_api_crl_ocsp.c:1419-1433
• [Low] test_wolfSSL_rw_bad_args calls wolfSSL_recv without the !WOLFSSL_LEANPSK guard used by test_wolfSSL_send — tests/api/test_ssl_rw.c:407-447

Skipped findings

• [Medium] Blind-private-key fix in wolfSSL_set_accept_state is still undeclared, and the OOM path is now fail-open
• [Medium] New handshake tests assert only that a value is non-NULL or one-of-two, so they cannot catch a wrong mapping
• [Low] New wolfSSL_writev argument validation is untested
• [Low] Three new doc comments in ssl_api_crl_ocsp.c describe the wrong thing
• [Low] New standalone scope blocks appear once the preprocessor removes the attaching statement
• [Info] Preprocessor continuation lines de-indented flush left, hurting readability
• [Info] Comment in the stapled-response test describes the wrong assertion

Review generated by Skoll via Claude/Codex

[Frauschi]

Some of these findings may come from missing CI coverage, see #11036

[Frauschi]

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 16 total — 10 posted, 6 skipped

Posted findings

• [High] Build break: test_wolfSSL_pk_callback_ctx calls VerifyMac accessors that do not exist in an AEAD-only TLS 1.2 build — tests/api/test_ssl_hs.c:487-489
• [High] Build break: the shared I/O helper block in test_ssl_hs.c is guarded neither as a superset nor a subset of its six callers — tests/api/test_ssl_hs.c:911-913,1291-1293
• [Medium] wolfSSL_read_ex() changes its NULL-object return value in non-OPENSSL_EXTRA builds, and the new Doxygen documents the old behaviour — src/ssl_api_rw.c:656-665
• [Medium] wolfSSL_ex_wrapper: timer-setup helper overwrites the WOLFSSL_FATAL_ERROR default in ret — src/ssl_api_hs.c:1316,1341-1345
• [Medium] Two new tests load PEM credential files without requiring WOLFSSL_PEM_TO_DER — tests/api/test_ssl_hs.c:1163-1165,tests/api/test_ssl_cert.c:912-914
• [Medium] The wolfSSL_shutdown change is neither covered by the new test nor described in the public Doxygen — tests/api/test_ssl_rw.c:824-826
• [Low] Rewritten Doxygen blocks still carry copy-pasted parameter text and wrong parameter directions — src/ssl_api_crl_ocsp.c:213-220,1044-1051,1099-1105
• [Low] myBuffer[0] = 0 is a dead store that the retained pragma already covers — src/ssl_api_rw.c:1152-1171
• [Low] New helper doc blocks omit return codes that reach the caller — src/ssl_api_rw.c:125-164,368-412
• [Low] *wolfSSL_OCSP_parse_url leaves ssl set on failure though the cleanup promises otherwise — src/ssl_api_crl_ocsp.c:1349-1353,1488-1498

Skipped findings

• [Medium] wolfSSL_set_accept_state gains a functional change for WOLFSSL_BLIND_PRIVATE_KEY builds, undocumented and untested
• [Medium] Step-failure tests accept both success and failure, so they cannot detect a regression
• [Low] wolfssl_handshake_flush parameter named 'connect' shadows the POSIX socket function
• [Low] Preprocessor directives left at column 0 inside the newly nested blocks
• [Low] WOLFSSL_ABI moved onto the return-type line, deviating from the tree-wide convention
• [Info] writeStates is a scalar loop bound carrying a plural array-style name

Review generated by Skoll via Claude/Codex

[Frauschi]

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 11 total — 4 posted, 7 skipped

Posted findings

• [Medium] wolfSSL_set_secret documents a MEMORY_E return it can never produce — src/ssl_api_dtls.c:755-765
• [Medium] set0 path of wolfssl_set_verify_cert_store leaks the caller's reference, and the comment's rationale is incorrect — src/ssl_api_cert.c:1761-1776
• [Medium] New test assertions that cannot fail: the state-string loops and the write_ex out-parameter check — tests/api/test_ssl_hs.c:153-208
• [Low] The doc/dox_comments sweep left six defects in the public manual — doc/dox_comments/header_files/ssl.h:2525-2536

Skipped findings

• [Medium] The new ChangeLog section: missing behavioural changes, an overstated claim, and wrong categorisation
• [Low] x509GetIssuerFromCM: the forward declaration carries !NO_CERTS but the use site does not
• [Low] Step-failure tests: two entries are not steps, and the expected result is selected by a positional magic index
• [Low] Two coverage claims that do not hold in the config or build they name
• [Low] No regression test for the CreatePeerCertChain double free the PR fixes
• [Low] Leftovers from the mechanical cleanup pass across the split files
• [Info] client_cert_cb guard narrowing is a build fix, not an API removal (verified, no action needed)

Review generated by Skoll via Claude/Codex

[SparkiDev]

Jenkins: retest this please

[Frauschi]

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 13 total — 11 posted, 2 skipped

Posted findings

• [Medium] New NULL-clear path in wolfSSL_CTX_set1_verify_cert_store can leave ctx->cm->x509_store_p dangling — src/ssl_api_cert.c:1703-1708
• [Medium] Shutdown's new "always leave a reason" contract does not cover the repeat-call case — src/ssl_api_rw.c:1017-1035
• [Medium] test_wolfSSL_ticket_key_cb_renew_ext cannot fail if the RENEW handling is reverted — tests/api/test_ssl_ext.c:1369-1372
• [Medium] State-string enum and OUTPUT_STR table split ~300 lines apart with no compile-time size check — src/ssl_api_hs.c:1684-1709
• [Medium] Path ':' rejected only when no explicit port is present, which also rejects unambiguous bracketed-IPv6 URLs — src/ssl_api_crl_ocsp.c:1424-1439
• [Low] The mechanical cleanup is not applied uniformly: an early return survives in wolfSSL_write_ex, and three converted returns lose the error trace master emitted — src/ssl_api_rw.c:453-458
• [Low] wolfssl_shutdown_flush_alert documents "0 = shutdown must continue" but returns 0 after completing the shutdown — src/ssl_api_rw.c:806-853
• [Low] Preprocessor hygiene in src/ssl_api_hs.c: four function-local macros still leak into the files included after it, and the 142-line helper block closes on a bare #endif — src/ssl_api_hs.c:1328
• [Low] Two test_ssl_rw.c assertions rely on things they should not: an unportable SIZE_MAX sentinel that truncates to int, and an over-long length that is safe only by implementation detail — tests/api/test_ssl_rw.c:481-491
• [Low] OCSP URL failure cases do not check or free the output pointers — tests/api/test_ssl_crl_ocsp.c:457-465
• [Info] QUIC helper block omits WOLFSSL_TLS13 while its caller uses a TLS 1.3 method — tests/api/test_ssl_hs.c:844-846

Skipped findings

• [Medium] The OCSP parse_url rewrite is documented as something it is not: the wolfIO_DecodeUrl parity claim does not hold, and the grammar changes are absent from the ChangeLog
• [Info] wolfssl_rehandshake_prepare dereferences ssl->secure_renegotiation with no documented precondition

Review generated by Skoll via Claude/Codex

[Frauschi]

🟡 [Medium] New NULL-clear path in wolfSSL_CTX_set1_verify_cert_store can leave ctx->cm->x509_store_p dangling
💡 SUGGEST bug

The diff adds a NULL-store branch that frees ctx->x509_store_pt and sets it to NULL. On master, str == NULL was rejected with ret = 0 and no side effect, so this path is new.

wolfSSL_CTX_set_cert_store() (same file, line 1641) publishes the store in two places: ctx->x509_store_pt = str; and ctx->cm->x509_store_p = ctx->x509_store_pt;. It up_refs str->cm but NOT the store itself, so the context owns exactly the caller's handed-over store reference. The new clear releases that reference — dropping it to zero and actually freeing the store — but leaves ctx->cm->x509_store_p pointing at the freed object. ctx->cm itself survives, because set_cert_store took its own cm reference.

cm->x509_store_p is dereferenced at src/crl.c:646-648: if (cm != NULL && cm->x509_store_p != NULL) { LoadCertByIssuer(cm->x509_store_p, ...) }, reachable under the --enable-all-style config (OPENSSL_ALL + WOLFSSL_CERT_GEN + CERT_REQ/CERT_EXT + filesystem + dir + HAVE_CRL). The dangling pointer is non-NULL, so the guard passes and the freed store is used.

The sibling non-NULL branch (ctx->x509_store_pt = str; at line 1722) has the identical aliasing flaw and is pre-existing — the class is not new, but this diff adds a second way to reach it. No test covers the combination: wolfSSL_CTX_set_cert_store does not appear anywhere in tests/api/test_ssl_cert.c, and the new test_wolfSSL_verify_cert_store only reaches the clear via set1_verify_cert_store, which never touches cm->x509_store_p.

Suggestion:

Suggested change

	/* Clear any store set on the context - revert to the one it owns. */
	else if (str == NULL) {
	wolfSSL_X509_STORE_free(ctx->x509_store_pt);
	ctx->x509_store_pt = NULL;
	/* The certificate manager aliases the store just released. Re-point
	* it at the store the context owns so nothing dereferences freed
	* memory (see the LoadCertByIssuer() use in src/crl.c). */
	if (ctx->cm != NULL) {
	ctx->cm->x509_store_p = &ctx->x509_store;
	}
	ret = 1;
	}

Recommendation: Re-point ctx->cm->x509_store_p when the store it aliases is released. Apply the same fix to the non-NULL branch at 1720-1722. Add a test calling wolfSSL_CTX_set_cert_store() then clearing, ideally with a CRL lookup afterwards under ASAN.

[Frauschi]

🟡 [Medium] Shutdown's new "always leave a reason" contract does not cover the repeat-call case
💡 SUGGEST question

The new else if records SOCKET_PEER_CLOSED_E so that, per its own comment, "the caller is not left with a failure and no error to query". Its guard includes (!ssl->options.sentNotify), so it never fires once a close_notify has been sent.

The combination (sentNotify == 1, closeNotify == 1) falls through both arms of the chain and returns the initializer WC_NO_ERR_TRACE(WOLFSSL_FATAL_ERROR) with ssl->error still WOLFSSL_ERROR_NONE — exactly the condition the new branch was added to eliminate. It is reachable in builds without OPENSSL_EXTRA / WOLFSSL_WPAS_SMALL, where the trailing wolfSSL_clear() that resets those flags is compiled out: after a completed bidirectional shutdown, a further wolfSSL_shutdown() skips the flush (error is NONE, not WANT_WRITE), skips wolfssl_shutdown_send_close_notify() because sentNotify is set, fails sentNotify && !closeNotify, and fails !sentNotify.

This hole pre-dates the PR and is not a regression — master's final if (sentNotify && !closeNotify) behaved identically. It is worth an explicit decision because this diff introduces the contract, and neither the ChangeLog entry nor the new block comment mentions the exception.

Suggestion:

Suggested change

	else if ((!done) && (!ssl->options.sentNotify) &&
	else if ((!done) && (ret != WOLFSSL_SUCCESS)) {

Recommendation: Either drop the !ssl->options.sentNotify term — the inner ssl->error == WOLFSSL_ERROR_NONE test already preserves any more specific error — or state in the block comment that a repeat call after a completed shutdown intentionally still returns WOLFSSL_FATAL_ERROR with no recorded error.

[Frauschi]

🟡 [Medium] test_wolfSSL_ticket_key_cb_renew_ext cannot fail if the RENEW handling is reverted
💡 SUGGEST test

The test installs a ticket key callback that returns TICKET_KEY_CB_RET_RENEW on decrypt, and its comment claims it proves that RENEW maps to "a new ticket is required rather than plain success" — the WOLFSSL_TICKET_RET_CREATE branch newly extracted into wolfssl_ticket_key_cb_process() (src/ssl_api_ext.c).

But the only assertions after the resumption handshake are test_memio_do_handshake(...) == 0 and wolfSSL_session_reused(ssl_c) == 1. Both hold identically when the callback returns TICKET_KEY_CB_RET_OK: the ticket decrypts and the session resumes either way. The difference between WOLFSSL_TICKET_RET_OK and WOLFSSL_TICKET_RET_CREATE is only whether a new ticket is issued to the client, which the test never observes.

Deleting the RENEW arm from wolfssl_ticket_key_cb_process() would leave this test green, so it does not cover the code path it documents.

Suggestion: Register wolfSSL_set_SessionTicket_cb() on the client for the resumption handshake and assert it fires, or capture the ticket bytes with wolfSSL_get_SessionTicket() before and after and assert they differ.

Recommendation: Add an assertion that observes the reissued ticket, so reverting the RENEW handling makes the test fail.

[Frauschi]

🟡 [Medium] State-string enum and OUTPUT_STR table split ~300 lines apart with no compile-time size check
💡 SUGGEST convention

The new file-scope enum WolfsslSsState has 24 entries (WOLFSSL_SS_NULL_STATE = 0 .. WOLFSSL_SS_HANDSHAKE_DONE = 23) and lines up row-for-row with the 24 rows of OUTPUT_STR[24][8][3]. I verified the alignment entry by entry — there is no off-by-one today, and WOLFSSL_SS_UNKNOWN = 100 is filtered out before the index is used.

What the refactor changes is the safety margin. Previously enum SslState was declared inside wolfSSL_state_string_long() immediately below the OUTPUT_STR initializer, so enum and table were adjacent and naturally edited together. Now the enum lives at file scope roughly 290 lines above the table, and the three switch statements that produce the index live in separate helper functions returning a plain int. Nothing ties the enum cardinality to the hard-coded 24, and the compiler can no longer see that the value reaching OUTPUT_STR[state][protocol][cbmode] is bounded. Adding one WOLFSSL_SS_* entry without the matching STATE_STRINGS_PROTO_RW row becomes a silent out-of-bounds read of a static table.

Suggestion:

Suggested change

	enum WolfsslSsState {
	enum WolfsslSsState {
	...
	WOLFSSL_SS_HANDSHAKE_DONE,
	WOLFSSL_SS_STATE_CNT
	};
	...
	wc_static_assert(sizeof(OUTPUT_STR) / sizeof(OUTPUT_STR[0]) ==
	WOLFSSL_SS_STATE_CNT);

Recommendation: Add a trailing count sentinel plus a wc_static_assert next to the table, and the equivalent for the protocol dimension. Alternatively add a defensive range check on state before indexing.

[Frauschi]

🟡 [Medium] Path ':' rejected only when no explicit port is present, which also rejects unambiguous bracketed-IPv6 URLs
💡 SUGGEST bug

When the authority scan finds no ':' (uport == NULL), the rewrite walks the entire path and fails on any ':' it finds. That rule is applied asymmetrically and is no longer needed by this parser.

After the rewrite the port is determined purely from the authority, which is bounded by authEnd (the first '/' after the scheme, line 1369). A ':' in the path can no longer be mistaken for a port by this code, so the rationale in the comment — "it would leave the port of the request open to argument between this parser and any other" — is about hypothetical third-party parsers, not this one.

Traced results:
http://host/a:b -> WOLFSSL_FAILURE
http://host:80/a:b -> WOLFSSL_SUCCESS, path "/a:b" (path scan skipped)
http://[::1]/a:b -> WOLFSSL_FAILURE
http://[::1]:80/a:b -> WOLFSSL_SUCCESS, path "/a:b"

The same path is accepted or rejected purely on whether an explicit port was written. For the plain-host form this matches master (the old if (upath != NULL && uport >= upath) goto err; rejected it too), so it is not a regression there. The bracketed-IPv6 case is newly reachable behaviour — master mis-parsed those URLs entirely (host came out as "[") — and brackets make the authority completely unambiguous. Both OpenSSL's OCSP_parse_url and wolfSSL's own wolfIO_DecodeUrl accept a ':' in the path.

Suggestion: Drop the path scan entirely — the authority-bounded scan above already removes the ambiguity the comment describes — or at minimum exempt the bracketed-IPv6 branch, where no ambiguity exists.

Recommendation: Make the treatment of a path ':' independent of whether an explicit port was written, and update the corresponding assertions in tests/api/test_ssl_crl_ocsp.c.

[Frauschi]

🔵 [Low] wolfssl_shutdown_flush_alert documents "0 = shutdown must continue" but returns 0 after completing the shutdown
🔧 NIT convention

The helper's documented contract is "@return 1 when the shutdown is finished and ret holds the result. @return 0 when the shutdown must continue." In the sentNotify && closeNotify path it sets ssl->options.shutdownDone = 1 and *ret = WOLFSSL_SUCCESS — the shutdown IS finished — yet returns done = 0.

It also leaves *ret untouched entirely on two paths (outer condition false, or SendBuffered succeeding with sentNotify clear), so correctness depends on the caller's int ret = WC_NO_ERR_TRACE(WOLFSSL_FATAL_ERROR); initializer, which the @param ret text does not mention.

Visible consequence in WOLFSSL_SHUTDOWNONCE builds: after this helper decides WOLFSSL_SUCCESS with done = 0, the SHUTDOWNONCE block at 1002-1010 runs and overwrites the success with SSL_SHUTDOWN_ALREADY_DONE_E when isClosed or connReset is set. That matches master's control flow exactly, so it is not a regression — but the newly extracted helper now carries a written contract its own code contradicts. wolfssl_shutdown_send_close_notify() has the same shape.

Suggestion:

Suggested change

	/* Flush an alert still sitting in the output buffer.
	* @param [in, out] ret Result for wolfSSL_shutdown() to return. Updated only
	* when this call reaches a decision; otherwise left as
	* the caller initialised it.
	* @return 1 when no further shutdown step may run.
	* @return 0 when the caller must continue.

Recommendation: Reword both helpers' contracts to match actual behaviour. No code change needed.

[Frauschi]

🔵 [Low] Preprocessor hygiene in src/ssl_api_hs.c: four function-local macros still leak into the files included after it, and the 142-line helper block closes on a bare #endif
🔧 NIT convention

Sites: src/ssl_api_hs.c:1328; src/ssl_api_hs.c:230
This commit adds #undef STATE_STRINGS_PROTO / #undef STATE_STRINGS_PROTO_RW at line 2039 with the rationale "Only used by the table above, and this file is compiled into ssl.c, so do not leave them defined for the files included after it."

That rule is not applied to the other four macros in the same file, all purely local helpers that leak into every src/ssl_*.c included into ssl.c after ssl_api_hs.c (included at ssl.c:3614, with ssl_api_ext.c, bio.c and x509.c following): ERR_OUT (1328), AddTimes (1147), SubtractTimes (1158), CmpTimes (1168).

Two related points. AddTimes is dead — verified by grep, it has exactly one occurrence in the whole tree, its own definition. And ERR_OUT expands to a bare brace block containing a return rather than do { } while (0), so if (x) ERR_OUT(e); else ... would not compile.

The leakage itself is pre-existing (these were already at preprocessor file scope despite being written indented inside the function body), and this diff only de-indented them. What is new is that the commit establishes and documents the opposite convention a few hundred lines away without applying it here.

Also, at src/ssl_api_hs.c:230 (Low, review scan) — Bare #endif closing the 142-line helper block, unlike every other long conditional in the cleaned-up file:

The new #if !defined(NO_TLS) && !(defined(WOLFSSL_NO_TLS12) && defined(NO_OLD_TLS) && defined(WOLFSSL_TLS13)) && (!defined(NO_WOLFSSL_CLIENT) || !defined(NO_WOLFSSL_SERVER)) at line 88 spans 142 lines and two function definitions, but its #endif at line 230 carries no comment. The nested #endif immediately above it (line 228, closing wolfssl_handshake_done) does carry the full condition, as does essentially every other multi-line conditional this commit reformats.

For the record, the guards themselves are correct this round, in both directions. wolfssl_handshake_flush, wolfssl_handshake_done, wolfssl_accept_check_creds, wolfssl_ex_wrapper_set_timer/reset_timer and the four wolfssl_state_string* helpers are each defined under exactly the set of configurations in which at least one caller compiles, and never when none does. The two hand-written justification comments at 169-171 and 661-662 are both accurate.

Suggestion:

Suggested change

	#define ERR_OUT(x) { ssl->hsInfoOn = 0; ssl->toInfoOn = 0; return x; }
	#undef ERR_OUT
	...
	#undef SubtractTimes
	#undef CmpTimes

Recommendation: Add #undef ERR_OUT after wolfSSL_ex_wrapper() and #undef SubtractTimes / #undef CmpTimes before #endif /* WOLFSSL_CALLBACKS */, and drop the dead AddTimes. If ERR_OUT is kept, wrap its body so it cannot break an if/else.

[src/ssl_api_hs.c:230] Label the #endif with its condition, matching the convention used throughout the rest of this diff.

[Frauschi]

🔵 [Low] Two test_ssl_rw.c assertions rely on things they should not: an unportable SIZE_MAX sentinel that truncates to int, and an over-long length that is safe only by implementation detail
🔧 NIT test

Sites: tests/api/test_ssl_rw.c:481-491; tests/api/test_ssl_rw.c:683-690
Two issues with the SIZE_MAX sentinel used to prove that wolfSSL_write_ex clears *wr while wolfSSL_read_ex leaves *rd untouched.

First, portability: this is the only use of SIZE_MAX in the tree outside wolfcrypt/src/port/mynewt. Nothing includes <stdint.h> unconditionally — wolfssl/wolfcrypt/types.h:429-432 only does so under HAVE_UINTPTR_T && !NO_STDINT_H. Autotools and CMake both probe for it, so CI is fine, but a user_settings.h build that compiles tests/api would fail to find the macro.

Second, the assertion is weaker than it looks: ExpectInt casts both operands to int (tests/unit.h:208-215), so ExpectIntEQ(rd, SIZE_MAX) compares (int)rd against -1. It still catches rd being zeroed, but it is not checking the full value.

Also, at tests/api/test_ssl_rw.c:683-690 (Low, review scan) — Over-long length passed to wolfSSL_inject with a short source buffer:

The test passes the 32-byte msg buffer with length maxLength + 1, which is several kilobytes. This is safe only because wolfSSL_inject() detects pending application data and returns APP_DATA_READY before reaching the XMEMCPY (src/ssl_api_rw.c:406-426) — the length is compared, never used to read. But the test is relying on an implementation detail to avoid a large out-of-bounds read, and the very same test later allocates a correctly-sized big buffer precisely to avoid this for the success case.

Suggestion:

Suggested change

	wr = SIZE_MAX;
	wr = 0x5a5a;
	rd = 0x5a5a;
	...
	ExpectIntEQ(rd, 0x5a5a);

Recommendation: Use a small plain sentinel that survives the int cast and needs no <stdint.h>.

[tests/api/test_ssl_rw.c:683-690] Pass a buffer that actually backs the length, so the test does not depend on the early return to stay in bounds.

[Frauschi]

🔵 [Low] OCSP URL failure cases do not check or free the output pointers
🔧 NIT test

Three consecutive failure assertions neither ExpectNull(host) nor free host/port/path, unlike the surrounding cases in the same test which consistently do one or the other. The err: cleanup label in wolfSSL_OCSP_parse_url is what guarantees those pointers come back NULL, so these cases silently skip verifying the very cleanup the rewrite added. A regression that leaked on those paths would be masked, since the next successful call overwrites the pointers. The same pattern recurs around lines 556-558.

Suggestion: Add ExpectNull(host); ExpectNull(port); ExpectNull(path); after each failure assertion, matching the pattern used elsewhere in this test.

Recommendation: Assert the out-params are NULL on every failure case so the err: cleanup path is actually covered.

[Frauschi]

⚪ [Info] QUIC helper block omits WOLFSSL_TLS13 while its caller uses a TLS 1.3 method
🔧 NIT style

The guard on the QUIC callback block is defined(WOLFSSL_QUIC) && (defined(OPENSSL_ALL) || defined(OPENSSL_EXTRA)) && !defined(NO_WOLFSSL_CLIENT) && !defined(NO_TLS) — it does not mention WOLFSSL_TLS13, yet the caller test_wolfSSL_SSL_do_handshake_quic() calls wolfTLSv1_3_client_method(). This compiles only because wolfssl/wolfcrypt/settings.h:5760-5763 raises an #error for QUIC without TLS 1.3, so the combination cannot exist. The equivalent block in tests/api/test_ssl_rw.c:718-719 does spell out defined(WOLFSSL_TLS13), so the two are inconsistent.

Suggestion:

Suggested change

	#if defined(WOLFSSL_QUIC) && \
	#if defined(WOLFSSL_QUIC) && defined(WOLFSSL_TLS13) && \
	(defined(OPENSSL_ALL) || defined(OPENSSL_EXTRA)) && \
	!defined(NO_WOLFSSL_CLIENT) && !defined(NO_TLS)

Recommendation: Add defined(WOLFSSL_TLS13) for consistency with the sibling block in test_ssl_rw.c, so the guard states its real requirement rather than relying on a settings.h #error.