Fix UDP stream memory leaks#112
Open
lokeshmuthuraj wants to merge 1 commit into
Open
Conversation
Contributor
Author
|
@lubaihua33 / @simonxiaoss Please review the PR. Thanks. |
There was a problem hiding this comment.
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()freese->testbut does not freee->test->bind_address. With this PR,default_ntttcp_test()allocatesbind_addressviastrdup(), 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 pointbind_addressintoargv).
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 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 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 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 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 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 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 | ||
| ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.c—ASPRINTFlog leaked on early return inrun_ntttcp_sender_udp4_stream()udpstream.c—ASPRINTFlog leaked in socket error pathsIssue
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 viaASPRINTFoverwrite in receiver bind pathIn the receiver bind error path (lines 251-264), when appending the error code to an existing log message, the code pattern was:
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 exitrun_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
4. Wrong Socket Type Constant
Issue -
udpstream.c— wrongsocket()type parameter in UDP senderLine 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.c—sockfdnot closed in bind-retry loop in UDP receiverReceiver 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 exitEven 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.