From aec9cd2390fec736ee5673adb30261b8d31a873c Mon Sep 17 00:00:00 2001 From: Erwan Velu Date: Fri, 11 Mar 2016 16:27:34 +0100 Subject: [PATCH] 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 --- src/common/SubProcess.h | 6 +++--- src/test/librbd/fsx.cc | 8 ++++---- src/test/test_subprocess.cc | 32 ++++++++++++++++---------------- 3 files changed, 23 insertions(+), 23 deletions(-) diff --git a/src/common/SubProcess.h b/src/common/SubProcess.h index 82070ac2d538d..6106f7511e24a 100644 --- 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 8375b58db0d19..f0519ec7d62ee 100644 --- 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 a38669ea184ee..29c7b94acd7f4 100644 --- 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'); } -- 2.39.5