ssl_sess: sanity test session cache save and restore. - #11070
Open
philljj wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes persistent session-cache save/restore to serialize the full cache (not just a partial SessionRow), adds additional sanitization on restore, and adds API tests to validate both memory- and file-based cache persistence paths.
Changes:
- Introduces
wolfssl/ssl_sess.hto centralize session-cache layout constants/types and persistence sizing macros. - Updates
src/ssl_sess.cpersistence code to save fullSessionRowdata and restore without clobbering active row locks; expands restore-time sanitization. - Adds a new unit test
test_wolfSSL_session_cache_restorecoveringmemsave/memrestoreandsave/restoresession-cache APIs.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfssl/ssl_sess.h | New internal header consolidating session-cache sizing/layout and persistence helpers. |
| wolfssl/internal.h | Improves #endif comments around session/ticket-related conditionals. |
| wolfssl/include.am | Adds the new internal header to noinst_HEADERS. |
| tests/api/test_session.h | Registers the new session-cache restore unit test. |
| tests/api/test_session.c | Adds the new session-cache restore unit test implementation. |
| src/ssl_sess.c | Fixes persistence to copy full rows, avoid clobbering locks, and sanitize restored sessions. |
| CMakeLists.txt | Ensures ssl_sess.h remains excluded from installed public headers. |
Suppressed comments (3)
tests/api/test_session.c:1970
- test_read_file() logs the wrong variable on read failure (it prints ret instead of the read count) and uses "%d" with a size_t, which is undefined behavior on LP64 platforms.
read = XFREAD(cache_mem, sizeof(struct sess_cache_t), 1, file);
if (read != 1) {
WOLFSSL_MSG_EX("error: read %s: %d", fname, ret);
goto read_file_cleanup;
}
tests/api/test_session.c:2102
- In the remove() failure path, the fprintf prints ret instead of the remove() return code rc, which makes the diagnostic misleading.
int rc = remove(fname);
if (rc) {
fprintf(stderr, "remove(%s) failed: %d\n", fname, ret);
}
wolfssl/internal.h:4977
- This #endif closes a "#ifndef NO_SESSION_CACHE" block, but the comment currently says "NO_SESSION_CACHE" (missing the negation), which is confusing when scanning the struct layout.
#ifndef NO_SESSION_CACHE
int cacheRow; /* row in session cache */
#endif /* NO_SESSION_CACHE */
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1932
to
+1936
| n_write = (int)XFWRITE(cache_mem, sizeof(struct sess_cache_t), 1, file); | ||
| if (n_write != 1) { | ||
| WOLFSSL_MSG_EX("error: write %s: %d", fname, n_write); | ||
| goto write_file_cleanup; | ||
| } |
| #endif /* WOLFSSL_TICKET_NONCE_MALLOC && FIPS_VERSION_GE(5,3) */ | ||
| } TicketNonce; | ||
| #endif | ||
| #endif /* WOLFSSL_TLS13 && HAVE_SESSION_TICKET || !NO_PSK*/ |
Comment on lines
+385
to
+387
| #if defined(SESSION_CERTS) && defined(OPENSSL_EXTRA) | ||
| s->peer = NULL; | ||
| #endif | ||
| #endif /* SESSION_CERTS || OPENSSL_EXTRA */ |
Comment on lines
+48
to
+50
| SMALL_SESSION_CACHE only stores 6 sessions, good for embedded clients | ||
| or systems where the default of is too much RAM. | ||
| SessionCache takes about 2K, ClientCache takes about 3Kbytes |
Comment on lines
+1664
to
+1665
| /* set type, ticketLen, rem_ess_cb, heap, peer, etc to test | ||
| * canary values. These should be sanitized on session restore. */ |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
mem[save, restore]_session_cache(), and file[save, restore]_session_cache.#endifs better.Fixes zd#22196.
Fixes F-7286, F-7287, F-7288, F-7292.
Testing
test_wolfSSL_session_cache_restoreall-c89-clang-tidy