Skip to content

Fix resource leaks in TCP stream and system counters - #113

Merged
lubaihua33 merged 4 commits into
microsoft:masterfrom
lokeshmuthuraj:fix/tcp-sync-leaks
Aug 3, 2026
Merged

Fix resource leaks in TCP stream and system counters#113
lubaihua33 merged 4 commits into
microsoft:masterfrom
lokeshmuthuraj:fix/tcp-sync-leaks

Conversation

@lokeshmuthuraj

Copy link
Copy Markdown
Contributor

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(), if getaddrinfo() fails after ASPRINTF(&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.csockfd not closed in bind-retry loop in ntttcp_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 via ASPRINTF overwrite in ntttcp_server_listen()

The bind error logging used:

ASPRINTF(&log, "%s. errcode = %d", log, errno);  // BUG: log on both sides

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.cnewfd not closed in ntttcp_server_epoll() error paths

In ntttcp_server_epoll(), after accept() creates newfd, 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.cnewfd not closed in ntttcp_server_select() error paths

Same 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) when p == 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/interrupts file descriptor not closed on early return

In get_interrupts_from_proc_by_dev, if dev_name is 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.

@lokeshmuthuraj

Copy link
Copy Markdown
Contributor Author

lubaihua33 / Simon Xiao (@simonxiaoss) Please review the PR. Thanks.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.c across bind-retry, epoll, and select error paths.
  • Close /proc/interrupts on early return in oscounter.c.
  • Change default_ntttcp_test() to return an error code and allocate bind_address via strdup(), 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.

Comment thread src/ntttcp.c
Comment thread src/ntttcp.c
Comment thread src/tcpstream.c

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment thread src/tcpstream.c

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment thread src/tcpstream.c
Comment thread src/tcpstream.c

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, but PRINT_ERR_FREE(log) dereferences it (see logger.c:60-84 + logger.h:22-26). Add the semicolon and guard the print/free when log is NULL.
							int accept_errno = errno;
							ASPRINTF(&log, "error to accept new connections. errno = %d", accept_errno)
							PRINT_ERR_FREE(log);
							break;

@lokeshmuthuraj

Copy link
Copy Markdown
Contributor Author

lubaihua33 I have fixed the merge conflict and resolved Copilot comments. Please review PR. Thanks.

@lokeshmuthuraj

Copy link
Copy Markdown
Contributor Author

lubaihua33 Can you merge this PR? I don't have the permission to merge. Thanks.

@lubaihua33
lubaihua33 merged commit 771f40e into microsoft:master Aug 3, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants