aws_credentials: fix use-after-free when a refresh races get_credentials() - #12207
aws_credentials: fix use-after-free when a refresh races get_credentials()#12207Nahid-NHB wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughChangesAWS credential providers now use a dedicated cache mutex. New helpers synchronize credential copies, refresh timestamps, and cache replacement. EC2, HTTP, profile, STS, and EKS providers use these helpers. A multithreaded EKS regression test covers concurrent refreshes. AWS credential cache synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Worker
participant AWSProvider
participant CredentialCache
participant STS
Worker->>AWSProvider: request credentials
AWSProvider->>CredentialCache: read refresh time and copy credentials
alt credentials require refresh
AWSProvider->>STS: request refreshed credentials
STS-->>AWSProvider: return credentials and expiration
AWSProvider->>CredentialCache: atomically publish credentials and deadline
end
CredentialCache-->>AWSProvider: return consistent credential copy
AWSProvider-->>Worker: return credentials
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 0a9fdeb45e
ℹ️ 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: 4
🧹 Nitpick comments (1)
src/aws/flb_aws_credentials_profile.c (1)
127-131: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider returning the still-valid copy when the trylock fails.
If another thread holds the provider lock, this path discards
credsand returns NULL, even when the copy is still usable. The EC2, HTTP, and STS providers return the stale copy in the same situation. Returning the copy here avoids an unnecessary signing failure while a refresh is in progress. The current code is not a regression, so treat this as optional.♻️ Proposed change
} else { AWS_CREDS_WARN("Another thread is refreshing credentials, will retry"); - flb_aws_credentials_destroy(creds); - return NULL; + if (!creds) { + return NULL; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aws/flb_aws_credentials_profile.c` around lines 127 - 131, Optionally update the trylock-failure branch in flb_aws_credentials_refresh to return the still-valid creds copy instead of destroying it and returning NULL, matching the EC2, HTTP, and STS provider behavior while another thread refreshes credentials.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aws/flb_aws_credentials_profile.c`:
- Around line 719-723: Update the expiration adjustment in
get_credentials_fn_profile so subtracting FLB_AWS_REFRESH_WINDOW never produces
zero or a negative deadline; clamp positive expiration values at or below the
refresh window to a positive value that causes the next call to refresh, while
preserving the existing zero-expiration handling.
In `@src/aws/flb_aws_credentials.c`:
- Around line 543-544: Update flb_aws_env_provider_create() to initialize
provider->cache_lock with pthread_mutex_init alongside provider->lock, ensuring
flb_aws_provider_destroy() always destroys an initialized mutex; alternatively,
track creation state and guard cache_lock destruction in the destroy path.
In `@tests/internal/aws_credentials_sts.c`:
- Around line 675-745: Make concurrency_ctx.stop an atomic flag and use atomic
loads in concurrency_reader and an atomic store in concurrency_writer,
preserving the existing loop termination behavior. Update the test setup
initialization alongside the concurrency_ctx initialization to initialize the
atomic value correctly, eliminating the writer/readers data race.
- Around line 814-826: Track successful thread creation separately in the
concurrency test around pthread_create calls for concurrency_reader and
concurrency_writer, and only call pthread_join for threads whose creation
returned success. Ensure failed creations do not leave an uninitialized
pthread_t passed to pthread_join while preserving the existing continuation
behavior after TEST_CHECK.
---
Nitpick comments:
In `@src/aws/flb_aws_credentials_profile.c`:
- Around line 127-131: Optionally update the trylock-failure branch in
flb_aws_credentials_refresh to return the still-valid creds copy instead of
destroying it and returning NULL, matching the EC2, HTTP, and STS provider
behavior while another thread refreshes credentials.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e49ee57-906e-4a1f-914b-6333b6d643f1
📒 Files selected for processing (7)
include/fluent-bit/flb_aws_credentials.hsrc/aws/flb_aws_credentials.csrc/aws/flb_aws_credentials_ec2.csrc/aws/flb_aws_credentials_http.csrc/aws/flb_aws_credentials_profile.csrc/aws/flb_aws_credentials_sts.ctests/internal/aws_credentials_sts.c
| struct concurrency_ctx { | ||
| struct flb_aws_provider *provider; | ||
| int stop; | ||
|
|
||
| /* results, one slot per reader so they need no synchronization */ | ||
| int null_creds[CONCURRENCY_READERS]; | ||
| int torn_creds[CONCURRENCY_READERS]; | ||
| int reads[CONCURRENCY_READERS]; | ||
| }; | ||
|
|
||
| struct concurrency_reader_arg { | ||
| struct concurrency_ctx *ctx; | ||
| int id; | ||
| }; | ||
|
|
||
| /* Returns the generation number in a "<prefix>_<n>" credential field */ | ||
| static int credential_generation(const char *value, const char *prefix) | ||
| { | ||
| size_t prefix_len = strlen(prefix); | ||
|
|
||
| if (!value || strncmp(value, prefix, prefix_len) != 0) { | ||
| return -1; | ||
| } | ||
|
|
||
| return atoi(value + prefix_len); | ||
| } | ||
|
|
||
| static void *concurrency_reader(void *arg) | ||
| { | ||
| struct concurrency_reader_arg *reader = arg; | ||
| struct concurrency_ctx *ctx = reader->ctx; | ||
| struct flb_aws_provider *provider = ctx->provider; | ||
| struct flb_aws_credentials *creds; | ||
| int akid; | ||
| int skid; | ||
| int token; | ||
|
|
||
| while (ctx->stop == FLB_FALSE) { | ||
| creds = provider->provider_vtable->get_credentials(provider); | ||
| if (!creds) { | ||
| ctx->null_creds[reader->id]++; | ||
| continue; | ||
| } | ||
|
|
||
| akid = credential_generation(creds->access_key_id, "akid_"); | ||
| skid = credential_generation(creds->secret_access_key, "skid_"); | ||
| token = credential_generation(creds->session_token, "token_"); | ||
|
|
||
| if (akid < 0 || akid != skid || akid != token) { | ||
| ctx->torn_creds[reader->id]++; | ||
| } | ||
|
|
||
| ctx->reads[reader->id]++; | ||
| flb_aws_credentials_destroy(creds); | ||
| } | ||
|
|
||
| return NULL; | ||
| } | ||
|
|
||
| static void *concurrency_writer(void *arg) | ||
| { | ||
| struct concurrency_ctx *ctx = arg; | ||
| struct flb_aws_provider *provider = ctx->provider; | ||
| int i; | ||
|
|
||
| for (i = 0; i < CONCURRENCY_REFRESHES; i++) { | ||
| provider->provider_vtable->refresh(provider); | ||
| } | ||
|
|
||
| ctx->stop = FLB_TRUE; | ||
| return NULL; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Synchronize the stop flag between the writer and the readers.
ctx.stop is a plain int. The writer stores it at Line 744. The readers load it at Line 712 with no synchronization. Two problems follow:
- The compiler can hoist the load out of the reader loop, because nothing in the loop body is visibly related to
ctx.stop. The readers then spin forever andpthread_joinat Line 826 hangs the test suite. - The comment at Lines 668-669 asks the maintainer to run this test under TSan. TSan reports this store/load pair as a data race, which adds noise to the exact signal the test is meant to produce.
Use a C11 atomic for the flag, or guard it with a mutex.
🔒 Proposed fix using C11 atomics
+#include <stdatomic.h>
+
struct concurrency_ctx {
struct flb_aws_provider *provider;
- int stop;
+ atomic_int stop;
/* results, one slot per reader so they need no synchronization */
int null_creds[CONCURRENCY_READERS];
int torn_creds[CONCURRENCY_READERS];
int reads[CONCURRENCY_READERS];
};- while (ctx->stop == FLB_FALSE) {
+ while (atomic_load(&ctx->stop) == FLB_FALSE) {- ctx->stop = FLB_TRUE;
+ atomic_store(&ctx->stop, FLB_TRUE);Also update the initialization at Line 812:
- ctx.stop = FLB_FALSE;
+ atomic_store(&ctx.stop, FLB_FALSE);📝 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.
| struct concurrency_ctx { | |
| struct flb_aws_provider *provider; | |
| int stop; | |
| /* results, one slot per reader so they need no synchronization */ | |
| int null_creds[CONCURRENCY_READERS]; | |
| int torn_creds[CONCURRENCY_READERS]; | |
| int reads[CONCURRENCY_READERS]; | |
| }; | |
| struct concurrency_reader_arg { | |
| struct concurrency_ctx *ctx; | |
| int id; | |
| }; | |
| /* Returns the generation number in a "<prefix>_<n>" credential field */ | |
| static int credential_generation(const char *value, const char *prefix) | |
| { | |
| size_t prefix_len = strlen(prefix); | |
| if (!value || strncmp(value, prefix, prefix_len) != 0) { | |
| return -1; | |
| } | |
| return atoi(value + prefix_len); | |
| } | |
| static void *concurrency_reader(void *arg) | |
| { | |
| struct concurrency_reader_arg *reader = arg; | |
| struct concurrency_ctx *ctx = reader->ctx; | |
| struct flb_aws_provider *provider = ctx->provider; | |
| struct flb_aws_credentials *creds; | |
| int akid; | |
| int skid; | |
| int token; | |
| while (ctx->stop == FLB_FALSE) { | |
| creds = provider->provider_vtable->get_credentials(provider); | |
| if (!creds) { | |
| ctx->null_creds[reader->id]++; | |
| continue; | |
| } | |
| akid = credential_generation(creds->access_key_id, "akid_"); | |
| skid = credential_generation(creds->secret_access_key, "skid_"); | |
| token = credential_generation(creds->session_token, "token_"); | |
| if (akid < 0 || akid != skid || akid != token) { | |
| ctx->torn_creds[reader->id]++; | |
| } | |
| ctx->reads[reader->id]++; | |
| flb_aws_credentials_destroy(creds); | |
| } | |
| return NULL; | |
| } | |
| static void *concurrency_writer(void *arg) | |
| { | |
| struct concurrency_ctx *ctx = arg; | |
| struct flb_aws_provider *provider = ctx->provider; | |
| int i; | |
| for (i = 0; i < CONCURRENCY_REFRESHES; i++) { | |
| provider->provider_vtable->refresh(provider); | |
| } | |
| ctx->stop = FLB_TRUE; | |
| return NULL; | |
| struct concurrency_ctx { | |
| struct flb_aws_provider *provider; | |
| atomic_int stop; | |
| /* results, one slot per reader so they need no synchronization */ | |
| int null_creds[CONCURRENCY_READERS]; | |
| int torn_creds[CONCURRENCY_READERS]; | |
| int reads[CONCURRENCY_READERS]; | |
| }; | |
| struct concurrency_reader_arg { | |
| struct concurrency_ctx *ctx; | |
| int id; | |
| }; | |
| /* Returns the generation number in a "<prefix>_<n>" credential field */ | |
| static int credential_generation(const char *value, const char *prefix) | |
| { | |
| size_t prefix_len = strlen(prefix); | |
| if (!value || strncmp(value, prefix, prefix_len) != 0) { | |
| return -1; | |
| } | |
| return atoi(value + prefix_len); | |
| } | |
| static void *concurrency_reader(void *arg) | |
| { | |
| struct concurrency_reader_arg *reader = arg; | |
| struct concurrency_ctx *ctx = reader->ctx; | |
| struct flb_aws_provider *provider = ctx->provider; | |
| struct flb_aws_credentials *creds; | |
| int akid; | |
| int skid; | |
| int token; | |
| while (atomic_load(&ctx->stop) == FLB_FALSE) { | |
| creds = provider->provider_vtable->get_credentials(provider); | |
| if (!creds) { | |
| ctx->null_creds[reader->id]++; | |
| continue; | |
| } | |
| akid = credential_generation(creds->access_key_id, "akid_"); | |
| skid = credential_generation(creds->secret_access_key, "skid_"); | |
| token = credential_generation(creds->session_token, "token_"); | |
| if (akid < 0 || akid != skid || akid != token) { | |
| ctx->torn_creds[reader->id]++; | |
| } | |
| ctx->reads[reader->id]++; | |
| flb_aws_credentials_destroy(creds); | |
| } | |
| return NULL; | |
| } | |
| static void *concurrency_writer(void *arg) | |
| { | |
| struct concurrency_ctx *ctx = arg; | |
| struct flb_aws_provider *provider = ctx->provider; | |
| int i; | |
| for (i = 0; i < CONCURRENCY_REFRESHES; i++) { | |
| provider->provider_vtable->refresh(provider); | |
| } | |
| atomic_store(&ctx->stop, FLB_TRUE); | |
| return NULL; |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 698-698: Avoid atoi/atol/atoll/atof: they perform no error detection, returning 0 on non-numeric input and invoking undefined behavior on out-of-range values. This rule flags every use of these functions regardless of data provenance. Convert strings with strtol/strtoul/strtod and check errno (and the endptr) so malformed or overflowing input is rejected.
Context: atoi(value + prefix_len)
Note: [CWE-20] Improper Input Validation.
(atoi-no-error-detection-c)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/internal/aws_credentials_sts.c` around lines 675 - 745, Make
concurrency_ctx.stop an atomic flag and use atomic loads in concurrency_reader
and an atomic store in concurrency_writer, preserving the existing loop
termination behavior. Update the test setup initialization alongside the
concurrency_ctx initialization to initialize the atomic value correctly,
eliminating the writer/readers data race.
0a9fdeb to
1e86b00
Compare
|
Fixed the commit prefix, Memory checkingI said in the description that I couldn't attach Valgrind output. Valgrind is installed on this machine, it just refuses to start without glibc debuginfo, which I can't install here: So I built with Against the unpatched providers, ASan catches it on the first run and the stack is the one from the issue report: Reader on T3 is in the unlocked copy at With the patch, all six AWS credential test binaries are clean, no ASan reports and no LeakSanitizer reports: Run as The plain (non-sanitized) build is also green, and the new concurrency test segfaults immediately without the patch (rc=139) and passes 10/10 with it. If a maintainer wants actual Valgrind output on top of this I can set up a container with |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/internal/aws_credentials_sts.c (2)
814-826: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winJoin only the threads that were created.
If
pthread_createfails at Line 817,readers[i]stays uninitialized. Line 826 then passes an indeterminatepthread_ttopthread_join, which is undefined behavior. The same applies towriterat Line 821 and Line 824. Track the number of started threads, and join only those. If the writer fails to start, set the stop flag so the reader joins return.This is separate from the accepted pattern of continuing after
TEST_CHECK. The defect is the use of an uninitialized value.🛡️ Proposed fix
+ int started = 0; + for (i = 0; i < CONCURRENCY_READERS; i++) { args[i].ctx = &ctx; args[i].id = i; ret = pthread_create(&readers[i], NULL, concurrency_reader, &args[i]); - TEST_CHECK(ret == 0); + if (!TEST_CHECK(ret == 0)) { + break; + } + started++; } ret = pthread_create(&writer, NULL, concurrency_writer, &ctx); - TEST_CHECK(ret == 0); - - pthread_join(writer, NULL); - for (i = 0; i < CONCURRENCY_READERS; i++) { + if (TEST_CHECK(ret == 0)) { + pthread_join(writer, NULL); + } + else { + /* stop the readers so the joins below return */ + ctx.stop = FLB_TRUE; + } + + for (i = 0; i < started; i++) { pthread_join(readers[i], NULL);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/internal/aws_credentials_sts.c` around lines 814 - 826, Update the thread setup and cleanup around concurrency_reader and concurrency_writer to track which threads successfully started, joining only those pthread_t values. Handle writer creation failure by setting the shared stop flag so active readers can exit and be joined safely, while preserving the existing TEST_CHECK behavior.Source: Learnings
712-712: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSynchronize the
stopflag between the writer and the readers.
ctx.stopis a plainint. The writer stores it at Line 744. The readers load it at Line 712 without synchronization. The compiler can hoist the load out of the reader loop, so the readers can spin forever andpthread_joinat Line 826 can hang the suite. TSan also reports this pair as a data race, which adds noise to the signal this test produces.Use a C11 atomic for the flag, or guard it with a mutex.
🔒 Proposed fix using C11 atomics
+#include <stdatomic.h> + struct concurrency_ctx { struct flb_aws_provider *provider; - int stop; + atomic_int stop;- while (ctx->stop == FLB_FALSE) { + while (atomic_load(&ctx->stop) == FLB_FALSE) {- ctx->stop = FLB_TRUE; + atomic_store(&ctx->stop, FLB_TRUE);Also update Line 812:
- ctx.stop = FLB_FALSE; + atomic_store(&ctx.stop, FLB_FALSE);Also applies to: 744-744
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/internal/aws_credentials_sts.c` at line 712, Make the ctx->stop flag atomic and use atomic loads in the reader loop and an atomic store where the writer stops processing, including the cleanup/reset path around the referenced line 812. Update the ctx field declaration and all accesses consistently so pthread_join cannot hang and the test no longer reports a data race.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aws/flb_aws_credentials_sts.c`:
- Around line 110-133: Revalidate the cache after acquiring provider->lock in
the STS path around try_lock_provider: reread the refresh deadline and cached
credentials, and call sts_assume_role_request only when the cache is empty or
still expired; otherwise skip the redundant refresh while preserving credential
return behavior. Apply the same post-lock revalidation in
src/aws/flb_aws_credentials_sts.c lines 397-415 for the EKS path.
- Line 291: Check the return value of cache_lock pthread_mutex_init in the STS
and EKS paths at src/aws/flb_aws_credentials_sts.c:291 and :541, and in the
standard-chain cache initialization path. On failure, make
flb_sts_provider_create and the standard-chain path destroy provider->lock and
free provider; route EKS cleanup through flb_aws_provider_destroy so both
mutexes are destroyed.
---
Duplicate comments:
In `@tests/internal/aws_credentials_sts.c`:
- Around line 814-826: Update the thread setup and cleanup around
concurrency_reader and concurrency_writer to track which threads successfully
started, joining only those pthread_t values. Handle writer creation failure by
setting the shared stop flag so active readers can exit and be joined safely,
while preserving the existing TEST_CHECK behavior.
- Line 712: Make the ctx->stop flag atomic and use atomic loads in the reader
loop and an atomic store where the writer stops processing, including the
cleanup/reset path around the referenced line 812. Update the ctx field
declaration and all accesses consistently so pthread_join cannot hang and the
test no longer reports a data race.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bc36b71-0763-425e-b6f6-6d98eebb59d2
📒 Files selected for processing (7)
include/fluent-bit/flb_aws_credentials.hsrc/aws/flb_aws_credentials.csrc/aws/flb_aws_credentials_ec2.csrc/aws/flb_aws_credentials_http.csrc/aws/flb_aws_credentials_profile.csrc/aws/flb_aws_credentials_sts.ctests/internal/aws_credentials_sts.c
🚧 Files skipped from review as they are similar to previous changes (5)
- include/fluent-bit/flb_aws_credentials.h
- src/aws/flb_aws_credentials_ec2.c
- src/aws/flb_aws_credentials.c
- src/aws/flb_aws_credentials_http.c
- src/aws/flb_aws_credentials_profile.c
Every credential provider caches one flb_aws_credentials and hands out copies of it. get_credentials() made that copy without holding the provider lock, while the refresh path freed and replaced the cached struct under it. That was fine when flushes were coroutines on a single thread, but with `workers N` they run on N real pthreads, so the copy and the swap can overlap. Three ways it goes wrong: a reader dereferences a pointer the refresh already freed and takes a SIGSEGV in flb_sds_create(), a reader lands between `*creds = NULL` and the reassignment and bails out with no credentials, or a reader picks up the access key from one refresh and the secret from the next and gets InvalidSignatureException. All five providers (sts, eks, ec2, http, profile) had the same shape. The provider lock can't just be taken by readers: it is held across the network call that fetches new credentials, which yields the coroutine, so a second coroutine on the same thread blocking on it would deadlock. Add a separate cache lock that is only ever held for the copy and for the pointer swap, never across IO, and route both sides through flb_aws_cache_get_credentials() and flb_aws_cache_set_credentials(). The swap now publishes the new credentials first and frees the old ones afterwards, so there is no window where the cache is empty. next_refresh is read and written under the same lock since it is part of the same cached state. Fixes fluent#12206 Signed-off-by: Md Nahid Hassan Bhuiyan <nahid151341@gmail.com>
Six threads read credentials from an EKS provider while a seventh refreshes it in a loop. The mock STS response tags the access key, secret key and session token with the same generation number, so a reader that stitched together two different refreshes is detectable without a sanitizer. Against the unfixed providers this segfaults within a second, the same crash reported in fluent#12206. Under ASan or TSan it also flags the use-after-free directly. Signed-off-by: Md Nahid Hassan Bhuiyan <nahid151341@gmail.com>
1e86b00 to
1492bd8
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/internal/aws_credentials_sts.c (2)
675-745: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSynchronize the
stopflag between the writer and the readers.
ctx.stopis a plainint. The writer stores it at Line 744. The readers load it at Line 712 without synchronization. The compiler can hoist the load out of the reader loop, because nothing in the loop body touchesctx.stop. The readers then never exit andpthread_joinat Line 826 hangs the suite. The comment at Lines 668-669 also asks the maintainer to run this test under TSan, and TSan reports this store/load pair as a data race.Use a C11 atomic for the flag, or guard it with a mutex.
🔒 Proposed fix using C11 atomics
+#include <stdatomic.h> + struct concurrency_ctx { struct flb_aws_provider *provider; - int stop; + atomic_int stop;- while (ctx->stop == FLB_FALSE) { + while (atomic_load(&ctx->stop) == FLB_FALSE) {- ctx->stop = FLB_TRUE; + atomic_store(&ctx->stop, FLB_TRUE);Also update the initialization at Line 812.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/internal/aws_credentials_sts.c` around lines 675 - 745, Make concurrency_ctx.stop an atomic flag and use atomic load/store operations in concurrency_reader and concurrency_writer so readers reliably observe termination without a data race. Update the stop initialization in the test setup to use the atomic initialization form, preserving the existing reader loop and writer completion behavior.
814-826: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDo not join threads that were not created.
If
pthread_createfails at Line 817,readers[i]stays uninitialized. Line 826 then passes an indeterminatepthread_ttopthread_join, which is undefined behavior. The same applies towriterat Line 821 and Line 824. Track the number of threads that started, and join only those. If the writer fails to start, setctx.stopso the readers exit.🛡️ Proposed fix
+ int started = 0; + for (i = 0; i < CONCURRENCY_READERS; i++) { args[i].ctx = &ctx; args[i].id = i; ret = pthread_create(&readers[i], NULL, concurrency_reader, &args[i]); - TEST_CHECK(ret == 0); + if (!TEST_CHECK(ret == 0)) { + break; + } + started++; } ret = pthread_create(&writer, NULL, concurrency_writer, &ctx); - TEST_CHECK(ret == 0); - - pthread_join(writer, NULL); - for (i = 0; i < CONCURRENCY_READERS; i++) { + if (TEST_CHECK(ret == 0)) { + pthread_join(writer, NULL); + } + else { + ctx.stop = FLB_TRUE; + } + + for (i = 0; i < started; i++) { pthread_join(readers[i], NULL);This is separate from the accepted pattern of continuing after
TEST_CHECK. The problem is the use of an indeterminate value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/internal/aws_credentials_sts.c` around lines 814 - 826, Update the thread setup and cleanup around concurrency_reader and concurrency_writer to track which pthread_create calls succeed, join only successfully created threads, and avoid passing uninitialized pthread_t values to pthread_join. If pthread_create fails for writer, set ctx.stop so active readers terminate; preserve the existing TEST_CHECK behavior for reporting failures.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tests/internal/aws_credentials_sts.c`:
- Around line 675-745: Make concurrency_ctx.stop an atomic flag and use atomic
load/store operations in concurrency_reader and concurrency_writer so readers
reliably observe termination without a data race. Update the stop initialization
in the test setup to use the atomic initialization form, preserving the existing
reader loop and writer completion behavior.
- Around line 814-826: Update the thread setup and cleanup around
concurrency_reader and concurrency_writer to track which pthread_create calls
succeed, join only successfully created threads, and avoid passing uninitialized
pthread_t values to pthread_join. If pthread_create fails for writer, set
ctx.stop so active readers terminate; preserve the existing TEST_CHECK behavior
for reporting failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5abc0d11-ab44-4307-b488-270c0babbd7f
📒 Files selected for processing (7)
include/fluent-bit/flb_aws_credentials.hsrc/aws/flb_aws_credentials.csrc/aws/flb_aws_credentials_ec2.csrc/aws/flb_aws_credentials_http.csrc/aws/flb_aws_credentials_profile.csrc/aws/flb_aws_credentials_sts.ctests/internal/aws_credentials_sts.c
🚧 Files skipped from review as they are similar to previous changes (6)
- include/fluent-bit/flb_aws_credentials.h
- src/aws/flb_aws_credentials_http.c
- src/aws/flb_aws_credentials_sts.c
- src/aws/flb_aws_credentials_profile.c
- src/aws/flb_aws_credentials.c
- src/aws/flb_aws_credentials_ec2.c
|
The unit test job failure here is Looks like the known flakiness that #12218 is fixing: the Loki mock server only handles |
1 participant
Every AWS credential provider keeps one cached
flb_aws_credentialsand hands out copies of it.get_credentials()made that copy without holding the provider lock, while the refresh path freed and replaced the cached struct under it. That was safe back when flushes were coroutines on one thread, but withworkers Nthey run on N real pthreads, so the copy and the swap overlap.Three ways it goes wrong:
flb_sds_create()*creds = NULLand the reassignment and comes back with nothingInvalidSignatureExceptionwith no crashSame shape in all five providers: sts, eks, ec2, http, profile. So ECS, EC2 instance role and profile deployments are exposed too, not just EKS/IRSA.
Readers can't just take
provider->lock. It's held across the network call that fetches new credentials, and that call yields the coroutine, so a second coroutine on the same thread blocking on it would deadlock. That's why the code uses trylock in the first place.So this adds a second mutex,
provider->cache_lock, that is only ever held for the copy and for the pointer swap and never across IO, which makes a blocking lock safe. Both sides now go through helpers:flb_aws_cache_get_credentials()copies all three fields under the lockflb_aws_cache_set_credentials()publishes the new credentials first and frees the old ones after releasing the lock, so the cache is never empty mid-swapflb_aws_cache_get_refresh_time()readsnext_refresh, which is part of the same cached stateLock order is always
lockthencache_lock, never the other way round.The copy block was duplicated in all five providers, so collapsing it into one helper removes more code than it adds.
Fixes #12206
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:
Valgrind is installed here but refuses to start without glibc debuginfo, which I can't install on this box, so I used ASan with LeakSanitizer instead. For a use-after-free across threads that's the better tool anyway, since memcheck serialises threads and would likely never hit the window. Full ASan output is in a comment below: it reproduces the exact stack from the issue against the unpatched code, and all six AWS credential test binaries come back clean with the patch.
I also added
eks_credential_provider_concurrent_refreshtotests/internal/aws_credentials_sts.c. Six reader threads callget_credentials()in a loop while a seventh refreshes the provider 3000 times. The mock STS response tags the access key, secret key and session token with the same generation number, so a set that was stitched together from two refreshes is detectable without a sanitizer.Against the unpatched providers it segfaults within a second, which is the crash from the issue:
With the fix in place it passes, and so does everything else:
Ran the concurrency test 10 more times to check it isn't flaky, no failures. Under ASan or TSan it also flags the use-after-free directly rather than just the torn read.
The reproducer config from the issue (tail -> cloudwatch_logs with
workers 25on EKS with IRSA) is the real world case this covers.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
No user visible behaviour change, nothing to document.
Backporting
The code is identical from 1.8 through current master, so every supported release is affected.
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
Bug Fixes
Tests