Skip to content

libexpr-c: fix changing eval settings having no effect #310

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
numinit wants to merge 1 commit into
base: main
Choose a base branch
from
Open

libexpr-c: fix changing eval settings having no effect #310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions src/libcmd/common-eval-args.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,8 @@

namespace nix {

EvalSettings evalSettings{
settings.readOnlyMode,
{
static GlobalConfig::Register rEvalSettings(&evalSettings, [] {
evalSettings.lookupPathHooks = {
{
"flake",
[](EvalState & state, std::string_view rest) {
Expand All @@ -40,10 +39,8 @@ EvalSettings evalSettings{
return state.storePath(storePath);
},
},
},
};

static GlobalConfig::Register rEvalSettings(&evalSettings);
};
});

flake::Settings flakeSettings;

Expand Down
17 changes: 9 additions & 8 deletions src/libexpr-c/nix_api_expr.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "nix/expr/eval-gc.hh"
#include "nix/store/globals.hh"
#include "nix/expr/eval-settings.hh"
#include "nix/fetchers/fetch-settings.hh"
#include "nix/util/ref.hh"

#include "nix_api_expr.h"
Expand Down Expand Up @@ -137,8 +138,8 @@ nix_eval_state_builder * nix_eval_state_builder_new(nix_c_context * context, Sto
return unsafe_new_with_self<nix_eval_state_builder>([&](auto * self) {
return nix_eval_state_builder{
.store = nix::ref<nix::Store>(store->ptr),
.settings = nix::EvalSettings{/* &bool */ self->readOnlyMode},
.fetchSettings = nix::fetchers::Settings{},
.settings = &nix::evalSettings,
.fetchSettings = &nix::fetchSettings,
.readOnlyMode = true,
};
});
Expand All @@ -159,9 +160,9 @@ nix_err nix_eval_state_builder_load(nix_c_context * context, nix_eval_state_buil
context->last_err_code = NIX_OK;
try {
// TODO: load in one go?
builder->settings.readOnlyMode = nix::settings.readOnlyMode;
loadConfFile(builder->settings);
loadConfFile(builder->fetchSettings);
builder->settings->readOnlyMode = nix::settings.readOnlyMode;
loadConfFile(*builder->settings);
loadConfFile(*builder->fetchSettings);
}
NIXC_CATCH_ERRS
}
Expand All @@ -188,9 +189,9 @@ EvalState * nix_eval_state_build(nix_c_context * context, nix_eval_state_builder
try {
return unsafe_new_with_self<EvalState>([&](auto * self) {
return EvalState{
.fetchSettings = std::move(builder->fetchSettings),
.settings = std::move(builder->settings),
.state = nix::EvalState(builder->lookupPath, builder->store, self->fetchSettings, self->settings),
.fetchSettings = builder->fetchSettings,
.settings = builder->settings,
.state = nix::EvalState(builder->lookupPath, builder->store, *self->fetchSettings, *self->settings),
};
});
}
Expand Down
8 changes: 4 additions & 4 deletions src/libexpr-c/nix_api_expr_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,17 @@ extern "C" {
struct nix_eval_state_builder
{
nix::ref<nix::Store> store;
nix::EvalSettings settings;
nix::fetchers::Settings fetchSettings;
nix::EvalSettings * settings;
nix::fetchers::Settings * fetchSettings;
nix::LookupPath lookupPath;
// TODO: make an EvalSettings setting own this instead?
bool readOnlyMode;
};

struct EvalState
{
nix::fetchers::Settings fetchSettings;
nix::EvalSettings settings;
nix::fetchers::Settings * fetchSettings;
nix::EvalSettings * settings;
nix::EvalState state;
};

Expand Down
5 changes: 5 additions & 0 deletions src/libexpr/eval-settings.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "nix/util/users.hh"
#include "nix/util/config-global.hh"
#include "nix/store/globals.hh"
#include "nix/store/profiles.hh"
#include "nix/expr/eval.hh"
Expand Down Expand Up @@ -118,4 +119,8 @@ std::filesystem::path getNixDefExpr()
return settings.useXDGBaseDirectories ? getStateDir() / "defexpr" : getHome() / ".nix-defexpr";
}

EvalSettings evalSettings{settings.readOnlyMode};
Copy link

@coderabbitai coderabbitai bot Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find the definition of the global settings object

# Search for the definition of settings global object
rg -n -C5 '^Settings settings' --type=cpp

# Also search for extern declarations
rg -n 'extern.*Settings.*settings' --type=cpp -g '*.hh' -g '*.h'

Repository: DeterminateSystems/nix-src

Length of output: 793

🏁 Script executed:

# Find EvalSettings class definition and constructor
rg -n 'class EvalSettings' --type=cpp -A 10

# Also search for the constructor implementation
rg -n 'EvalSettings::EvalSettings' --type=cpp -A 5

Repository: DeterminateSystems/nix-src

Length of output: 536

🏁 Script executed:

# Find EvalSettings class header definition
rg -n 'class EvalSettings' --type=cpp -B 5 -A 20 src/libexpr/

# Also check the header file
fd 'eval-settings.hh' -x cat {}

Repository: DeterminateSystems/nix-src

Length of output: 16493

🏁 Script executed:

# Check the Register call in eval-settings.cc around line 124
sed -n '120,130p' src/libexpr/eval-settings.cc

# Check what GlobalConfig::Register does and if it provides ordering guarantees
rg -n 'class.*Register|struct.*Register' src/libutil/ --type=cpp -A 5

Repository: DeterminateSystems/nix-src

Length of output: 638

🏁 Script executed:

# Find the Register implementation
rg -n 'GlobalConfig::Register::Register' src/libutil/ --type=cpp -A 10

# Also check if there's any lazy initialization or factory pattern
rg -n 'evalSettings' src/libexpr/eval-settings.cc -B 5 -A 5

Repository: DeterminateSystems/nix-src

Length of output: 1490

Address static initialization order dependency between evalSettings and settings.

The global evalSettings stores a reference to settings.readOnlyMode. Since settings is defined in src/libstore/globals.cc and evalSettings in src/libexpr/eval-settings.cc, the C++ standard leaves their initialization order undefined. If evalSettings initializes before settings, it will hold a dangling reference.

Consider using lazy initialization (e.g., a factory function or callback mechanism) to defer creation of evalSettings until settings is guaranteed to be initialized.

🤖 Prompt for AI Agents 
In @src/libexpr/eval-settings.cc around line 122, The global evalSettings
currently binds to settings.readOnlyMode at static init time, risking a dangling
reference due to undefined init order between evalSettings and settings; change
evalSettings to be lazily initialized (e.g., replace the global EvalSettings
evalSettings with an accessor like getEvalSettings() that creates a
function-local static EvalSettings and reads settings.readOnlyMode at first use)
or store the readOnlyMode value (or a safe pointer) inside EvalSettings instead
of a reference; ensure the new code references the existing EvalSettings type
and the settings.readOnlyMode symbol so callers use getEvalSettings() (or the
new safe construct) everywhere evalSettings was used.


static GlobalConfig::Register rEvalSettings(&evalSettings);

} // namespace nix
5 changes: 5 additions & 0 deletions src/libexpr/include/nix/expr/eval-settings.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,4 +412,9 @@ std::filesystem::path getNixDefExpr();
*/
constexpr size_t evalStackSize = 60 * 1024 * 1024;

/**
* EvalSettings instance from libexpr.
*/
extern EvalSettings evalSettings;

} // namespace nix
2 changes: 1 addition & 1 deletion src/libflake-c/nix_api_flake.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ nix_err nix_flake_settings_add_to_eval_state_builder(
{
nix_clear_err(context);
try {
settings->settings->configureEvalSettings(builder->settings);
settings->settings->configureEvalSettings(*builder->settings);
}
NIXC_CATCH_ERRS
}
Expand Down
11 changes: 10 additions & 1 deletion src/libutil/config-global.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,16 @@ GlobalConfig globalConfig;

GlobalConfig::Register::Register(Config * config)
{
configRegistrations().emplace_back(config);
auto regs = configRegistrations();
if (std::find(regs.begin(), regs.end(), config) == regs.end()) {
configRegistrations().emplace_back(config);
}
}

GlobalConfig::Register::Register(Config * config, std::function<void()> && callback)
: Register(config)
{
callback();
}

ExperimentalFeatureSettings experimentalFeatureSettings;
Expand Down
1 change: 1 addition & 0 deletions src/libutil/include/nix/util/config-global.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ struct GlobalConfig : public AbstractConfig
struct Register
{
Register(Config * config);
Register(Config * config, std::function<void()> && callback);
};
};

Expand Down