libstore: read SSL_CERT_FILE after main, not in a static initializer - #16383
libstore: read SSL_CERT_FILE after main, not in a static initializer#16383awsmadi wants to merge 2 commits into
Conversation
Follow-up to review feedback on NixOS#16364 from xokdvium: that change caught the symptom rather than removing it. NIX_SSL_CERT_FILE / SSL_CERT_FILE were read in FileTransferSettings' constructor, and that object is a global, so the read ran during static initialization. A bad value could not be reported there: on Unix an escaping exception reaches std::terminate before main, and on Windows the loader absorbs it and the process dies having printed nothing at all (NixOS#16356). NixOS#16364 worked around that by catching and warning. Every other config source is converted post-main through Setting::set(), so apply this one there too, in loadConfFile() next to the existing NIX_CONFIG handling. A non-absolute path is then an ordinary reportable error again rather than a startup warning, which is what the trace text added in NixOS#16364 said should eventually happen, and the constructor's try/catch is no longer needed. Applied before the config files, so an ssl-cert-file in nix.conf keeps the precedence it had when the constructor assigned during static initialization. Note that callers using initLibStore(false) no longer get the override, where before it applied unconditionally. Assisted-by: Claude Code (claude-opus-5)
Ericson2314
left a comment
There was a problem hiding this comment.
Hehe @xokdvium I asked it not to move this code before, because I didn't like where it moved it, but I like this new location better. Let's ship it
| if (auto sslCertFile = getEnvOsNonEmpty(OS_STR("NIX_SSL_CERT_FILE")).or_else([] { | ||
| return getEnvOsNonEmpty(OS_STR("SSL_CERT_FILE")); | ||
| })) |
There was a problem hiding this comment.
Actually agent, the way you did *NonEmpty changes the semantics subtly. We do not want to fall back to the next variable if it is set, even if it is empty.
There was a problem hiding this comment.
You're right, and it's a real regression rather than just a wording thing. getEnvOsNonEmpty returns {}
for set-but-empty, so it is indistinguishable from unset and or_else falls through. The original chain did
not: or_else saw NIX_SSL_CERT_FILE as present, so SSL_CERT_FILE was never consulted, and the empty
check afterwards turned it into "no override" — an empty NIX_SSL_CERT_FILE suppresses the fallback.
Restored getEnvOs with the empty check after the fallback, so it is the original chain relocated rather
than a rewrite. Verified against master: NIX_SSL_CERT_FILE= with SSL_CERT_FILE set gives the built-in
default on both, where my previous version picked up SSL_CERT_FILE.
Both from Ericson2314's review. getEnvOsNonEmpty reports set-but-empty and unset identically, so getEnvOsNonEmpty(NIX_SSL_CERT_FILE).or_else(getEnvOsNonEmpty(SSL_CERT_FILE)) fell through to SSL_CERT_FILE when NIX_SSL_CERT_FILE was set to the empty string. The original chain did not: or_else saw the variable as present, so the empty check afterwards yielded "no override" and SSL_CERT_FILE stayed suppressed. Restored getEnvOs with the empty check after the fallback. Also restored the addTrace, as add-trace-and-rethrow rather than the add-trace-and-warn it was in NixOS#16364, so an invalid value still says which environment variable it came from: error: … while applying the 'NIX_SSL_CERT_FILE' or 'SSL_CERT_FILE' environment variable error: not an absolute path: "rel/ca.crt" Measured against master: empty-suppresses-fallback, both-unset, and NIX_-unset-SSL_-set all agree; only the invalid-value case differs, which is the point of the change. Assisted-by: Claude Code (claude-opus-5)
|
The red It fails in The artifact it wanted is present and unexpired — run 33135116639 (master For what it's worth the previous head of this PR, Update — discriminated rather than assumed. Merge-queue run 33195884177, created 15 minutes after this |
|
@Ericson2314 both points from your review are already in The if (auto sslCertFile = getEnvOs(OS_STR("NIX_SSL_CERT_FILE"))
.or_else([] { return getEnvOs(OS_STR("SSL_CERT_FILE")); })
.and_then([](OsString s) -> std::optional<OsString> {
return s.empty() ? std::nullopt : std::optional{std::move(s)};
})) {
The On the red CI, which is not this change. The only failing run on this head is 33194720856, and the job that failed is That action downloads a prebuilt installer and compiles nothing, so no content of For what it is worth the same step passed on merge-queue run 33195884177 fifteen minutes later under identical CI config, which says the step is flaky rather than broken. I should be straight that this is weaker evidence than it looks: that run's head is I do not have re-run rights on this repo. A re-run of 33194720856 is all this needs; if it would be easier I am happy to push an empty commit to retrigger instead, just say which you prefer. |
|
The red That also means the failure is not specific to me: once the newest successful master run ages past the artifact retention window, A re-run should clear it and I can't trigger one. The two review points are already in |
3869842 to
4cac5cc
Compare
|
@awsmadi Good new this is passing CI now. Can you rebase and squash down the fixup? |
Follow-up to xokdvium's review comment on the merged #16364, which
asked whether we could avoid running this
in a static initializer rather than defending against it. That's the right call, and this does it.
NIX_SSL_CERT_FILE/SSL_CERT_FILEwere read inFileTransferSettings' constructor. That object is aglobal, so the read ran during static initialization, where a bad value cannot be reported: on Unix an
escaping exception reaches
std::terminatebeforemain, and on Windows the loader absorbs it and theprocess dies having printed nothing at all (#16356). #16364 worked around that by catching and warning.
Every other config source is converted post-
mainthroughSetting::set(), so this one is now appliedthere too — in
loadConfFile(), next to the existingNIX_CONFIGhandling. The constructor goes awayentirely, along with its
try/catch.Behaviour
A non-absolute value is an ordinary reportable error again, which is what the trace text added in #16364
said should eventually happen:
Everything else is unchanged. Measured with
nix config show ssl-cert-fileagainst master:The override is applied before the config files so that
nix.confkeeps the precedence it had when theconstructor assigned during static initialization — verified above rather than assumed.
One behaviour change worth flagging
Callers using
initLibStore(false)no longer get the override, where previously it appliedunconditionally because it happened in a constructor. That seems consistent with asking not to load config,
but it is a change.
Verified
nix build .#checks.x86_64-linux.pre-commit(7/7 hooks, no diff) and that bothnix-storeandnix-store-x86_64-w64-mingw32compile.