Skip to content

Fix UDP stream memory leaks#112

Open
lokeshmuthuraj wants to merge 1 commit into
microsoft:masterfrom
lokeshmuthuraj:fix/udp-stream-leaks
Open

Fix UDP stream memory leaks#112
lokeshmuthuraj wants to merge 1 commit into
microsoft:masterfrom
lokeshmuthuraj:fix/udp-stream-leaks

Conversation

@lokeshmuthuraj
Copy link
Copy Markdown
Contributor

Overview

This PR fixes 8 critical bugs in the UDP stream implementation that caused memory leaks, socket leaks, and incorrect socket creation. These bugs prevented proper cleanup and caused resource exhaustion in long-running or high-connection-count UDP tests.

Depends on: PR #110
Contains changes from PR #110 and this PR will be merged after merging PR #110 .

Problems Solved

1. Log Memory Leaks in Error Paths

How found: ASAN, static analysis

  • udpstream.cASPRINTF log leaked on early return in run_ntttcp_sender_udp4_stream()
  • udpstream.cASPRINTF log leaked in socket error paths

Issue

Five error paths in UDP sender/receiver used PRINT_ERR(log) instead of PRINT_ERR_FREE(log), leaking the ASPRINTF-allocated error message string.

Impact

Every UDP connection failure leaked 40-200 bytes depending on message length. High-connection tests could leak kilobytes per run.

Locations

Line 58: Interface lookup failure (get_interface_name_by_ip)
Line 66: Client info update failure (ntttcp_update_client_info)
Line 91: Socket bind error with error code
Line 103: Socket bind to device error
Line 117: Socket connect failure

2. Log Reallocation Memory Leak

Issue - udpstream.c — log string leaked via ASPRINTF overwrite in receiver bind path

In the receiver bind error path (lines 251-264), when appending the error code to an existing log message, the code pattern was:

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

This creates a memory leak because the old log pointer is overwritten before being freed.

Impact

Every bind retry (common with multiple addresses) leaked the original error message.

3. Buffer and Socket Leaks on Receiver Error

Issue - udpstream.c — receive buffer and socket not freed on function exit

run_ntttcp_receiver_udp4_stream() allocated a large buffer and created sockfd, but the early return paths (lines 216-217, 277-279) didn't free/close them.

Impact

  • Buffer: Leaked 65,536 bytes per receiver error (RECV_BUFFER_SIZE)
  • Socket: File descriptor leak, exhausting system limit over time

4. Wrong Socket Type Constant

Issue - udpstream.c — wrong socket() type parameter in UDP sender

Line 69 created socket with socket(sc->domain, sc->domain, 0) — using domain (AF_INET/AF_INET6) as socket type instead of UDP (SOCK_DGRAM).

Impact

This would either fail or create the wrong socket type. Likely masked by later kernel auto-correction, but fundamentally wrong.

5. Incorrect sockfd Initialization

Issue - udpstream.csockfd not closed in bind-retry loop in UDP receiver

Receiver initialized int sockfd = 0 (line 198), but 0 is a valid file descriptor (stdin). Error checking used if (sockfd >= 0) which would incorrectly try to close stdin.

Impact

Potential incorrect close of file descriptor 0 on error paths.

6. Port String Memory Leak

Issue

Lines 214-217 called ASPRINTF(&port_str, "%d", ss->server_port) then getaddrinfo(), but if getaddrinfo() failed, the early return leaked port_str.

Impact

5-10 bytes leaked per getaddrinfo failure.

7. Socket Leak in Bind Loop

Issue

The bind retry loop (lines 241-267) created a socket, tried to bind, but on failure only logged an error and continue. The socket was never closed before retrying, leaking it.

Impact

Each bind attempt leaked a file descriptor. With multiple addresses, this quickly exhausted fd limits.

8. Missing Final Cleanup

Issue - udpstream.c — receive buffer and socket not freed on function exit

Even on successful receiver completion, the function returned without freeing buffer or closing sockfd (end of function at line 305).

Impact

Every successful UDP receiver run leaked 64KB buffer + 1 socket fd.

Functional test run

All 20 tests execute successfully.

image

