From 39a1e248c9e7adea312c5b7e1743a8e55da63e6c Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sat, 6 Apr 2024 22:08:58 +0200 Subject: [PATCH] libutil: remove sinkToSource eof callback this is only used in one place, and only to set a nicer error message on EndOfFile. the only caller that actually *catches* this exception should provide an error message in that catch block rather than forcing support for setting error message so deep into the stack. copyStorePath is never called outside of PathSubstitutionGoal anyway, which catches everything. Change-Id: Ifbae8706d781c388737706faf4c8a8b7917ca278 --- src/libstore/build/substitution-goal.cc | 12 ++++++-- src/libstore/store-api.cc | 2 -- src/libutil/serialise.cc | 15 +++++----- src/libutil/serialise.hh | 6 +--- tests/functional/meson.build | 1 + tests/functional/substitute-truncated-nar.sh | 29 ++++++++++++++++++++ 6 files changed, 48 insertions(+), 17 deletions(-) create mode 100644 tests/functional/substitute-truncated-nar.sh diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index d8d9ba283..cc4cb3c8c 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -217,6 +217,7 @@ void PathSubstitutionGoal::tryToRun() promise = std::promise(); thr = std::thread([this]() { + auto & fetchPath = subPath ? *subPath : storePath; try { ReceiveInterrupts receiveInterrupts; @@ -226,10 +227,17 @@ void PathSubstitutionGoal::tryToRun() Activity act(*logger, actSubstitute, Logger::Fields{worker.store.printStorePath(storePath), sub->getUri()}); PushActivity pact(act.id); - copyStorePath(*sub, worker.store, - subPath ? *subPath : storePath, repair, sub->isTrusted ? NoCheckSigs : CheckSigs); + copyStorePath( + *sub, worker.store, fetchPath, repair, sub->isTrusted ? NoCheckSigs : CheckSigs + ); promise.set_value(); + } catch (const EndOfFile &) { + promise.set_exception(std::make_exception_ptr(EndOfFile( + "NAR for '%s' fetched from '%s' is incomplete", + sub->printStorePath(fetchPath), + sub->getUri() + ))); } catch (...) { promise.set_exception(std::current_exception()); } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 244ecf256..7c0902978 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1072,8 +1072,6 @@ void copyStorePath( }); TeeSink tee { sink, progressSink }; srcStore.narFromPath(storePath, tee); - }, [&]() { - throw EndOfFile("NAR for '%s' fetched from '%s' is incomplete", srcStore.printStorePath(storePath), srcStore.getUri()); }); dstStore.addToStore(*info, *source, repair, checkSigs); diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index 3a8a01f16..80b111f08 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -266,20 +266,17 @@ std::unique_ptr sourceToSink(std::function fun) } -std::unique_ptr sinkToSource( - std::function fun, - std::function eof) +std::unique_ptr sinkToSource(std::function fun) { struct SinkToSource : Source { typedef boost::coroutines2::coroutine coro_t; std::function fun; - std::function eof; std::optional coro; - SinkToSource(std::function fun, std::function eof) - : fun(fun), eof(eof) + SinkToSource(std::function fun) + : fun(fun) { } @@ -298,7 +295,9 @@ std::unique_ptr sinkToSource( }); } - if (!*coro) { eof(); abort(); } + if (!*coro) { + throw EndOfFile("coroutine has finished"); + } if (pos == cur.size()) { if (!cur.empty()) { @@ -317,7 +316,7 @@ std::unique_ptr sinkToSource( } }; - return std::make_unique(fun, eof); + return std::make_unique(fun); } diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh index c9294ba2d..0632e3109 100644 --- a/src/libutil/serialise.hh +++ b/src/libutil/serialise.hh @@ -338,11 +338,7 @@ std::unique_ptr sourceToSink(std::function fun); * Convert a function that feeds data into a Sink into a Source. The * Source executes the function as a coroutine. */ -std::unique_ptr sinkToSource( - std::function fun, - std::function eof = []() { - throw EndOfFile("coroutine has finished"); - }); +std::unique_ptr sinkToSource(std::function fun); void writePadding(size_t len, Sink & sink); diff --git a/tests/functional/meson.build b/tests/functional/meson.build index a13dee001..eede1834c 100644 --- a/tests/functional/meson.build +++ b/tests/functional/meson.build @@ -182,6 +182,7 @@ functional_tests_scripts = [ 'debugger.sh', 'test-libstoreconsumer.sh', 'extra-sandbox-profile.sh', + 'substitute-truncated-nar.sh', ] # Plugin tests require shared libraries support. diff --git a/tests/functional/substitute-truncated-nar.sh b/tests/functional/substitute-truncated-nar.sh new file mode 100644 index 000000000..1ac7efaf6 --- /dev/null +++ b/tests/functional/substitute-truncated-nar.sh @@ -0,0 +1,29 @@ +source common.sh + +BINARY_CACHE=file://$cacheDir + +build() { + nix-build --no-out-link "$@" --expr 'derivation { + name = "text"; + system = builtins.currentSystem; + builder = "/bin/sh"; + args = [ "-c" "echo some text to make the nar less empty > $out" ]; + }' +} + +path=$(build) +nix copy --to "$BINARY_CACHE" "$path" +nix-collect-garbage >/dev/null 2>&1 + +nar=0bylmx35yjy2b1b4k7gjsl7i4vc03cpmryb41grfb1mp40n3hifl.nar.xz + +[ -e $cacheDir/nar/$nar ] || fail "long nar missing?" + +xzcat $cacheDir/nar/$nar > $TEST_HOME/tmp +truncate -s $(( $(stat -c %s $TEST_HOME/tmp) - 10 )) $TEST_HOME/tmp +xz < $TEST_HOME/tmp > $cacheDir/nar/$nar + +# Copying back '$path' from the binary cache. This should fail as it is truncated +if build --option substituters "$BINARY_CACHE" --option require-sigs false -j0; then + fail "Importing a truncated nar should fail" +fi