From 3c614a136b26ab16e462811439b9c24ae444ae98 Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Sun, 22 Jun 2025 00:58:16 +0200 Subject: [PATCH] libstore/binary-cache: fix catching JSON exceptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We were catching ForeignExceptions believing it came from the TRY_AWAIT handler, but this was misguided. `j.dump()` is evaluated in synchronous context, outside of the `try { ... } catch (...)` block from `TRY_AWAIT`. Therefore, we need to use `JSON::Exception` directly. The previous test case did not catch it because: (1) https://git.lix.systems/lix-project/lix/issues/865 hid the fact that `--arg` was wrong. (2) we did not grep for the warning because… we were not even copying the strange store path to the binary cache. (3) checking for the NAR happened after the NAR directory was emptied for test reasons and this was not even caught neither. Anyway, the test case was completely busted and has now been tested without this commit and after this commit and we can confirm that prior to this commit, the test will fail with an exception trace. Co-authored-by: Maximilian Bosch Change-Id: I8df5befd06c4a449072b987f82a67bc4437e7e49 Signed-off-by: Raito Bezarius --- lix/libstore/binary-cache-store.cc | 26 +++++++++---------- .../common/vars-and-functions.sh.in | 8 ++++++ tests/functional/nar-access.nix | 2 +- tests/functional/nar-access.sh | 22 ++++++++++------ 4 files changed, 36 insertions(+), 22 deletions(-) diff --git a/lix/libstore/binary-cache-store.cc b/lix/libstore/binary-cache-store.cc index 61d4db872..5d4cbfcc8 100644 --- a/lix/libstore/binary-cache-store.cc +++ b/lix/libstore/binary-cache-store.cc @@ -200,19 +200,19 @@ try { }; try { - TRY_AWAIT(upsertFile( - std::string(info.path.hashPart()) + ".ls", j.dump(), "application/json", context - )); - } catch (ForeignException & exc) { - if (exc.is()) { - warn( - "Skipping NAR listing for path '%1%' due to serialization failure: %2%", - printStorePath(narInfo->path), - exc.what() - ); - } else { - throw exc; - } + TRY_AWAIT( + upsertFile(std::string(info.path.hashPart()) + ".ls", j.dump(), "application/json", context) + ); + // FIXME(raito): use a ForeignException here by wrapping basic_json in a JSON type that + // wraps all non-exception-free methods into methods that throws ForeignExceptions + // instead. + // NOLINTNEXTLINE(lix-foreign-exceptions): see above + } catch (JSON::exception & exc) { + warn( + "Skipping NAR listing for path '%1%' due to serialization failure: %2%", + printStorePath(narInfo->path), + exc.what() + ); } } diff --git a/tests/functional/common/vars-and-functions.sh.in b/tests/functional/common/vars-and-functions.sh.in index f6326e6fd..aa2eee6f5 100644 --- a/tests/functional/common/vars-and-functions.sh.in +++ b/tests/functional/common/vars-and-functions.sh.in @@ -148,6 +148,10 @@ if [[ $(uname) == Linux ]] && [[ -L /proc/self/ns/user ]] && unshare --user true _canUseSandbox=1 fi +if touch `echo -e $(mktemp -d)/$'\322'.txt`; then + _canWriteNonUtf8Inodes=1 +fi + isDaemonNewer () { [[ -n "${NIX_DAEMON_PACKAGE:-}" ]] || return 0 local requiredVersion="$1" @@ -173,6 +177,10 @@ canUseSandbox() { [[ ${_canUseSandbox-} ]] } +canWriteNonUtf8Inodes() { + [[ ${_canWriteNonUtf8Inodes-} ]] +} + requireSandboxSupport () { canUseSandbox || skipTest "Sandboxing not supported" } diff --git a/tests/functional/nar-access.nix b/tests/functional/nar-access.nix index 6756291b4..490c240f4 100644 --- a/tests/functional/nar-access.nix +++ b/tests/functional/nar-access.nix @@ -14,7 +14,7 @@ rec { touch $out/qux mkdir $out/zyx - ${if nonUtf8Inodes then ''printf "data" > "$out/invalid-\x80file"'' else ""} + ${if nonUtf8Inodes then ''touch `echo -e $out/$'\322'.txt`'' else ""} cat >$out/foo/data </dev/full ; then exit -1 fi - # Test reading from remote nar listings if available - nix copy --to "file://$cacheDir?write-nar-listing=true" $storePath +if canWriteNonUtf8Inodes; then + strangerStorePath="$(nix-build "$TEST_FILES_ROOT/nar-access.nix" -A a --arg nonUtf8Inodes true --no-out-link)" + expect 0 nix copy --to "file://$cacheDir?write-nar-listing=true" $strangerStorePath 2>&1 | grep "warning: Skipping NAR listing for path" + # Confirm that NARs with non-UTF8 inodes can still be listed + expect 0 nix store ls $strangerStorePath/ --store "file://$cacheDir" +fi + export _NIX_FORCE_HTTP=1 diff -u \ @@ -75,14 +81,14 @@ diff -u \ <(nix store ls --json -R $storePath/foo/bar --store "file://$cacheDir" | jq -S) \ <(echo '{"narOffset": 368,"type":"regular","size":0}' | jq -S) + # Confirm that we are reading from ".ls" file by deleting the nar rm -rf $cacheDir/nar diff -u \ <(nix store ls --json -R $storePath/foo/bar --store "file://$cacheDir" | jq -S) \ <(echo '{"narOffset": 368,"type":"regular","size":0}' | jq -S) -# Confirm that there's no more than one `.ls` in the `$cacheDir` because non-UTF8 inodes cannot have `.ls` generated for them. -[[ $(find $cacheDir -type f -name '*.ls' | wc -l) -eq 1 ]] || (echo "Expected at most one listing file in $cacheDir, found more"; exit -1) - -# Confirm that NARs with non-UTF8 inodes can still be listed -expect 0 nix store ls $strangerStorePath/ --store "file://$cacheDir" +if canWriteNonUtf8Inodes; then + # Confirm that there's no more than one `.ls` in the `$cacheDir` because non-UTF8 inodes cannot have `.ls` generated for them. + [[ $(find $cacheDir -type f -name '*.ls' | wc -l) -eq 1 ]] || (echo "Expected at most one listing file in $cacheDir, found more"; exit -1) +fi