tls: fix regression and proper shutdown #10924
Merged
Conversation
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
WalkthroughAdds a TLS session invalidation hook and public API, implements invalidation for the OpenSSL backend, invokes session invalidation during upstream connection teardown when TLS is enabled, and adds a unit test exercising upstream TLS teardown. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Upstream
participant T as TLS core
participant B as TLS Backend (OpenSSL)
rect rgba(232,244,253,0.6)
Note over U: prepare_destroy_conn (TLS-enabled)
U->>T: flb_tls_session_invalidate(session)
alt backend supports invalidate
T->>B: session_invalidate(session)
B-->>T: session invalidated (fd = -1, mutex released)
else
T-->>U: no-op / error
end
end
U->>U: shutdown_connection()
U->>U: close(fd)
Note over U: connection moved to destroy queue / marked shutdown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
edsiper
force-pushed
the
tls-regression-reuse
branch
from
b44c142 to
cecb120
Compare
edsiper
force-pushed
the
tls-regression-reuse
branch
from
cecb120 to
7d873fb
Compare
edsiper
force-pushed
the
tls-regression-reuse
branch
from
7d873fb to
43ce960
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/internal/CMakeLists.txt (2)
58-64: Use list(APPEND) instead of set for appending to listsMore idiomatic and avoids re-specifying the existing variable.
if(FLB_TLS) - set(UNIT_TESTS_FILES - ${UNIT_TESTS_FILES} - upstream_tls.c - ) + list(APPEND UNIT_TESTS_FILES + upstream_tls.c + ) endif()
58-64: Windows build compatibility checkThis adds the TLS test on all platforms when FLB_TLS is ON. If upstream_tls.c uses POSIX-only APIs, it may fail on Windows. Confirm it builds on Windows CI; otherwise wrap it similarly to other POSIX-only tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/internal/CMakeLists.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: PR - fuzzing test
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
🔇 Additional comments (1)
tests/internal/CMakeLists.txt (1)
58-64: FLB_TLS guard is correct — keep if(FLB_TLS) in tests/internal/CMakeLists.txtTop-level CMakeLists.txt declares option(FLB_TLS) and FLB_HAVE_TLS is derived from it, so the if(FLB_TLS) guard is correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/internal/upstream_tls.c (1)
84-89: Strengthen post-conditions: also verify busy_queue is empty.After release, the connection should no longer be in busy_queue. This complements the destroy_queue assertion.
TEST_CHECK(mk_list_size(&queue->destroy_queue) == 1); TEST_CHECK(conn.shutdown_flag == FLB_TRUE); + TEST_CHECK(mk_list_size(&queue->busy_queue) == 0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/internal/CMakeLists.txt(1 hunks)tests/internal/upstream_tls.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/internal/CMakeLists.txt
🧰 Additional context used
🧬 Code graph analysis (1)
tests/internal/upstream_tls.c (1)
src/flb_upstream.c (2)
flb_upstream_queue_init(219-224)flb_upstream_conn_release(862-947)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: PR - fuzzing test
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-centos-7
🔇 Additional comments (2)
tests/internal/upstream_tls.c (2)
23-30: LGTM: backend session_invalidate shim is simple and correct.The callback increments exactly once; clear and side‑effect free.
51-53: Resource handling looks correct.socket_pair[0] is closed by the release path; socket_pair[1] is explicitly closed by the test. Windows init/cleanup is properly guarded.
If CI has a Windows job, please confirm this test passes there too (event loop closing semantics can differ). -->
Also applies to: 90-95
Comment on lines
+69
to
+76
| conn.fd = socket_pair[0]; | ||
| conn.event.fd = conn.fd; | ||
| conn.event.status = 0; | ||
| conn.stream = (struct flb_stream *) &upstream; | ||
| conn.net = &upstream.base.net; | ||
| conn.tls_session = &tls_session; | ||
| conn.net_error = 0; | ||
|
|
There was a problem hiding this comment.
Set conn.upstream and use upstream.base for conn.stream (fix crash risk).
flb_upstream_conn_release dereferences conn->upstream. Here it’s never set, and conn->stream is pointing to &upstream (not &upstream.base). This can lead to NULL deref/UB at runtime.
Apply this diff:
conn.fd = socket_pair[0];
conn.event.fd = conn.fd;
conn.event.status = 0;
- conn.stream = (struct flb_stream *) &upstream;
+ conn.upstream = &upstream;
+ conn.stream = &upstream.base;
conn.net = &upstream.base.net;
conn.tls_session = &tls_session;
- conn.net_error = 0;
+ /* -1 is the sentinel used in keepalive path checks for “no error” */
+ conn.net_error = -1;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| conn.fd = socket_pair[0]; | |
| conn.event.fd = conn.fd; | |
| conn.event.status = 0; | |
| conn.stream = (struct flb_stream *) &upstream; | |
| conn.net = &upstream.base.net; | |
| conn.tls_session = &tls_session; | |
| conn.net_error = 0; | |
| conn.fd = socket_pair[0]; | |
| conn.event.fd = conn.fd; | |
| conn.event.status = 0; | |
| conn.upstream = &upstream; | |
| conn.stream = &upstream.base; | |
| conn.net = &upstream.base.net; | |
| conn.tls_session = &tls_session; | |
| /* -1 is the sentinel used in keepalive path checks for "no error" */ | |
| conn.net_error = -1; |
🤖 Prompt for AI Agents
In tests/internal/upstream_tls.c around lines 69 to 76, conn->upstream is never
set and conn->stream is incorrectly set to &upstream (instead of the
upstream.base), which risks a NULL deref in flb_upstream_conn_release; set
conn.upstream = &upstream and set conn.stream = (struct flb_stream *)
&upstream.base to ensure the connection references the upstream object and its
base stream correctly.
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
edsiper
force-pushed
the
tls-regression-reuse
branch
from
43ce960 to
984a13d
Compare
Member
Author
|
rebased on top of master, #10930 should fix the processor issue |
This was referenced Sep 25, 2025
|
I am reviewing the fix for Issue #10923 (Azure Kusto output plugin connection failure caused by PR #10895). Could you please confirm the specific release that contains the fix? We checked the release notes and were unable to see PR #10924 (or any Azure Kusto plugin fixes) explicitly listed in any version’s changelog. Thank you for your help and for the quick turnaround on the original issue. |
Collaborator
|
Looks like you already raised an issue but the commit shows the tag: #11172 (comment) |
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.
Fixes #10923
Added a backend hook to invalidate TLS sessions, invoked before socket shutdown so SSL_shutdown never targets a recycled descriptor.
Also introduced a unit test that confirms prepare_destroy_conn invalidates the TLS session and wired it into the internal test suite when TLS is enabled
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Bug Fixes
Tests