Merge pull request #11813 from xokdvium/dev/fix-use-after-free-libstore-tests

Mic92 · web-flow · commit 2ef5e222df64 · 2024-11-06T06:48:20.000+01:00
fix(libstore-tests): remove use-after-free bug for `StringSource`
diff --git 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
@@ -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
@@ -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;
 };
 
