SubProcess: Avoid buffer corruption when calling err()

Some code like crush/CrushTester.cc, uses err() to get the output status of a
spawned command.

As per reported in bug #15011, some tests were failing because of some utf8
decoding issues. The implied buffers were generated by the output of err()
function of SubProcess.

err() is returning the str().c_str() value of 'errstr' object which is not
living much longer that the function itself.

This patch returns a std::string to avoid the temporary allocation and fixes
the associated err() calls accordingly.

Since this commit, the make check is now passing everytime.

Fixes: #15011
Signed-off-by: Erwan Velu <erwan@redhat.com>

diff --git a/src/common/SubProcess.h b/src/common/SubProcess.h
index 82070ac2d538ddb03cb594a34fe61d8ec646e878..6106f7511e24a512e5a5d41544435e4ecb7920ca 100644 (file)
--- a/src/common/SubProcess.h
+++ b/src/common/SubProcess.h
@@ -87,7 +87,7 @@ public:
 
   void kill(int signo = SIGTERM) const;
 
-  const char* err() const;
+  const std::string err() const;
 
 protected:
   bool is_child() const { return pid == 0; }
@@ -219,8 +219,8 @@ inline void SubProcess::kill(int signo) const {
   assert(ret == 0);
 }
 
-inline const char* SubProcess::err() const {
-  return errstr.str().c_str();
+inline const std::string SubProcess::err() const {
+  return errstr.str();
 }
 
 class fd_buf : public std::streambuf {

diff --git a/src/test/librbd/fsx.cc b/src/test/librbd/fsx.cc
index 8375b58db0d19952982a4451acb0376018ca584d..f0519ec7d62ee1031306ccc2552be864e580439d 100644 (file)
--- a/src/test/librbd/fsx.cc
+++ b/src/test/librbd/fsx.cc
@@ -1045,7 +1045,7 @@ nbd_open(const char *name, struct rbd_ctx *ctx)
 
         r = process.spawn();
         if (r < 0) {
-               prt("nbd_open failed to run rbd-nbd error: %s\n", process.err());
+               prt("nbd_open failed to run rbd-nbd error: %s\n", process.err().c_str());
                return r;
         }
        r = safe_read(process.get_stdout(), dev, sizeof(dev));
@@ -1059,7 +1059,7 @@ nbd_open(const char *name, struct rbd_ctx *ctx)
        dev[r] = 0;
        r = process.join();
        if (r) {
-               prt("rbd-nbd failed with error: %s", process.err());
+               prt("rbd-nbd failed with error: %s", process.err().c_str());
                return -EINVAL;
        }
 
@@ -1099,12 +1099,12 @@ nbd_close(struct rbd_ctx *ctx)
 
         r = process.spawn();
         if (r < 0) {
-               prt("nbd_close failed to run rbd-nbd error: %s\n", process.err());
+               prt("nbd_close failed to run rbd-nbd error: %s\n", process.err().c_str());
                return r;
         }
        r = process.join();
        if (r) {
-               prt("rbd-nbd failed with error: %d", process.err());
+               prt("rbd-nbd failed with error: %d", process.err().c_str());
                return -EINVAL;
        }
 

diff --git a/src/test/test_subprocess.cc b/src/test/test_subprocess.cc
index a38669ea184ee5c66b6535268c7ff9e435967f21..29c7b94acd7f4f521e9f29d883da698f08124117 100644 (file)
--- a/src/test/test_subprocess.cc
+++ b/src/test/test_subprocess.cc
@@ -38,7 +38,7 @@ TEST(SubProcess, True)
   SubProcess p("true");
   ASSERT_EQ(p.spawn(), 0);
   ASSERT_EQ(p.join(), 0);
-  ASSERT_TRUE(p.err()[0] == '\0');
+  ASSERT_TRUE(p.err().c_str()[0] == '\0');
 }
 
 TEST(SubProcess, False)
@@ -46,7 +46,7 @@ TEST(SubProcess, False)
   SubProcess p("false");
   ASSERT_EQ(p.spawn(), 0);
   ASSERT_EQ(p.join(), 1);
-  ASSERT_FALSE(p.err()[0] == '\0');
+  ASSERT_FALSE(p.err().c_str()[0] == '\0');
 }
 
 TEST(SubProcess, NotFound)
@@ -58,7 +58,7 @@ TEST(SubProcess, NotFound)
   std::cerr << "stderr: " << buf;
   ASSERT_EQ(p.join(), 1);
   std::cerr << "err: " << p.err() << std::endl;
-  ASSERT_FALSE(p.err()[0] == '\0');
+  ASSERT_FALSE(p.err().c_str()[0] == '\0');
 }
 
 TEST(SubProcess, Echo)
@@ -72,7 +72,7 @@ TEST(SubProcess, Echo)
   std::cerr << "stdout: " << buf;
   ASSERT_EQ(buf, "1 2 3\n");
   ASSERT_EQ(echo.join(), 0);
-  ASSERT_TRUE(echo.err()[0] == '\0');
+  ASSERT_TRUE(echo.err().c_str()[0] == '\0');
 }
 
 TEST(SubProcess, Cat)
@@ -91,7 +91,7 @@ TEST(SubProcess, Cat)
   ASSERT_TRUE(read_from_fd(cat.get_stderr(), buf));
   ASSERT_EQ(buf, "");
   ASSERT_EQ(cat.join(), 0);
