out_azure_kusto: Close OAuth handles in case of failure#11438
out_azure_kusto: Close OAuth handles in case of failure#11438ag-ramachandran wants to merge 2 commits intofluent:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds explicit resource cleanup paths during Azure Kusto plugin initialization failures, tightens MSI/OAuth2 error messages, enables OAuth2 for service-principal token construction, and removes an error log on token-mutex lock failure in the plugin's C implementation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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 |
![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
Here are some automated review suggestions for this pull request.
Reviewed commit: 8287a33d31
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
plugins/out_azure_kusto/azure_kusto.c (3)
946-962:⚠️ Potential issue | 🔴 CriticalNull-pointer dereference of
ctx->uand double-free ofctx->oauth_url.Two issues in this error-handling block:
NULL dereference (pre-existing, adjacent to your change):
ctx->uis used at lines 947–951 before the NULL check at line 952. Ifflb_upstream_create_urlreturnsNULL, the access toctx->u->basewill crash. The NULL check must come first.Double-free: Line 958 calls
flb_sds_destroy(ctx->oauth_url)without settingctx->oauth_url = NULL. Then line 960 callsflb_azure_kusto_conf_destroy(ctx), which [per the snippet atazure_kusto_conf.c:858-861] also destroysctx->oauth_urlif it's non-NULL—resulting in a double-free.Proposed fix
ctx->u = flb_upstream_create_url(config, ctx->ingestion_endpoint, io_flags, ins->tls); + if (!ctx->u) { + flb_plg_error(ctx->ins, "upstream creation failed"); + pthread_mutex_destroy(&ctx->resources_mutex); + pthread_mutex_destroy(&ctx->token_mutex); + pthread_mutex_destroy(&ctx->blob_mutex); + flb_azure_kusto_conf_destroy(ctx); + return -1; + } if (ctx->buffering_enabled == FLB_TRUE){ flb_stream_disable_flags(&ctx->u->base, FLB_IO_ASYNC); ctx->u->base.net.io_timeout = ctx->io_timeout; ctx->has_old_buffers = azure_kusto_store_has_data(ctx); } - if (!ctx->u) { - flb_plg_error(ctx->ins, "upstream creation failed"); - pthread_mutex_destroy(&ctx->resources_mutex); - pthread_mutex_destroy(&ctx->token_mutex); - pthread_mutex_destroy(&ctx->blob_mutex); - if (ctx->oauth_url) { - flb_sds_destroy(ctx->oauth_url); - } - flb_azure_kusto_conf_destroy(ctx); - return -1; - }Note:
flb_azure_kusto_conf_destroyalready handlesctx->oauth_url,ctx->o, andctx->resourcescleanup, so the separateflb_sds_destroy(ctx->oauth_url)is not only a double-free but also unnecessary.
967-980:⚠️ Potential issue | 🔴 CriticalSame double-free of
ctx->oauth_urlas in the upstream failure path.Lines 971–973 free
ctx->oauth_urlwithout nulling it, thenflb_azure_kusto_conf_destroyat line 978 frees it again. Either remove the manualflb_sds_destroy(sinceconf_destroyhandles it) or null out the pointer after freeing.Proposed fix (remove redundant free, let conf_destroy handle it)
if (!ctx->o) { flb_plg_error(ctx->ins, "cannot create oauth2 context"); - if (ctx->oauth_url) { - flb_sds_destroy(ctx->oauth_url); - } flb_upstream_destroy(ctx->u); pthread_mutex_destroy(&ctx->resources_mutex); pthread_mutex_destroy(&ctx->token_mutex); pthread_mutex_destroy(&ctx->blob_mutex); flb_azure_kusto_conf_destroy(ctx); return -1; }
152-158:⚠️ Potential issue | 🟠 MajorPre-existing: mutex leak when
outputallocation fails.Line 157 returns
NULLwithout reaching thepthread_mutex_unlockat line 163. Since this PR is specifically about closing handles and unlocking mutexes on error paths, this missed case is worth fixing too.Proposed fix
if (ret == 0) { output = flb_sds_create_size(flb_sds_len(ctx->o->token_type) + flb_sds_len(ctx->o->access_token) + 2); if (!output) { flb_plg_error(ctx->ins, "error creating token buffer"); - return NULL; + pthread_mutex_unlock(&ctx->token_mutex); + return NULL; }
🤖 Fix all issues with AI agents
In `@plugins/out_azure_kusto/azure_kusto.c`:
- Around line 116-123: token_cleanup currently unlocks ctx->token_mutex and then
returns success, which both hides errors and causes a double-unlock; fix by
removing the mutex unlock from the token_cleanup block and ensuring error paths
return -1 instead of jumping to token_cleanup. Concretely: in the token_cleanup
label (symbol: token_cleanup) delete the pthread_mutex_unlock(&ctx->token_mutex)
call and change token_cleanup to return -1 for error paths (or replace all
existing error goto token_cleanup sites in get_azure_kusto_token with direct
"return -1"); leave the mutex unlock to the caller get_azure_kusto_token
(symbol: get_azure_kusto_token) and ensure successful completion still returns 0
only when ctx->o->access_token is valid.
🧹 Nitpick comments (1)
plugins/out_azure_kusto/azure_kusto.c (1)
102-105: Redundant null check:ctx->ois already dereferenced earlier in this function.
ctx->ois used at line 77 (flb_oauth2_payload_clear(ctx->o)) and lines 79–97 (flb_oauth2_payload_append(ctx->o, ...)), so if it wereNULL, the function would have already crashed before reaching line 103. The guard is a no-op.Also, minor style nit: missing space after
if(if(ctx->o)→if (ctx->o)).Simplify
- /* Enable OAuth2 for token retrieval */ - if(ctx->o){ - ctx->o->cfg.enabled = FLB_TRUE; - } + /* Enable OAuth2 for token retrieval */ + ctx->o->cfg.enabled = FLB_TRUE;
There was a problem hiding this comment.
Actionable comments posted: 1
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)
941-957:⚠️ Potential issue | 🔴 CriticalCritical:
ctx->uis dereferenced before NULL check when buffering is enabled, causing crash and preventing cleanup code from executing.When
flb_upstream_create_urlreturns NULL andctx->buffering_enabled == FLB_TRUE, lines 943–944 dereferencectx->ubefore the null check at line 947. This causes a NULL-pointer dereference crash.Additionally, there is a double-free bug: line 953 explicitly destroys
ctx->oauth_urlbut does not NULL it. Then line 955 callsflb_azure_kusto_conf_destroy, which will attempt to destroy the same pointer again (lines 860–862 in azure_kusto_conf.c checkif (ctx->oauth_url), which is still non-NULL after the explicit destroy).The null check must move above the buffering block, and
ctx->oauth_urlmust be set to NULL after explicit destruction or the explicit destruction removed entirely (lettingconf_destroyhandle it). The same pattern appears at lines 966–973 (oauth2 creation failure).Proposed fix for NULL-deref and double-free
ctx->u = flb_upstream_create_url(config, ctx->ingestion_endpoint, io_flags, ins->tls); + if (!ctx->u) { + flb_plg_error(ctx->ins, "upstream creation failed"); + pthread_mutex_destroy(&ctx->resources_mutex); + pthread_mutex_destroy(&ctx->token_mutex); + pthread_mutex_destroy(&ctx->blob_mutex); + if (ctx->oauth_url) { + flb_sds_destroy(ctx->oauth_url); + ctx->oauth_url = NULL; + } + flb_azure_kusto_conf_destroy(ctx); + return -1; + } if (ctx->buffering_enabled == FLB_TRUE){ flb_stream_disable_flags(&ctx->u->base, FLB_IO_ASYNC); ctx->u->base.net.io_timeout = ctx->io_timeout; ctx->has_old_buffers = azure_kusto_store_has_data(ctx); } - if (!ctx->u) { - flb_plg_error(ctx->ins, "upstream creation failed"); - pthread_mutex_destroy(&ctx->resources_mutex); - pthread_mutex_destroy(&ctx->token_mutex); - pthread_mutex_destroy(&ctx->blob_mutex); - if (ctx->oauth_url) { - flb_sds_destroy(ctx->oauth_url); - } - flb_azure_kusto_conf_destroy(ctx); - return -1; - }Note: Add
ctx->oauth_url = NULL;after the explicit destroy to prevent double-free inconf_destroy.
🤖 Fix all issues with AI agents
In `@plugins/out_azure_kusto/azure_kusto.c`:
- Around line 102-105: Remove the redundant null-check around enabling OAuth2:
instead of wrapping the assignment in an if, directly set ctx->o->cfg.enabled =
FLB_TRUE; (delete the surrounding if(ctx->o) block entirely). Ensure spacing
follows project style if you edit surrounding lines (use "if (…)" only where a
check is actually required elsewhere) and leave the assignment to
ctx->o->cfg.enabled as-is so the OAuth2 token retrieval logic sees the flag.
🧹 Nitpick comments (2)
plugins/out_azure_kusto/azure_kusto.c (2)
962-975: Cleanup on OAuth2 creation failure looks correct in structure.The cleanup order (oauth_url → upstream → mutexes → conf_destroy) mirrors the exit path in
cb_azure_kusto_exit, which is good. The same double-free concern forctx->oauth_urlraised above applies here as well — ifflb_azure_kusto_conf_destroyfreesoauth_url, lines 966–968 would double-free it.A defensive fix (applicable to both error paths) is to NULL out the pointer after destroying:
if (ctx->oauth_url) { flb_sds_destroy(ctx->oauth_url); ctx->oauth_url = NULL; }
126-128: Silent failure on mutex lock.When
pthread_mutex_lockfails, the function returnsNULLwith no diagnostic. The caller will log a generic "cannot retrieve oauth2 token" error, which makes it harder to identify mutex-related failures. Consider keeping at least aflb_plg_warnhere.
Signed-off-by: ag-ramachandran <[email protected]>
ag-ramachandran
force-pushed
the
bugfix/OAuthErrorFixes
branch
from
756236b to
aa033e7
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/out_azure_kusto/azure_kusto.c (2)
146-154:⚠️ Potential issue | 🔴 CriticalMutex leak on allocation failure —
token_mutexis never unlocked.When
flb_sds_create_sizefails at line 148, the function returnsNULLat line 150 while still holdingctx->token_mutex(locked at line 124). This permanently deadlocks all subsequent token operations. Given this PR's focus on closing handles on failure paths, this appears to be a missed case.Proposed fix
if (ret == 0) { output = flb_sds_create_size(flb_sds_len(ctx->o->token_type) + flb_sds_len(ctx->o->access_token) + 2); if (!output) { flb_plg_error(ctx->ins, "error creating token buffer"); + pthread_mutex_unlock(&ctx->token_mutex); return NULL; } flb_sds_snprintf(&output, flb_sds_alloc(output), "%s %s", ctx->o->token_type, ctx->o->access_token); }
894-915:⚠️ Potential issue | 🟠 MajorLeaking
ctxon buffering validation failuresThe early
return -1statements at lines 899, 906, 910, and 914 don't callflb_azure_kusto_conf_destroy(ctx). Sincectxis allocated at line 884, these returns leak the configuration context. Later error paths in the same function (e.g., line 925) correctly include cleanup—these validation checks should too.Fix for all four paths
int ret = azure_kusto_store_init(ctx); if (ret == -1) { flb_plg_error(ctx->ins, "Failed to initialize kusto storage: %s", ctx->store_dir); + flb_azure_kusto_conf_destroy(ctx); return -1; } ctx->has_old_buffers = azure_kusto_store_has_data(ctx); /* validate 'total_file_size' */ if (ctx->file_size <= 0) { flb_plg_error(ctx->ins, "Failed to parse upload_file_size"); + flb_azure_kusto_conf_destroy(ctx); return -1; } if (ctx->file_size < 1000000) { flb_plg_error(ctx->ins, "upload_file_size must be at least 1MB"); + flb_azure_kusto_conf_destroy(ctx); return -1; } if (ctx->file_size > MAX_FILE_SIZE) { flb_plg_error(ctx->ins, "Max total_file_size must be lower than %ld bytes", MAX_FILE_SIZE); + flb_azure_kusto_conf_destroy(ctx); return -1; }
Signed-off-by: ag-ramachandran <[email protected]>
1 participant
Signed-off-by: ag-ramachandran [email protected]
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
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