Repository navigation
Fix test_tls13_ticket_set_alloc_fail with --enable-trackmemory - #12063
ohki-saito wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
4 open issues
ticket_alloc_prev_mallocbut free via… · Newticket_alloc_prev_mallocbut free via… · Newticket_alloc_prev_mallocbut free via… · NewExpectIntEQ(...)inside the block fails, execution typically continues, which can leave… · New
What changed in this PR
Adjusts the TLS 1.3 session ticket allocation-failure test to wrap the “real” allocators (the ones installed before the test) so allocations can be cleaned up correctly after allocator restoration.
Changes:
- Store the pre-test allocator callbacks in file-scope statics and route allocations/frees through them.
- Update the failure-injection malloc/free/realloc wrappers to call the previous callbacks when present.
- Replace per-test local “prev allocator” variables with the new statics.
| File | Description |
|---|---|
| tests/api/test_tls13.c | Routes test allocation wrappers through previously installed allocators and updates the test to restore allocator state using saved callbacks. |
Review effort: Lite
| return (ticket_alloc_prev_malloc != NULL) ? | ||
| ticket_alloc_prev_malloc(size) : malloc(size); |
| if (ticket_alloc_prev_free != NULL) | ||
| ticket_alloc_prev_free(ptr); | ||
| else | ||
| free(ptr); |
| return (ticket_alloc_prev_realloc != NULL) ? | ||
| ticket_alloc_prev_realloc(ptr, size) : realloc(ptr, size); |
| ExpectIntEQ(wolfSSL_send_SessionTicket(ssl_s), WOLFSSL_SUCCESS); | ||
| if (EXPECT_SUCCESS()) { | ||
| ticket_alloc_fail_sz = ssl_c->session->ticketLen; | ||
| ExpectIntEQ(wolfSSL_GetAllocators(&prevM, &prevF, &prevR), 0); | ||
| ExpectIntEQ(wolfSSL_GetAllocators(&ticket_alloc_prev_malloc, | ||
| &ticket_alloc_prev_free, &ticket_alloc_prev_realloc), 0); | ||
| ExpectIntEQ(wolfSSL_SetAllocators(ticket_alloc_fail_malloc, | ||
| ticket_alloc_fail_free, ticket_alloc_fail_realloc), 0); | ||
| ExpectIntEQ(wolfSSL_read(ssl_c, readBuf, sizeof(readBuf)), -1); | ||
| ticket_alloc_fail_sz = 0; | ||
| (void)wolfSSL_SetAllocators(prevM, prevF, prevR); | ||
| (void)wolfSSL_SetAllocators(ticket_alloc_prev_malloc, | ||
| ticket_alloc_prev_free, ticket_alloc_prev_realloc); | ||
| ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), | ||
| WC_NO_ERR_TRACE(MEMORY_E)); | ||
| } |
|
I don't think a change is needed, for the following reason wolfSSL_Malloc/Free/Realloc in memory.c also fall back to the standard functions per callback when one is NULL, and these callbacks do the same, so the pairing of allocations and frees doesn’t change. If the callbacks were only partly set, wolfSSL itself would already break: with SetAllocators(TrackMalloc, NULL, TrackRealloc), the test aborts in wolfSSL_send_SessionTicket before these callbacks are installed. Requiring all three to be non-NULL would also stop the test from running in the default build, where they are all NULL. ExpectIntEQ() doesn’t change control flow; after a failure it only skips evaluating later Expect checks. The reset of ticket_alloc_fail_sz and the allocator restore are plain statements, so they run on every path, and GetAllocators() always runs because the block is only entered on success. The structure is unchanged from master. |
|
retest this please |
|
Description
Non-Fenrir findings
test_tls13_ticket_set_alloc_fail aborts with --enable-trackmemory because its allocators call raw malloc/free, while the ticket buffer was allocated by TrackMalloc before the swap. Delegate to the saved allocators instead, as done for the PKCS12 and OCSP OOM tests.
Other tests also call raw malloc/free in their allocators, but they don’t fail with --enable-trackmemory today.
Testing
make check with --enable-all --enable-trackmemory.
Checklist