-  ASSERT_TRUE(cat.err()[0] == '\0');
+  ASSERT_TRUE(cat.err().c_str()[0] == '\0');
 }
 
 TEST(SubProcess, CatDevNull)
@@ -106,7 +106,7 @@ TEST(SubProcess, CatDevNull)
   ASSERT_TRUE(read_from_fd(cat.get_stderr(), buf));
   ASSERT_EQ(buf, "");
   ASSERT_EQ(cat.join(), 0);
-  ASSERT_TRUE(cat.err()[0] == '\0');
+  ASSERT_TRUE(cat.err().c_str()[0] == '\0');
 }
 
 TEST(SubProcess, Killed)
@@ -117,7 +117,7 @@ TEST(SubProcess, Killed)
   cat.kill();
   ASSERT_EQ(cat.join(), 128 + SIGTERM);
   std::cerr << "err: " << cat.err() << std::endl;
-  ASSERT_FALSE(cat.err()[0] == '\0');
+  ASSERT_FALSE(cat.err().c_str()[0] == '\0');
 }
 
 TEST(SubProcess, CatWithArgs)
@@ -139,7 +139,7 @@ TEST(SubProcess, CatWithArgs)
   ASSERT_FALSE(buf.empty());
   ASSERT_EQ(cat.join(), 1);
   std::cerr << "err: " << cat.err() << std::endl;
-  ASSERT_FALSE(cat.err()[0] == '\0');
+  ASSERT_FALSE(cat.err().c_str()[0] == '\0');
 }
 
 TEST(SubProcess, Subshell)
@@ -164,7 +164,7 @@ TEST(SubProcess, Subshell)
   ASSERT_EQ(buf, "error from subshell\n");
   ASSERT_EQ(sh.join(), 13);
   std::cerr << "err: " << sh.err() << std::endl;
-  ASSERT_FALSE(sh.err()[0] == '\0');
+  ASSERT_FALSE(sh.err().c_str()[0] == '\0');
 }
 
 TEST(SubProcessTimed, True)
@@ -172,7 +172,7 @@ TEST(SubProcessTimed, True)
   SubProcessTimed p("true", SubProcess::CLOSE, SubProcess::CLOSE, SubProcess::CLOSE, 10);
   ASSERT_EQ(p.spawn(), 0);
   ASSERT_EQ(p.join(), 0);
-  ASSERT_TRUE(p.err()[0] == '\0');
+  ASSERT_TRUE(p.err().c_str()[0] == '\0');
 }
 
 TEST(SubProcessTimed, SleepNoTimeout)
@@ -182,7 +182,7 @@ TEST(SubProcessTimed, SleepNoTimeout)
 
   ASSERT_EQ(sleep.spawn(), 0);
   ASSERT_EQ(sleep.join(), 0);
-  ASSERT_TRUE(sleep.err()[0] == '\0');
+  ASSERT_TRUE(sleep.err().c_str()[0] == '\0');
 }
 
 TEST(SubProcessTimed, Killed)
@@ -198,7 +198,7 @@ TEST(SubProcessTimed, Killed)
   ASSERT_TRUE(buf.empty());
   ASSERT_EQ(cat.join(), 128 + SIGTERM);
   std::cerr << "err: " << cat.err() << std::endl;
-  ASSERT_FALSE(cat.err()[0] == '\0');
+  ASSERT_FALSE(cat.err().c_str()[0] == '\0');
 }
 
 TEST(SubProcessTimed, SleepTimedout)
@@ -213,7 +213,7 @@ TEST(SubProcessTimed, SleepTimedout)
   ASSERT_FALSE(buf.empty());
   ASSERT_EQ(sleep.join(), 128 + SIGKILL);
   std::cerr << "err: " << sleep.err() << std::endl;
-  ASSERT_FALSE(sleep.err()[0] == '\0');
+  ASSERT_FALSE(sleep.err().c_str()[0] == '\0');
 }
 
 TEST(SubProcessTimed, SubshellNoTimeout)
@@ -233,7 +233,7 @@ TEST(SubProcessTimed, SubshellNoTimeout)
   std::cerr << "stderr: " << buf << std::endl;
   ASSERT_EQ(buf, msg);
   ASSERT_EQ(sh.join(), 0);
-  ASSERT_TRUE(sh.err()[0] == '\0');
+  ASSERT_TRUE(sh.err().c_str()[0] == '\0');
 }
 
 TEST(SubProcessTimed, SubshellKilled)
@@ -250,7 +250,7 @@ TEST(SubProcessTimed, SubshellKilled)
   ASSERT_TRUE(buf.empty());
   ASSERT_EQ(sh.join(), 128 + SIGTERM);
   std::cerr << "err: " << sh.err() << std::endl;
-  ASSERT_FALSE(sh.err()[0] == '\0');
+  ASSERT_FALSE(sh.err().c_str()[0] == '\0');
 }
 
 TEST(SubProcessTimed, SubshellTimedout)
@@ -264,5 +264,5 @@ TEST(SubProcessTimed, SubshellTimedout)
   ASSERT_FALSE(buf.empty());
   ASSERT_EQ(sh.join(), 128 + SIGTERM);
   std::cerr << "err: " << sh.err() << std::endl;
-  ASSERT_FALSE(sh.err()[0] == '\0');
+  ASSERT_FALSE(sh.err().c_str()[0] == '\0');
 }