Fix resource leaks in TCP stream and system counters - #113
Conversation
|
lubaihua33 / Simon Xiao (@simonxiaoss) Please review the PR. Thanks. |
There was a problem hiding this comment.
Pull request overview
This PR targets resource-exhaustion issues in long-running or high-connection tests by plugging several socket/memory/file-descriptor leaks in TCP stream handling and OS counter collection, and by improving default test initialization error handling (carried forward from PR #110).
Changes:
- Fix multiple TCP receiver-side FD leaks in
tcpstream.cacross bind-retry, epoll, and select error paths. - Close
/proc/interruptson early return inoscounter.c. - Change
default_ntttcp_test()to return an error code and allocatebind_addressviastrdup(), plus free the test results threads array (from PR #110).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tcpstream.c | Frees/close resources on error paths to avoid leaking sockets and heap allocations during bind/accept/epoll/select. |
| src/oscounter.c | Ensures /proc/interrupts is closed on early return when dev_name is empty. |
| src/ntttcp.h | Updates default_ntttcp_test() signature to return an int status code. |
| src/ntttcp.c | Implements default_ntttcp_test() error return + bind_address allocation; frees e->results->threads. |
| src/main.c | Handles default_ntttcp_test() return code; closes sync socket in several sender error paths. |
| src/const.h | Bumps tool version string. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/tcpstream.c:517
- Missing semicolon after ASPRINTF() causes a compile error here, and this error path can still crash on OOM because ASPRINTF only logs an error and may leave
log == NULL, butPRINT_ERR_FREE(log)dereferences it (see logger.c:60-84 + logger.h:22-26). Add the semicolon and guard the print/free whenlogis NULL.
int accept_errno = errno;
ASPRINTF(&log, "error to accept new connections. errno = %d", accept_errno)
PRINT_ERR_FREE(log);
break;
|
lubaihua33 I have fixed the merge conflict and resolved Copilot comments. Please review PR. Thanks. |
|
lubaihua33 Can you merge this PR? I don't have the permission to merge. Thanks. |
Overview
This PR fixes critical socket and memory leaks in TCP stream handling (both epoll and select modes) and adds a missing file descriptor close in system counter reading. These bugs caused resource exhaustion in long-running tests and multi-connection scenarios.
Depends on: PR #110
This PR contains changes from PR #110 and PR 110 will be merged before this PR.
Problems Solved - TCP Stream Issues (
src/tcpstream.c)1. Port String Memory Leak in Error Path
Issue
In
ntttcp_server_listen(), ifgetaddrinfo()fails afterASPRINTF(&port_str, ...), the early return leaked the allocated port string.Impact
5-10 bytes leaked per getaddrinfo failure.
2. Socket Leak in Bind Retry Loop
Issue -
tcpstream.c—sockfdnot closed in bind-retry loop inntttcp_server_listen()The bind retry loop created a socket, attempted bind, but on failure only logged an error and used continue without closing the socket. Each retry leaked a file descriptor.
Impact
Multiple bind attempts quickly exhausted fd limits (especially with multiple address candidates).
3. Log Reallocation Memory Leak
Issue -
tcpstream.c— log string leaked viaASPRINTFoverwrite inntttcp_server_listen()The bind error logging used:
This overwrites the log pointer before freeing the old string.
Impact
Every bind retry leaked the original error message.
4. Socket Leaks in Epoll Error Paths
Issue -
tcpstream.c—newfdnot closed inntttcp_server_epoll()error pathsIn
ntttcp_server_epoll(), afteraccept()createsnewfd, several error paths failed to close it before continuing:set_socket_non_blocking()failure (line 504)set_socket_tcp_nodelay()failure (line 509)epoll_ctl()failure (line 535)Impact
Each connection that hit these error paths, leaked a file descriptor. High-connection tests quickly exhausted fds.
5. Socket Leaks in Select Error Paths
Issue -
tcpstream.c—newfdnot closed inntttcp_server_select()error pathsSame as Issue 4 above but in ntttcp_server_select():
set_socket_non_blocking()failure (line 650)set_socket_tcp_nodelay()failure (line 657)Impact
Same as Issue 4 above for select-based receiver mode.
6. Redundant Socket Close After Loop
Issue
Line 395 attempted
close(sockfd)whenp == NULL(no address could bind). However, the socket was already closed in the loop's error path, making this a potential double-close or close of uninitialized fd.Impact
Undefined behavior, potential close of wrong fd.
7. File Descriptor Leak in Early Return (System Counter Issue (src/oscounter.c))
Issue -
oscounter.c—/proc/interruptsfile descriptor not closed on early returnIn
get_interrupts_from_proc_by_dev, ifdev_nameis empty string, the function returns early without closing the opened interrupts file.Impact
Leaked one file descriptor per call with empty
dev_name. Over time this exhausts fd limits.