From 5bc8957c7373c25d493473b0f7c86247f3347779 Mon Sep 17 00:00:00 2001
From: Sergei Zimmerman <145775305+xokdvium@users.noreply.github.com>
Date: Wed, 6 Nov 2024 02:21:35 +0300
Subject: [PATCH] fix(libstore-tests): remove use-after-free bug for
 `StringSource`

Unfortunately `StringSource` class is very easy was very easy to misuse
because the ctor took a plain `std::string_view` which has a bad habit
of being implicitly convertible from an rvalue `std::string`. This lead
to unintentional use-after-free bugs.

This patch makes `StringSource` much harder to misuse by disabling the ctor
from a `std::string &&` (but `const std::string &` is ok).

Fix affected tests from libstore-tests.
Reformat those tests with clangd's range formatting since the diff is tiny
and it seems appropriate.
---
 src/libstore-tests/serve-protocol.cc  | 15 ++++-----------
 src/libstore-tests/worker-protocol.cc | 15 ++++-----------
 src/libutil/serialise.hh              |  8 ++++++++
 3 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/src/libstore-tests/serve-protocol.cc b/src/libstore-tests/serve-protocol.cc
index 2505c5a9a..5171fea0f 100644
--- a/src/libstore-tests/serve-protocol.cc
+++ b/src/libstore-tests/serve-protocol.cc
@@ -459,21 +459,14 @@ TEST_F(ServeProtoTest, handshake_client_truncated_replay_throws)
     CharacterizationTest::readTest("handshake-to-client", [&](std::string toClientLog) {
         for (size_t len = 0; len < toClientLog.size(); ++len) {
             NullBufferedSink nullSink;
-            StringSource in {
-                // truncate
-                toClientLog.substr(0, len)
-            };
+            auto substring = toClientLog.substr(0, len);
+            StringSource in{substring};
             if (len < 8) {
                 EXPECT_THROW(
-                    ServeProto::BasicClientConnection::handshake(
-                        nullSink, in, defaultVersion, "blah"),
-                    EndOfFile);
+                    ServeProto::BasicClientConnection::handshake(nullSink, in, defaultVersion, "blah"), EndOfFile);
             } else {
                 // Not sure why cannot keep on checking for `EndOfFile`.
-                EXPECT_THROW(
-                    ServeProto::BasicClientConnection::handshake(
-                        nullSink, in, defaultVersion, "blah"),
-                    Error);
+                EXPECT_THROW(ServeProto::BasicClientConnection::handshake(nullSink, in, defaultVersion, "blah"), Error);
             }
         }
     });
diff --git a/src/libstore-tests/worker-protocol.cc b/src/libstore-tests/worker-protocol.cc
index bbea9ed75..1185c37f4 100644
--- a/src/libstore-tests/worker-protocol.cc
+++ b/src/libstore-tests/worker-protocol.cc
@@ -725,21 +725,14 @@ TEST_F(WorkerProtoTest, handshake_client_truncated_replay_throws)
     CharacterizationTest::readTest("handshake-to-client", [&](std::string toClientLog) {
         for (size_t len = 0; len < toClientLog.size(); ++len) {
             NullBufferedSink nullSink;
-            StringSource in {
-                // truncate
-                toClientLog.substr(0, len)
-            };
+            auto substring = toClientLog.substr(0, len);
+            StringSource in{substring};
             if (len < 8) {
                 EXPECT_THROW(
-                    WorkerProto::BasicClientConnection::handshake(
-                        nullSink, in, defaultVersion, {}),
-                    EndOfFile);
+                    WorkerProto::BasicClientConnection::handshake(nullSink, in, defaultVersion, {}), EndOfFile);
             } else {
                 // Not sure why cannot keep on checking for `EndOfFile`.
-                EXPECT_THROW(
-                    WorkerProto::BasicClientConnection::handshake(
-                        nullSink, in, defaultVersion, {}),
-                    Error);
+                EXPECT_THROW(WorkerProto::BasicClientConnection::handshake(nullSink, in, defaultVersion, {}), Error);
             }
         }
     });
diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh
index d9e34e1e0..14721d069 100644
--- a/src/libutil/serialise.hh
+++ b/src/libutil/serialise.hh
@@ -2,6 +2,7 @@
 ///@file
 
 #include <memory>
+#include <type_traits>
 
 #include "types.hh"
 #include "util.hh"
@@ -202,7 +203,14 @@ struct StringSource : Source
 {
     std::string_view s;
     size_t pos;
+
+    // NOTE: Prevent unintentional dangling views when an implicit conversion
+    // from std::string -> std::string_view occurs when the string is passed
+    // by rvalue.
+    StringSource(std::string &&) = delete;
     StringSource(std::string_view s) : s(s), pos(0) { }
+    StringSource(const std::string& str): StringSource(std::string_view(str)) {}
+
     size_t read(char * data, size_t len) override;
 };