From b9b51f9579c15c9789814cd4b6969b0f85e13980 Mon Sep 17 00:00:00 2001
From: Yorick van Pelt <yorick@yorickvanpelt.nl>
Date: Fri, 11 Aug 2023 11:58:33 +0200
Subject: [PATCH 1/4] Prevent overriding virtual methods that are called in a
 destructor

Virtual methods are no longer valid once the derived destructor has
run. This means the compiler is free to optimize them to be
non-virtual.

Found using clang-tidy
---
 src/libmain/progress-bar.cc                 | 3 ++-
 src/libstore/build/local-derivation-goal.hh | 4 +++-
 src/libstore/build/substitution-goal.hh     | 3 ++-
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/libmain/progress-bar.cc b/src/libmain/progress-bar.cc
index 6600ec177..45b1fdfd1 100644
--- a/src/libmain/progress-bar.cc
+++ b/src/libmain/progress-bar.cc
@@ -108,7 +108,8 @@ public:
         stop();
     }
 
-    void stop() override
+    /* Called by destructor, can't be overridden */
+    void stop() override final
     {
         {
             auto state(state_.lock());
diff --git a/src/libstore/build/local-derivation-goal.hh b/src/libstore/build/local-derivation-goal.hh
index 9acd7593d..8827bfca3 100644
--- a/src/libstore/build/local-derivation-goal.hh
+++ b/src/libstore/build/local-derivation-goal.hh
@@ -272,8 +272,10 @@ struct LocalDerivationGoal : public DerivationGoal
 
     /**
      * Forcibly kill the child process, if any.
+     *
+     * Called by destructor, can't be overridden
      */
-    void killChild() override;
+    void killChild() override final;
 
     /**
      * Kill any processes running under the build user UID or in the
diff --git a/src/libstore/build/substitution-goal.hh b/src/libstore/build/substitution-goal.hh
index 9fc041920..1b693baa1 100644
--- a/src/libstore/build/substitution-goal.hh
+++ b/src/libstore/build/substitution-goal.hh
@@ -114,7 +114,8 @@ public:
     void handleChildOutput(int fd, std::string_view data) override;
     void handleEOF(int fd) override;
 
-    void cleanup() override;
+    /* Called by destructor, can't be overridden */
+    void cleanup() override final;
 
     JobCategory jobCategory() override { return JobCategory::Substitution; };
 };

From 1ffb26311b5d74e9f607ec31bee6c17bbae6fdae Mon Sep 17 00:00:00 2001
From: Yorick van Pelt <yorick@yorickvanpelt.nl>
Date: Fri, 11 Aug 2023 12:00:11 +0200
Subject: [PATCH 2/4] MultiCommand::toJSON: Fix use-after-move

---
 src/libutil/args.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/libutil/args.cc b/src/libutil/args.cc
index 3cf3ed9ca..00281ce6f 100644
--- a/src/libutil/args.cc
+++ b/src/libutil/args.cc
@@ -410,8 +410,8 @@ nlohmann::json MultiCommand::toJSON()
         auto cat = nlohmann::json::object();
         cat["id"] = command->category();
         cat["description"] = trim(categories[command->category()]);
-        j["category"] = std::move(cat);
         cat["experimental-feature"] = command->experimentalFeature();
+        j["category"] = std::move(cat);
         cmds[name] = std::move(j);
     }
 

From 2e5096e4f0959cb2974a17fe50ad5ad891b2384f Mon Sep 17 00:00:00 2001
From: Yorick van Pelt <yorick@yorickvanpelt.nl>
Date: Fri, 11 Aug 2023 12:00:31 +0200
Subject: [PATCH 3/4] FileTransfer::download: fix use-after-move

std::move(state->data) and data.empty() were called in a loop, and
could run with no other threads intervening. Accessing moved objects
is undefined behavior, and could cause a crash.
---
 src/libstore/filetransfer.cc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc
index 38b691279..a283af5a2 100644
--- a/src/libstore/filetransfer.cc
+++ b/src/libstore/filetransfer.cc
@@ -863,6 +863,8 @@ void FileTransfer::download(FileTransferRequest && request, Sink & sink)
             }
 
             chunk = std::move(state->data);
+            /* Reset state->data after the move, since we check data.empty() */
+            state->data = "";
 
             state->request.notify_one();
         }

From e78e9a6bd1c5f26684a9da0d94ede7d960788e0e Mon Sep 17 00:00:00 2001
From: Yorick van Pelt <yorick@yorickvanpelt.nl>
Date: Fri, 11 Aug 2023 12:05:45 +0200
Subject: [PATCH 4/4] SimpleLogger::log: fix unintended fallthrough

---
 src/libutil/logging.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/libutil/logging.cc b/src/libutil/logging.cc
index 5a2dd99af..54702e4ea 100644
--- a/src/libutil/logging.cc
+++ b/src/libutil/logging.cc
@@ -67,7 +67,7 @@ public:
             case lvlWarn: c = '4'; break;
             case lvlNotice: case lvlInfo: c = '5'; break;
             case lvlTalkative: case lvlChatty: c = '6'; break;
-            case lvlDebug: case lvlVomit: c = '7';
+            case lvlDebug: case lvlVomit: c = '7'; break;
             default: c = '7'; break; // should not happen, and missing enum case is reported by -Werror=switch-enum
             }
             prefix = std::string("<") + c + ">";