out_azure_kusto: fix timer/flush race condition causing SIGSEGV #11303
Conversation
The ingest_all_chunks() function had nested mk_list_foreach_safe loops that both used the same 'tmp' variable as the iterator. The macro stores the 'next' pointer in its second argument for safe iteration during list modification. When the inner loop overwrote 'tmp', it corrupted the outer loop's iteration state, causing undefined behavior and a SIGSEGV crash when processing buffered backlog data on startup. Fix: Add a dedicated 'f_tmp' variable for the inner loop to prevent iterator corruption. Also adds a regression test (buffering_backlog) that exercises the buffering/backlog restart code path to guard against future regressions. Signed-off-by: Yash Ananth <[email protected]> Signed-off-by: Yashwanth Anantharaju <[email protected]>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR introduces thread synchronization and timer management to the Azure Kusto output plugin. Changes add a mutex ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/out_azure_kusto/azure_kusto.c (1)
668-683: Potential use-after-free in retry loop after unlocking file.After gzip compression fails:
- Line 674:
azure_kusto_store_file_unlock(file)setsfile->locked = FLB_FALSE- Line 675:
file->failures += 1(small window but OK)- Line 681:
sleep(backoff_time + jitter)- Line 683:
continueretries using the samefilepointerDuring the sleep, another thread (e.g.,
cb_azure_kusto_exitor another flush) could callazure_kusto_store_file_deletewhich now seeslocked=FALSEand proceeds to free the file. When the retry loop continues, it accesses freed memory.The same pattern exists at lines 710-721 and lines 739-750.
Consider keeping the file locked during retries and only unlocking on final failure or success:
🔎 Suggested approach
Keep the file locked during the entire retry loop. Only unlock when:
- Maximum retries reached (before delete/inactive), or
- Successful ingestion (before delete)
if (ret != 0) { flb_plg_error(ctx->ins, "scheduler_kusto_ingest :: cannot gzip payload"); flb_free(buffer); flb_sds_destroy(payload); flb_sds_destroy(tag_sds); retry_count++; - if (file){ - azure_kusto_store_file_unlock(file); + if (file) { + /* Keep file locked during retries to prevent deletion by other threads */ file->failures += 1; } /* Add jitter: random value between 0 and backoff_time */Apply similar changes to the other retry failure paths (lines 710-721, 739-750).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
plugins/out_azure_kusto/azure_kusto.c(18 hunks)plugins/out_azure_kusto/azure_kusto.h(2 hunks)plugins/out_azure_kusto/azure_kusto_ingest.c(1 hunks)plugins/out_azure_kusto/azure_kusto_store.c(11 hunks)tests/runtime/out_azure_kusto.c(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T18:45:02.283Z
Learnt from: edsiper
Repo: fluent/fluent-bit PR: 11286
File: src/flb_http_client.c:1657-1715
Timestamp: 2025-12-17T18:45:02.283Z
Learning: In the Fluent Bit OAuth2 implementation (src/flb_oauth2.c), the function flb_oauth2_get_access_token returns a pointer to the cached token stored in the OAuth2 context (ctx->access_token), not a new allocation. The OAuth2 context manages the token lifecycle, so callers must not free the returned token pointer.
Applied to files:
plugins/out_azure_kusto/azure_kusto.c
🧬 Code graph analysis (2)
plugins/out_azure_kusto/azure_kusto_store.c (1)
src/flb_fstore.c (3)
flb_fstore_file_delete(259-277)flb_fstore_file_create(153-190)flb_fstore_file_inactive(238-256)
plugins/out_azure_kusto/azure_kusto.c (1)
plugins/out_azure_kusto/azure_kusto_store.c (5)
azure_kusto_store_file_lock(808-811)azure_kusto_store_file_unlock(822-825)azure_kusto_store_file_cleanup(694-716)azure_kusto_store_file_delete(732-755)azure_kusto_store_file_inactive(658-680)
🔇 Additional comments (15)
plugins/out_azure_kusto/azure_kusto.h (1)
130-147: LGTM! Well-documented synchronization primitives.The addition of
upload_timerfor timer lifecycle management andfiles_mutexfor protecting concurrent access to the file list are appropriate for addressing the race condition. The comment clearly documents the mutex's purpose.plugins/out_azure_kusto/azure_kusto_ingest.c (1)
641-651: LGTM! Correct ordering prevents use-after-free on queue failure.Moving the file deletion to after successful queue enqueue is the right fix. The previous approach deleted the file after blob creation but before queue enqueue, which could lose data if queue enqueue failed. The unlock-before-delete sequence aligns with the new
azure_kusto_store_file_deletesemantics that skip locked files.plugins/out_azure_kusto/azure_kusto_store.c (4)
143-153: Verify safe list iteration after deletion inside the loop.When
fsf->data == NULL, you delete the file viaflb_fstore_file_deleteand thencontinue. Themk_list_foreach_safemacro should handle safe deletion during iteration, but verify that thetmppointer (frommk_list_foreach_safe) correctly allows continuing after deletion. Looking at the code, this function uses a regularmk_list_foreach_safewhich captures the next pointer before the iteration body, so this should be safe.
248-286: LGTM! Proper synchronization during file creation.The mutex correctly guards the critical section where the file is created and
fsf->datais assigned. All error paths properly release the lock before returning. This prevents race conditions where another thread could access a partially initialized file.
736-752: Verify all callers handle the -1 return when file is locked.The skip-if-locked guard returns -1 when the file cannot be deleted because it's locked. This is correct for preventing races, but callers must handle this return value appropriately. For example,
azure_kusto_queued_ingestionlogs an error if delete fails, which is acceptable. Ensure all other call sites either:
- Already unlocked the file before calling (as done in azure_kusto.c line 755)
- Handle the -1 return appropriately
528-548: LGTM! Proper cleanup with dangling pointer mitigation.Setting
fsf->data = NULLafter freeing theazure_kusto_fileis a good defensive measure to prevent use-after-free via dangling pointers. The mutex protection around the cleanup iteration is appropriate.tests/runtime/out_azure_kusto.c (2)
31-45: LGTM! Appropriate test helper for directory cleanup.The
nftw-based recursive removal is a clean approach for test cleanup. Thestatcheck beforenftwprevents errors on non-existent paths.
329-416: Good regression test for the race condition.The test uses aggressive timing parameters (
Flush=0.5,upload_timeout=1s) to maximize the chance of triggering the race between the timer callback and flush/exit paths. The test comments clearly document the expected behavior before and after the fix. The Windows guard is appropriate since the test relies on POSIX-specific APIs (nftw,ftw.h).Note: This test relies on ASan to detect the UAF. Consider adding a comment noting that this test should be run with ASan enabled for full verification.
plugins/out_azure_kusto/azure_kusto.c (7)
146-147: LGTM! Proper mutex release on error path.The mutex must be released before returning on error. This was a bug in the previous code where the token_mutex would remain locked on memory allocation failure.
1556-1560: LGTM! Correct timer cancellation ordering.Canceling the upload timer before cleanup prevents the timer callback from racing with the exit cleanup. Setting
upload_timer = NULLafter destruction is good defensive practice.
426-557: LGTM! Correct lock protocol in ingest_all_chunks.The synchronization pattern is correct:
- Lock mutex (line 427)
- Mark chunk as locked (line 459) before releasing mutex
- Release mutex for I/O (line 460)
- Re-acquire mutex on all error paths before returning
- Re-acquire mutex before next iteration (line 552)
This ensures the chunk remains protected (via its
lockedflag) while I/O is in progress, preventing other threads from deleting it.
615-640: LGTM! Proper lock acquisition and file marking before I/O.The pattern of locking the mutex, marking the file as locked, then releasing the mutex before I/O is correct. This prevents other threads from deleting the file while I/O is in progress.
1339-1346: LGTM! Correct unlock-before-delete pattern.Unlocking the file before calling delete/inactive is necessary since those functions now skip locked files. This ensures the file can be properly cleaned up.
973-991: LGTM! Proper initialization of new synchronization primitives.Both
upload_timer(NULL) andfiles_mutexare properly initialized. The mutex is initialized alongside the other mutexes, maintaining consistency.
1259-1260: LGTM! Timer handle capture for lifecycle management.Passing
&ctx->upload_timertoflb_sched_timer_cb_createallows the plugin to cancel the timer during shutdown, preventing the race condition where the timer callback could access freed resources.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
fluent-bit/plugins/out_azure_kusto/azure_kusto.c
Lines 449 to 453 in 97759fa
Avoid deadlocking when dropping failed chunks
Inside ingest_all_chunks the function still holds ctx->files_mutex when it routes exhausted retries to azure_kusto_store_file_delete / azure_kusto_store_file_inactive (mutex taken at line 426). Those helpers now take the same files_mutex internally (see azure_kusto_store.c:736-742 and 664-669), so the first chunk that exceeds scheduler_max_retries will block on a recursive lock and leave the mutex permanently held, freezing subsequent flush/timer processing.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /* Mark chunk as locked before releasing mutex for I/O */ | ||
| azure_kusto_store_file_lock(chunk); | ||
| pthread_mutex_unlock(&ctx->files_mutex); |
There was a problem hiding this comment.
mk_list_foreach_safe caches f_tmp before the body runs, but the code immediately releases files_mutex (lines 458-460) to perform I/O and only re-acquires it later. While the mutex is unlocked, other paths that also lock files_mutex (flush/exit) can delete the next list node (which isn’t locked), leaving the cached f_tmp dangling; when the loop advances it dereferences freed memory, reintroducing the list-iteration crash this change is trying to fix.
Useful? React with 👍 / 👎.
This patch fixes a race condition between the scheduler timer callback (cb_azure_kusto_ingest) and flush/delete paths that causes SIGSEGV crashes when the timer accesses freed file list memory. Problem: The scheduler timer iterates ctx->stream_active->files via mk_list_foreach_safe while concurrent flush/exit paths delete files from the same list without synchronization. This causes use-after-free when the timer callback accesses memory that has been freed by task destruction. Stack trace from production crash: #0 memcpy() at memmove-vec-unaligned-erms.S:222 fluent#1 cio_file_content_copy() at lib/chunkio/src/cio_file.c:547 fluent#2 flb_fstore_file_content_copy() at src/flb_fstore.c:289 fluent#3 construct_request_buffer() at plugins/out_azure_kusto/azure_kusto.c fluent#4 cb_azure_kusto_ingest() at plugins/out_azure_kusto/azure_kusto.c fluent#5 flb_sched_event_handler() at src/flb_scheduler.c:624 Solution: 1. Add pthread_mutex_t files_mutex to protect file list operations 2. Implement lock→mark file locked→unlock→I/O→re-lock pattern in timer callback to minimize lock hold time during network I/O 3. Add skip-if-locked guards in delete/inactive/cleanup functions to prevent double-free and deadlock 4. Cancel upload timer in cb_azure_kusto_exit before cleanup to prevent races during shutdown 5. Move early-delete to after successful queue enqueue to prevent UAF on queue failure 6. Clear fsf->data = NULL in store_exit to prevent dangling pointers The fix has been validated in production environments where the SIGSEGV crashes were occurring with multiple workers and buffering enabled. Signed-off-by: Yash Ananthakrishnan <[email protected]> Signed-off-by: Yashwanth Anantharaju <[email protected]>
yaananth
force-pushed
the
fix/azure-kusto-timer-flush-race
branch
from
97759fa to
361baa1
Compare
1 participant
Summary
This patch fixes a race condition between the scheduler timer callback (
cb_azure_kusto_ingest) and flush/delete paths that causes SIGSEGV crashes when the timer accesses freed file list memory.Problem
The scheduler timer iterates
ctx->stream_active->filesviamk_list_foreach_safewhile concurrent flush/exit paths delete files from the same list without synchronization. This causes use-after-free when the timer callback accesses memory that has been freed by task destruction.Stack trace from production crash:
Race condition timing evidence:
The crash occurs immediately after task destruction, confirming that
cb_azure_kusto_ingest()is accessing memory from a chunk/task that has already been destroyed.Solution
pthread_mutex_t files_mutexto protect file list operationscb_azure_kusto_exitbefore cleanup to prevent races during shutdownfsf->data = NULLin store_exit to prevent dangling pointersTesting
flb_test_azure_kusto_timer_flush_racetest to exercise the race condition scenarioworkers: 2andbuffering_enabled: onConfiguration that triggers the bug
Related
This is a follow-up to #11212 which fixed a separate nested loop iterator corruption issue. This PR addresses the timer/delete race condition that was a distinct root cause.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.