Hardening: Fix memory leak in pcapng fail path and secure realloc logic#1665
Hardening: Fix memory leak in pcapng fail path and secure realloc logic#1665Noamismach wants to merge 1 commit intothe-tcpdump-group:masterfrom
Conversation
| free(p); | ||
| *err = 1; | ||
| return (NULL); | ||
| goto fail; |
There was a problem hiding this comment.
Not sure there's a leak there, as nothing other than the pcap_t-plus-private-data was allocated, and it's freed, but this is good practice anyway in case the code changes - the resource deallocation is, at least, centralized.
(This is not one of C's strengths.)
There was a problem hiding this comment.
Agreed. Even if the secondary allocations havent occurred yet at this specific point, funneling everything through a centralized goto fail block removes a major footgun for future modifications. And yes, definitely not one of C's strengths!
| free(p); | ||
| *err = 1; | ||
| return (NULL); | ||
| goto fail; |
|
Please make the commit message describe, using the conventional 72-column formatting, the problem(s) the commit fixes. |
Wireshark has a data structure called a I'll look at that after this is merged. We should probably backport this. |
Reroute early error returns in pcap_ng_check_header() (e.g., unknown timestamp precision or buffer allocation failures) through the centralized 'goto fail' path. This ensures that ps->ifaces and p->buffer are properly freed before returning NULL, preventing leaks. In read_block(), ensure that when p->buffer is successfully expanded via realloc() into a temporary pointer, the metadata (p->bufsize) is correctly updated to track the new capacity. This prevents stale size tracking and repeated, unnecessary reallocation attempts.
Noamismach
force-pushed
the
c99-pcapng-hardening
branch
from
70a6aa2 to
c6e5f63
Compare
|
@infrastation - Done. I've amended the commit message to include a detailed explanation of the fixes, wrapped at 72 columns, and force-pushed the update. @guyharris - That makes total sense. Having a unified struct for the buffer and its capacity would definitely eliminate this entire class of state-tracking bugs natively. Glad this patch helps secure things in the meantime, and honored to hear it might be backported! |
3 participants
This is a pure C99 follow-up to the discussions in PR #1664, isolating the parser hardening and memory leak fixes as recommended by @guyharris and @mcr.
What this PR changes:
pcap_ng_check_header: Reroutes early error returns (such as unknown timestamp precision orp->bufferallocation failures) through thegoto failpath. This ensures thatps->ifacesandp->bufferare properly freed before returningNULL.reallocstate inread_block: Ensures that whenp->bufferis successfully expanded viareallocinto a temporary pointer, the metadata (p->bufsize) is correctly updated to track the new capacity. This prevents stale size tracking and repeated, unnecessary reallocation attempts.This patch strictly adheres to the project's C99 requirements and introduces no changes to the build systems.