From f0e1f652607a4423ac10393cdb9250f15fead512 Mon Sep 17 00:00:00 2001 From: Bernardo Meurer Costa Date: Sun, 12 Oct 2025 02:00:27 +0000 Subject: [PATCH 1/3] fix(libstore): fix race condition in AWS credential provider caching The previous implementation had a check-then-create race condition where multiple threads could simultaneously: 1. Check the cache and find no provider (line 122) 2. Create their own providers (lines 126-145) 3. Insert into cache (line 161) This resulted in multiple credential providers being created when downloading multiple packages in parallel, as each .narinfo download would trigger provider creation on its own thread. Fix by using boost::concurrent_flat_map's try_emplace_and_cvisit, which provides atomic get-or-create semantics: - f1 callback: Called atomically during insertion, creates the provider - f2 callback: Called if key exists, returns cached provider - Other threads are blocked during f1, so no nullptr is ever visible --- src/libstore/aws-creds.cc | 85 ++++++++++++++++++++++----------------- 1 file changed, 47 insertions(+), 38 deletions(-) diff --git a/src/libstore/aws-creds.cc b/src/libstore/aws-creds.cc index cd404a554..05c11d24a 100644 --- a/src/libstore/aws-creds.cc +++ b/src/libstore/aws-creds.cc @@ -118,48 +118,57 @@ AwsCredentials getAwsCredentials(const std::string & profile) // Get or create credential provider with caching std::shared_ptr provider; - // Try to find existing provider - credentialProviderCache.visit(profile, [&](const auto & pair) { provider = pair.second; }); + // Use try_emplace_and_cvisit for atomic get-or-create + // This prevents race conditions where multiple threads create providers + credentialProviderCache.try_emplace_and_cvisit( + profile, + nullptr, // Placeholder - will be replaced in f1 before any thread can see it + [&](auto & kv) { + // f1: Called atomically during insertion with non-const reference + // Other threads are blocked until we finish, so nullptr is never visible + debug( + "[pid=%d] creating new AWS credential provider for profile '%s'", + getpid(), + profile.empty() ? "(default)" : profile.c_str()); - if (!provider) { - // Create new provider if not found - debug( - "[pid=%d] creating new AWS credential provider for profile '%s'", - getpid(), - profile.empty() ? "(default)" : profile.c_str()); + try { + initAwsCrt(); - try { - initAwsCrt(); + if (profile.empty()) { + Aws::Crt::Auth::CredentialsProviderChainDefaultConfig config; + config.Bootstrap = Aws::Crt::ApiHandle::GetOrCreateStaticDefaultClientBootstrap(); + kv.second = Aws::Crt::Auth::CredentialsProvider::CreateCredentialsProviderChainDefault(config); + } else { + Aws::Crt::Auth::CredentialsProviderProfileConfig config; + config.Bootstrap = Aws::Crt::ApiHandle::GetOrCreateStaticDefaultClientBootstrap(); + // This is safe because the underlying C library will copy this string + // c.f. https://github.com/awslabs/aws-c-auth/blob/main/source/credentials_provider_profile.c#L220 + config.ProfileNameOverride = Aws::Crt::ByteCursorFromCString(profile.c_str()); + kv.second = Aws::Crt::Auth::CredentialsProvider::CreateCredentialsProviderProfile(config); + } - if (profile.empty()) { - Aws::Crt::Auth::CredentialsProviderChainDefaultConfig config; - config.Bootstrap = Aws::Crt::ApiHandle::GetOrCreateStaticDefaultClientBootstrap(); - provider = Aws::Crt::Auth::CredentialsProvider::CreateCredentialsProviderChainDefault(config); - } else { - Aws::Crt::Auth::CredentialsProviderProfileConfig config; - config.Bootstrap = Aws::Crt::ApiHandle::GetOrCreateStaticDefaultClientBootstrap(); - // This is safe because the underlying C library will copy this string - // c.f. https://github.com/awslabs/aws-c-auth/blob/main/source/credentials_provider_profile.c#L220 - config.ProfileNameOverride = Aws::Crt::ByteCursorFromCString(profile.c_str()); - provider = Aws::Crt::Auth::CredentialsProvider::CreateCredentialsProviderProfile(config); + if (!kv.second) { + throw AwsAuthError( + "Failed to create AWS credentials provider for %s", + profile.empty() ? "default profile" : fmt("profile '%s'", profile)); + } + + provider = kv.second; + } catch (Error & e) { + // Exception during creation - remove the entry to allow retry + credentialProviderCache.erase(profile); + e.addTrace({}, "for AWS profile: %s", profile.empty() ? "(default)" : profile); + throw; + } catch (...) { + // Non-Error exception - still need to clean up + credentialProviderCache.erase(profile); + throw; } - } catch (Error & e) { - e.addTrace( - {}, - "while creating AWS credentials provider for %s", - profile.empty() ? "default profile" : fmt("profile '%s'", profile)); - throw; - } - - if (!provider) { - throw AwsAuthError( - "Failed to create AWS credentials provider for %s", - profile.empty() ? "default profile" : fmt("profile '%s'", profile)); - } - - // Insert into cache (try_emplace is thread-safe and won't overwrite if another thread added it) - credentialProviderCache.try_emplace(profile, provider); - } + }, + [&](const auto & kv) { + // f2: Called if key already exists (const reference) + provider = kv.second; + }); return getCredentialsFromProvider(provider); } From 6db86389ce89ac777d297e463021e549d6838d93 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 10 Oct 2025 19:08:38 +0200 Subject: [PATCH 2/3] util/error: Document addTrace params ... and rename e -> pos. That was weird. --- src/libutil/include/nix/util/error.hh | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/libutil/include/nix/util/error.hh b/src/libutil/include/nix/util/error.hh index e564ca5b9..49dd75991 100644 --- a/src/libutil/include/nix/util/error.hh +++ b/src/libutil/include/nix/util/error.hh @@ -192,13 +192,23 @@ public: err.traces.push_front(trace); } + /** + * @param pos Nullable `shared_ptr` + * @param fs Format string, see `HintFmt` + * @param args... Format string arguments. + */ template - void addTrace(std::shared_ptr && e, std::string_view fs, const Args &... args) + void addTrace(std::shared_ptr && pos, std::string_view fs, const Args &... args) { - addTrace(std::move(e), HintFmt(std::string(fs), args...)); + addTrace(std::move(pos), HintFmt(std::string(fs), args...)); } - void addTrace(std::shared_ptr && e, HintFmt hint, TracePrint print = TracePrint::Default); + /** + * @param pos Nullable `shared_ptr` + * @param hint Formatted error message + * @param print Optional, whether to always print (e.g. `addErrorContext`) + */ + void addTrace(std::shared_ptr && pos, HintFmt hint, TracePrint print = TracePrint::Default); bool hasTrace() const { From 48a5e2dde2625ebb0d7f6aa2e77051e152fb3411 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 13 Oct 2025 13:14:05 +0200 Subject: [PATCH 3/3] EvalState: add doc comment --- src/libexpr/include/nix/expr/eval.hh | 9 ++++++++- src/libutil/include/nix/util/error.hh | 10 +++++++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/libexpr/include/nix/expr/eval.hh b/src/libexpr/include/nix/expr/eval.hh index b87c45ce3..76ce62b87 100644 --- a/src/libexpr/include/nix/expr/eval.hh +++ b/src/libexpr/include/nix/expr/eval.hh @@ -508,8 +508,15 @@ private: public: + /** + * @param lookupPath Only used during construction. + * @param store The store to use for instantiation + * @param fetchSettings Must outlive the lifetime of this EvalState! + * @param settings Must outlive the lifetime of this EvalState! + * @param buildStore The store to use for builds ("import from derivation", C API `nix_string_realise`) + */ EvalState( - const LookupPath & _lookupPath, + const LookupPath & lookupPath, ref store, const fetchers::Settings & fetchSettings, const EvalSettings & settings, diff --git a/src/libutil/include/nix/util/error.hh b/src/libutil/include/nix/util/error.hh index 49dd75991..cc8460592 100644 --- a/src/libutil/include/nix/util/error.hh +++ b/src/libutil/include/nix/util/error.hh @@ -193,7 +193,9 @@ public: } /** - * @param pos Nullable `shared_ptr` + * Prepends an item to the error trace, as is usual for extra context. + * + * @param pos Nullable source position to put in trace item * @param fs Format string, see `HintFmt` * @param args... Format string arguments. */ @@ -204,9 +206,11 @@ public: } /** - * @param pos Nullable `shared_ptr` + * Prepends an item to the error trace, as is usual for extra context. + * + * @param pos Nullable source position to put in trace item * @param hint Formatted error message - * @param print Optional, whether to always print (e.g. `addErrorContext`) + * @param print Optional, whether to always print (used by `addErrorContext`) */ void addTrace(std::shared_ptr && pos, HintFmt hint, TracePrint print = TracePrint::Default);