Close wakeup fds on thread failure - #5562
Open
BNCMK wants to merge 2 commits into
Open
Conversation
rd_kafka_broker_add() opens the broker's wake-up pipe before starting the broker thread. If thrd_create() fails, the function calls rd_free(rkb), which does not run rd_kafka_broker_destroy_final() and so never closes rkb_wakeup_fd[0] or [1]. The struct holding their numbers is released at the same moment, so nothing can close them afterwards either. Two descriptors leak per failed call, both pipes. Refs confluentinc#4456
# File descriptors leak when broker thread creation fails Confirms confluentinc#4456, identifies the mechanism, and includes a reproduction and a one-hunk fix. `src/rdkafka_broker.c`, verified against 2.15.0. ## What leaks `rd_kafka_broker_add()` opens the broker's wake-up pipe, then starts the broker thread: - the pipe is created by `rd_pipe_nonblocking(rkb->rkb_wakeup_fd)` - `rkb_wakeup_fd[0]` and `[1]` are closed in `rd_kafka_broker_destroy_final()` - on `thrd_create()` failure the function calls `rd_free(rkb)` and returns NULL `rd_free()` is a plain free. It does not run `rd_kafka_broker_destroy_final()`, so neither descriptor is closed, and the struct holding their numbers is released at the same moment. Nothing can close them afterwards either: they stay open for the life of the process with no reference to them anywhere. Every failed `rd_kafka_broker_add()` therefore leaks exactly two descriptors, and each is a pipe, which matches the "unreleased FIFO pipes" wording in the report. ## Reproduction `repro_4456.c` counts `/proc/self/fd` before and after and reports how many are pipes, so the figure comes from the kernel rather than from the library's own bookkeeping. Triggering the path needs `thrd_create()` to fail. Two approaches do not work and are worth recording so nobody repeats them: - `RLIMIT_NPROC` fails the client's own threads first, so the run never reaches `rd_kafka_broker_add()`. - Interposing `pthread_create` with `LD_PRELOAD` has no effect: `librdkafka.so` carries no dynamic reference to it, because glibc 2.34 and later resolve it internally. The failure was forced with a temporary condition on the `thrd_create` call, which is not part of the patch: ```c if (getenv("RDK_FORCE_THRD_FAIL") || thrd_create(&rkb->rkb_thread, rd_kafka_broker_thread_main, rkb) != thrd_success) { ``` Results, same binary, same reproduction, 5 clients: | build | descriptors before | after | leaked | |---|---|---|---| | unpatched | 3 | 23 | **20** | | patched | 3 | 3 | 0 | Two descriptors per broker, two brokers per client, five clients. With the patch the count is unchanged after 8 rounds, and a normal run with thread creation succeeding is also unchanged, so the working path is undisturbed. ## Confirmed independently at the syscall boundary Watching the reproduction from outside the process, with no knowledge of librdkafka's internals: ``` repro_4456: pipe2 10, close 9, openat 15 descriptors opened 35, closed 9, never closed 26 ``` Ten `pipe2` calls: five clients, two brokers each, one wake-up pipe per broker. Twenty of those descriptors are never closed, which is the same figure the `/proc/self/fd` count reports, reached from the other direction. ## The diff ```diff --- a/src/rdkafka_broker.c +++ b/src/rdkafka_broker.c @@ -5450,6 +5450,18 @@ rd_kafka_op_err(rk, RD_KAFKA_RESP_ERR__CRIT_SYS_RESOURCE, "Unable to create broker thread"); + /* The wake-up pipe was opened above and is closed by + * rd_kafka_broker_destroy_final(), which this path does not + * reach: rd_free() releases the struct holding the descriptor + * numbers without closing them, so nothing can close them + * afterwards. Close them here before the struct goes. */ +#ifndef _WIN32 + if (rkb->rkb_wakeup_fd[0] != -1) + rd_socket_close(rkb->rkb_wakeup_fd[0]); + if (rkb->rkb_wakeup_fd[1] != -1) + rd_socket_close(rkb->rkb_wakeup_fd[1]); +#endif + rd_free(rkb); #ifndef _WIN32 ``` ## Tooling Found by reading the failure path, then confirmed at the syscall boundary with [ExecVis](https://github.com/BNCMK/ExecVis), which records what every process on a machine did without the program being instrumented, recompiled or aware. This is the kind of defect it exists to make visible: the leak only runs when thread creation fails, so no test exercises it, no log records it, and once `rd_free` runs the descriptors have no owner left to report them. Counting opens against closes per process finds the imbalance regardless.
|
Please sign the Contributor License Agreement here before this PR can be approved. |
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.
File descriptors leak when broker thread creation fails
Confirms #4456, identifies the mechanism, and includes a reproduction and a
one-hunk fix.
src/rdkafka_broker.c, verified against 2.15.0.What leaks
rd_kafka_broker_add()opens the broker's wake-up pipe, then starts the brokerthread:
rd_pipe_nonblocking(rkb->rkb_wakeup_fd)rkb_wakeup_fd[0]and[1]are closed inrd_kafka_broker_destroy_final()thrd_create()failure the function callsrd_free(rkb)and returns NULLrd_free()is a plain free. It does not runrd_kafka_broker_destroy_final(),so neither descriptor is closed, and the struct holding their numbers is released
at the same moment. Nothing can close them afterwards either: they stay open for
the life of the process with no reference to them anywhere.
Every failed
rd_kafka_broker_add()therefore leaks exactly two descriptors, andeach is a pipe, which matches the "unreleased FIFO pipes" wording in the report.
Reproduction
repro_4456.ccounts/proc/self/fdbefore and after and reports how many arepipes, so the figure comes from the kernel rather than from the library's own
bookkeeping.
Triggering the path needs
thrd_create()to fail. Two approaches do not work andare worth recording so nobody repeats them:
RLIMIT_NPROCfails the client's own threads first, so the run never reachesrd_kafka_broker_add().pthread_createwithLD_PRELOADhas no effect:librdkafka.socarries no dynamic reference to it, because glibc 2.34 and later resolve it
internally.
The failure was forced with a temporary condition on the
thrd_createcall, whichis not part of the patch:
Results, same binary, same reproduction, 5 clients:
Two descriptors per broker, two brokers per client, five clients. With the patch
the count is unchanged after 8 rounds, and a normal run with thread creation
succeeding is also unchanged, so the working path is undisturbed.
Confirmed independently at the syscall boundary
Watching the reproduction from outside the process, with no knowledge of
librdkafka's internals:
Ten
pipe2calls: five clients, two brokers each, one wake-up pipe per broker.Twenty of those descriptors are never closed, which is the same figure the
/proc/self/fdcount reports, reached from the other direction.The diff
Tooling
Found by reading the failure path, then confirmed at the syscall boundary with
ExecVis, which records what every process on a
machine did without the program being instrumented, recompiled or aware.
This is the kind of defect it exists to make visible: the leak only runs when
thread creation fails, so no test exercises it, no log records it, and once
rd_freeruns the descriptors have no owner left to report them. Counting opensagainst closes per process finds the imbalance regardless.