libstore/binary-cache: fix catching JSON exceptions

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 <maximilian@mbosch.me>
Change-Id: I8df5befd06c4a449072b987f82a67bc4437e7e49
Signed-off-by: Raito Bezarius <raito@lix.systems>

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<JSON::exception>()) {
-                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()
+            );
         }
     }
 

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"
 }

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 <<EOF
         lasjdöaxnasd

tests/functional/nar-access.sh

@@ -1,8 +1,9 @@
 source common.sh
 
 echo "building test path"
+TEST_FILES_ROOT="$PWD"
+
 storePath="$(nix-build nar-access.nix -A a --no-out-link)"
-strangerStorePath="$(nix-build nar-access.nix -A a --arg nonUtf8Inode true --no-out-link)"
 
 cd "$TEST_ROOT"
 
@@ -61,11 +62,16 @@ if nix-store --dump $storePath >/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