@lokeshmuthuraj
Copy link
Copy Markdown
Contributor Author

@lubaihua33 / @simonxiaoss Please review the PR. Thanks.

Copy link
Copy Markdown

Copilot AI left a comment

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-safety regressions in the UDP stream implementation (memory/socket leaks and incorrect UDP socket creation), and adds functional coverage intended to prevent these issues from reappearing. It also carries forward API/initialization work from PR #110 (e.g., recoverable default initialization) and bumps the tool version.

Changes:

  • Fix UDP sender/receiver error paths to correctly free ASPRINTF-allocated logs and close sockets/buffers during bind/connect failures and on exit.
  • Correct UDP socket creation to use the UDP socket type (SOCK_DGRAM) rather than the address family.
  • Add functional tests covering multi-connection UDP sender scenarios and update defaults initialization to return an error code.

Reviewed changes

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

Show a summary per file
File Description
test/functional_test.py Adds several UDP sender regression/stress tests.
src/udpstream.c Fixes UDP sender socket creation and receiver bind/retry cleanup (logs, sockets, buffers).
src/ntttcp.h Updates default_ntttcp_test signature to return an error code.
src/ntttcp.c Makes default initialization return status; fixes results cleanup by freeing threads array.
src/main.c Handles default_ntttcp_test() failure and closes sync socket on additional sender error paths.
src/const.h Bumps tool version to 1.4.6.
Comments suppressed due to low confidence (1)

src/ntttcp.c:250

  • free_ntttcp_test_endpoint_and_test() frees e->test but does not free e->test->bind_address. With this PR, default_ntttcp_test() allocates bind_address via strdup(), so the current teardown leaks it (and if you later decide to free it here, you’ll need a consistent ownership policy because argument parsing can also point bind_address into argv).
	for (i = 0; i < total_threads; i++)
		free(e->results->threads[i]);

	free(e->results->threads);
	free(e->results->init_cpu_usage);
	free(e->results->init_cpu_ps);
	free(e->results->init_tcp_retrans);
	free(e->results->final_cpu_usage);
	free(e->results->final_cpu_ps);
	free(e->results->final_tcp_retrans);
	free(e->results);
	free(e->threads);
	free(e->test);
	free(e);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/udpstream.c
Comment on lines +54 to +58
if (sc->use_client_address) {
/* get interface name using the interface ip address */
if (get_interface_name_by_ip(sc->client_address, sc->domain, if_name, IFNAMSIZ) != 0) {
ASPRINTF(&log, "failed to get interface name by address [%s]", sc->client_address);
PRINT_ERR_FREE(log);
Comment thread src/ntttcp.c
Comment on lines +62 to +67
/* Allocate bind_address last to ensure all other fields are initialized if allocation fails */
test->bind_address = strdup("0.0.0.0");
if (!test->bind_address) {
PRINT_ERR("failed to allocate memory for bind_address in defaults");
return ERROR_MEMORY_ALLOC;
}
Comment thread src/main.c
Comment on lines +348 to +355
// Handle error return from default_ntttcp_test
err_code = default_ntttcp_test(test);
if (err_code != NO_ERROR) {
PRINT_ERR("main: error when initializing default test parameters");
free(test->bind_address);
free(test);
exit(err_code);
}
Comment thread test/functional_test.py
Comment on lines +346 to +349
"""
Test UDP sender with custom starting port to verify socket binding works correctly.
This exercises the client_port binding logic in run_ntttcp_sender_udp4_stream.
"""
Comment thread test/functional_test.py
Comment on lines +401 to +407
# Test with high connection count
common_option = f"-u -P {n_server_ports}"
sender_option = f"-n {n_threads} -l {n_connections} -V -t 3"
receiver_cmd, sender_cmd = self.combine_command(
common_option=common_option,
sender_option=sender_option
)
Comment thread test/functional_test.py
Comment on lines +473 to +479
for iteration in range(test_iterations):
common_option = f"-u -P {n_ports}"
sender_option = f"-n {n_threads} -l {n_connections} -V -t 1"
receiver_cmd, sender_cmd = self.combine_command(
common_option=common_option,
sender_option=sender_option
)
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.

2 participants