Improves thread-safe death tests by changing to the original working directory...

Improves thread-safe death tests by changing to the original working directory before they are executed; also fixes out-dated comments about death tests.

include/gtest/gtest-death-test.h

@@ -56,29 +56,19 @@ GTEST_DECLARE_string(death_test_style);
 // Here's what happens when an ASSERT_DEATH* or EXPECT_DEATH* is
 // executed:
 //
-//   1. The assertion fails immediately if there are more than one
-//   active threads.  This is because it's safe to fork() only when
-//   there is a single thread.
+//   1. It generates a warning if there is more than one active
+//   thread.  This is because it's safe to fork() or clone() only
+//   when there is a single thread.
 //
-//   2. The parent process forks a sub-process and runs the death test
-//   in it; the sub-process exits with code 0 at the end of the death
-//   test, if it hasn't exited already.
+//   2. The parent process clone()s a sub-process and runs the death
+//   test in it; the sub-process exits with code 0 at the end of the
+//   death test, if it hasn't exited already.
 //
 //   3. The parent process waits for the sub-process to terminate.
 //
 //   4. The parent process checks the exit code and error message of
 //   the sub-process.
 //
-// Note:
-//
-// It's not safe to call exit() if the current process is forked from
-// a multi-threaded process, so people usually call _exit() instead in
-// such a case.  However, we are not concerned with this as we run
-// death tests only when there is a single thread.  Since exit() has a
-// cleaner semantics (it also calls functions registered with atexit()
-// and on_exit()), this macro calls exit() instead of _exit() to
-// terminate the child process.
-//
 // Examples:
 //
 //   ASSERT_DEATH(server.SendMessage(56, "Hello"), "Invalid port number");
@@ -95,6 +85,20 @@ GTEST_DECLARE_string(death_test_style);
 //   }
 //
 //   ASSERT_EXIT(client.HangUpServer(), KilledBySIGHUP, "Hanging up!");
+//
+// Known caveats:
+//
+//   A "threadsafe" style death test obtains the path to the test
+//   program from argv[0] and re-executes it in the sub-process.  For
+//   simplicity, the current implementation doesn't search the PATH
+//   when launching the sub-process.  This means that the user must
+//   invoke the test program via a path that contains at least one
+//   path separator (e.g. path/to/foo_test and
+//   /absolute/path/to/bar_test are fine, but foo_test is not).  This
+//   is rarely a problem as people usually don't put the test binary
+//   directory in PATH.
+//
+// TODO(wan@google.com): make thread-safe death tests search the PATH.
 
 // Asserts that a given statement causes the program to exit, with an
 // integer exit status that satisfies predicate, and emitting error output

include/gtest/gtest.h

@@ -479,6 +479,10 @@ class UnitTest {
   // INTERNAL IMPLEMENTATION - DO NOT USE IN A USER PROGRAM.
   int Run() GTEST_MUST_USE_RESULT;
 
+  // Returns the working directory when the first TEST() or TEST_F()
+  // was executed.  The UnitTest object owns the string.
+  const char* original_working_dir() const;
+
   // Returns the TestCase object for the test that's currently running,
   // or NULL if no test is running.
   const TestCase* current_test_case() const;

include/gtest/internal/gtest-filepath.h

@@ -70,6 +70,9 @@ class FilePath {
   String ToString() const { return pathname_; }
   const char* c_str() const { return pathname_.c_str(); }
 
+  // Returns the current working directory, or "" if unsuccessful.
+  static FilePath GetCurrentDir();
+
   // Given directory = "dir", base_name = "test", number = 0,
   // extension = "xml", returns "dir/test.xml". If number is greater
   // than zero (e.g., 12), returns "dir/test_12.xml".
@@ -91,6 +94,9 @@ class FilePath {
                                          const FilePath& base_name,
                                          const char* extension);
 
+  // Returns true iff the path is NULL or "".
+  bool IsEmpty() const { return c_str() == NULL || *c_str() == '\0'; }
+
   // If input name has a trailing separator character, removes it and returns
   // the name, otherwise return the name string unmodified.
   // On Windows platform, uses \ as the separator, other platforms use /.

src/gtest-death-test.cc

@@ -546,11 +546,32 @@ struct ExecDeathTestArgs {
 };
 
 // The main function for a threadsafe-style death test child process.
+// This function is called in a clone()-ed process and thus must avoid
+// any potentially unsafe operations like malloc or libc functions.
 static int ExecDeathTestChildMain(void* child_arg) {
   ExecDeathTestArgs* const args = static_cast<ExecDeathTestArgs*>(child_arg);
   GTEST_DEATH_TEST_CHECK_SYSCALL(close(args->close_fd));
+
+  // We need to execute the test program in the same environment where
+  // it was originally invoked.  Therefore we change to the original
+  // working directory first.
+  const char* const original_dir =
+      UnitTest::GetInstance()->original_working_dir();
+  // We can safely call chdir() as it's a direct system call.
+  if (chdir(original_dir) != 0) {
+    DeathTestAbort("chdir(\"%s\") failed: %s",
+                   original_dir, strerror(errno));
+    return EXIT_FAILURE;
+  }
+
+  // We can safely call execve() as it's a direct system call.  We
+  // cannot use execvp() as it's a libc function and thus potentially
+  // unsafe.  Since execve() doesn't search the PATH, the user must
+  // invoke the test program via a valid path that contains at least
+  // one path separator.
   execve(args->argv[0], args->argv, environ);
-  DeathTestAbort("execve failed: %s", strerror(errno));
+  DeathTestAbort("execve(%s, ...) in %s failed: %s",
+                 args->argv[0], original_dir, strerror(errno));
   return EXIT_FAILURE;
 }
 

src/gtest-filepath.cc

@@ -32,6 +32,8 @@
 #include <gtest/internal/gtest-filepath.h>
 #include <gtest/internal/gtest-port.h>
 
+#include <stdlib.h>
+
 #ifdef _WIN32_WCE
 #include <windows.h>
 #elif defined(_WIN32)
@@ -40,6 +42,7 @@
 #include <sys/stat.h>
 #else
 #include <sys/stat.h>
+#include <unistd.h>
 #endif // _WIN32_WCE or _WIN32
 
 #include <gtest/internal/gtest-string.h>
@@ -66,6 +69,21 @@ const char kPathSeparatorString[] = "/";
 const char kCurrentDirectoryString[] = "./";
 #endif  // GTEST_OS_WINDOWS
 
+// Returns the current working directory, or "" if unsuccessful.
+FilePath FilePath::GetCurrentDir() {
+#ifdef _WIN32_WCE
+// Windows CE doesn't have a current directory, so we just return
+// something reasonable.
+  return FilePath(kCurrentDirectoryString);
+#elif defined(GTEST_OS_WINDOWS)
+  char cwd[_MAX_PATH + 1] = {};
+  return FilePath(_getcwd(cwd, sizeof(cwd)) == NULL ? "" : cwd);
+#else
+  char cwd[PATH_MAX + 1] = {};
+  return FilePath(getcwd(cwd, sizeof(cwd)) == NULL ? "" : cwd);
+#endif
+}
+
 // Returns a copy of the FilePath with the case-insensitive extension removed.
 // Example: FilePath("dir/file.exe").RemoveExtension("EXE") returns
 // FilePath("dir/file"). If a case-insensitive extension is not

src/gtest-internal-inl.h

@@ -1003,6 +1003,21 @@ class UnitTestImpl : public TestPartResultReporterInterface {
   void AddTestInfo(Test::SetUpTestCaseFunc set_up_tc,
                    Test::TearDownTestCaseFunc tear_down_tc,
                    TestInfo * test_info) {
+    // In order to support thread-safe death tests, we need to
+    // remember the original working directory when the test program
+    // was first invoked.  We cannot do this in RUN_ALL_TESTS(), as
+    // the user may have changed the current directory before calling
+    // RUN_ALL_TESTS().  Therefore we capture the current directory in
+    // AddTestInfo(), which is called to register a TEST or TEST_F
+    // before main() is reached.
+    if (original_working_dir_.IsEmpty()) {
+      original_working_dir_.Set(FilePath::GetCurrentDir());
+      if (original_working_dir_.IsEmpty()) {
+        printf("%s\n", "Failed to get the current working directory.");
+        abort();
+      }
+    }
+
     GetTestCase(test_info->test_case_name(),
                 test_info->test_case_comment(),
                 set_up_tc,
@@ -1083,9 +1098,15 @@ class UnitTestImpl : public TestPartResultReporterInterface {
 #endif  // GTEST_HAS_DEATH_TEST
 
  private:
+  friend class ::testing::UnitTest;
+
   // The UnitTest object that owns this implementation object.
   UnitTest* const parent_;
 
+  // The working directory when the first TEST() or TEST_F() was
+  // executed.
+  internal::FilePath original_working_dir_;
+
   // Points to (but doesn't own) the test part result reporter.
   TestPartResultReporterInterface* test_part_result_reporter_;
 

src/gtest.cc

@@ -3211,6 +3211,12 @@ int UnitTest::Run() {
 #endif  // GTEST_OS_WINDOWS
 }
 
+// Returns the working directory when the first TEST() or TEST_F() was
+// executed.
+const char* UnitTest::original_working_dir() const {
+  return impl_->original_working_dir_.c_str();
+}
+
 // Returns the TestCase object for the test that's currently running,
 // or NULL if no test is running.
 // L < mutex_

test/gtest-death-test_test.cc

@@ -36,6 +36,8 @@
 
 #ifdef GTEST_HAS_DEATH_TEST
 
+#include <stdio.h>
+#include <unistd.h>
 #include <gtest/gtest-spi.h>
 
 // Indicates that this translation unit is part of Google Test's
@@ -85,13 +87,19 @@ class TestForDeathTest : public testing::Test {
  protected:
   // A static member function that's expected to die.
   static void StaticMemberFunction() {
-    GTEST_LOG(FATAL, "death inside StaticMemberFunction().");
+    fprintf(stderr, "%s", "death inside StaticMemberFunction().");
+    // We call _exit() instead of exit(), as the former is a direct
+    // system call and thus safer in the presence of threads.  exit()
+    // will invoke user-defined exit-hooks, which may do dangerous
+    // things that conflict with death tests.
+    _exit(1);
   }
 
   // A method of the test fixture that may die.
   void MemberFunction() {
     if (should_die_) {
-      GTEST_LOG(FATAL, "death inside MemberFunction().");
+      fprintf(stderr, "%s", "death inside MemberFunction().");
+      _exit(1);
     }
   }
 
@@ -157,13 +165,13 @@ int DieInDebugElse12(int* sideeffect) {
   return 12;
 }
 
-// Returns the exit status of a process that calls exit(2) with a
+// Returns the exit status of a process that calls _exit(2) with a
 // given exit code.  This is a helper function for the
 // ExitStatusPredicateTest test suite.
 static int NormalExitStatus(int exit_code) {
   pid_t child_pid = fork();
   if (child_pid == 0) {
-    exit(exit_code);
+    _exit(exit_code);
   }
   int status;
   waitpid(child_pid, &status, 0);
@@ -179,7 +187,7 @@ static int KilledExitStatus(int signum) {
   pid_t child_pid = fork();
   if (child_pid == 0) {
     raise(signum);
-    exit(1);
+    _exit(1);
   }
   int status;
   waitpid(child_pid, &status, 0);
@@ -223,7 +231,7 @@ TEST_F(TestForDeathTest, SingleStatement) {
     ASSERT_DEATH(return, "");
 
   if (true)
-    EXPECT_DEATH(exit(1), "");
+    EXPECT_DEATH(_exit(1), "");
   else
     // This empty "else" branch is meant to ensure that EXPECT_DEATH
     // doesn't expand into an "if" statement without an "else"
@@ -235,12 +243,12 @@ TEST_F(TestForDeathTest, SingleStatement) {
   if (false)
     ;
   else
-    EXPECT_DEATH(exit(1), "") << 1 << 2 << 3;
+    EXPECT_DEATH(_exit(1), "") << 1 << 2 << 3;
 }
 
 void DieWithEmbeddedNul() {
   fprintf(stderr, "Hello%cworld.\n", '\0');
-  abort();
+  _exit(1);
 }
 
 // Tests that EXPECT_DEATH and ASSERT_DEATH work when the error
@@ -257,24 +265,40 @@ TEST_F(TestForDeathTest, DISABLED_EmbeddedNulInMessage) {
 TEST_F(TestForDeathTest, SwitchStatement) {
   switch (0)
     default:
-      ASSERT_DEATH(exit(1), "") << "exit in default switch handler";
+      ASSERT_DEATH(_exit(1), "") << "exit in default switch handler";
 
   switch (0)
     case 0:
-      EXPECT_DEATH(exit(1), "") << "exit in switch case";
+      EXPECT_DEATH(_exit(1), "") << "exit in switch case";
 }
 
-// Tests that a static member function can be used in a death test.
-TEST_F(TestForDeathTest, StaticMemberFunction) {
+// Tests that a static member function can be used in a "fast" style
+// death test.
+TEST_F(TestForDeathTest, StaticMemberFunctionFastStyle) {
+  testing::GTEST_FLAG(death_test_style) = "fast";
   ASSERT_DEATH(StaticMemberFunction(), "death.*StaticMember");
 }
 
-// Tests that a method of the test fixture can be used in a death test.
-TEST_F(TestForDeathTest, MemberFunction) {
+// Tests that a method of the test fixture can be used in a "fast"
+// style death test.
+TEST_F(TestForDeathTest, MemberFunctionFastStyle) {
+  testing::GTEST_FLAG(death_test_style) = "fast";
   should_die_ = true;
   EXPECT_DEATH(MemberFunction(), "inside.*MemberFunction");
 }
 
+// Tests that death tests work even if the current directory has been
+// changed.
+TEST_F(TestForDeathTest, FastDeathTestInChangedDir) {
+  testing::GTEST_FLAG(death_test_style) = "fast";
+
+  chdir("/");
+  EXPECT_EXIT(_exit(1), testing::ExitedWithCode(1), "");
+
+  chdir("/");
+  ASSERT_DEATH(_exit(1), "");
+}
+
 // Repeats a representative sample of death tests in the "threadsafe" style:
 
 TEST_F(TestForDeathTest, StaticMemberFunctionThreadsafeStyle) {
@@ -292,14 +316,24 @@ TEST_F(TestForDeathTest, ThreadsafeDeathTestInLoop) {
   testing::GTEST_FLAG(death_test_style) = "threadsafe";
 
   for (int i = 0; i < 3; ++i)
-    EXPECT_EXIT(exit(i), testing::ExitedWithCode(i), "") << ": i = " << i;
+    EXPECT_EXIT(_exit(i), testing::ExitedWithCode(i), "") << ": i = " << i;
+}
+
+TEST_F(TestForDeathTest, ThreadsafeDeathTestInChangedDir) {
+  testing::GTEST_FLAG(death_test_style) = "threadsafe";
+
+  chdir("/");
+  EXPECT_EXIT(_exit(1), testing::ExitedWithCode(1), "");
+
+  chdir("/");
+  ASSERT_DEATH(_exit(1), "");
 }
 
 TEST_F(TestForDeathTest, MixedStyles) {
   testing::GTEST_FLAG(death_test_style) = "threadsafe";
-  EXPECT_DEATH(exit(1), "");
+  EXPECT_DEATH(_exit(1), "");
   testing::GTEST_FLAG(death_test_style) = "fast";
-  EXPECT_DEATH(exit(1), "");
+  EXPECT_DEATH(_exit(1), "");
 }
 
 namespace {
@@ -316,7 +350,7 @@ TEST_F(TestForDeathTest, DoesNotExecuteAtforkHooks) {
   testing::GTEST_FLAG(death_test_style) = "threadsafe";
   pthread_flag = false;
   ASSERT_EQ(0, pthread_atfork(&SetPthreadFlag, NULL, NULL));
-  ASSERT_DEATH(exit(1), "");
+  ASSERT_DEATH(_exit(1), "");
   ASSERT_FALSE(pthread_flag);
 }
 
@@ -528,8 +562,8 @@ TEST_F(TestForDeathTest, AssertDebugDeathAborts) {
 
 // Tests the *_EXIT family of macros, using a variety of predicates.
 TEST_F(TestForDeathTest, ExitMacros) {
-  EXPECT_EXIT(exit(1),  testing::ExitedWithCode(1),  "");
-  ASSERT_EXIT(exit(42), testing::ExitedWithCode(42), "");
+  EXPECT_EXIT(_exit(1),  testing::ExitedWithCode(1),  "");
+  ASSERT_EXIT(_exit(42), testing::ExitedWithCode(42), "");
   EXPECT_EXIT(raise(SIGKILL), testing::KilledBySignal(SIGKILL), "") << "foo";
   ASSERT_EXIT(raise(SIGUSR2), testing::KilledBySignal(SIGUSR2), "") << "bar";
 
@@ -539,7 +573,7 @@ TEST_F(TestForDeathTest, ExitMacros) {
   }, "This failure is expected.");
 
   EXPECT_FATAL_FAILURE({  // NOLINT
-    ASSERT_EXIT(exit(0), testing::KilledBySignal(SIGSEGV), "")
+    ASSERT_EXIT(_exit(0), testing::KilledBySignal(SIGSEGV), "")
         << "This failure is expected, too.";
   }, "This failure is expected, too.");
 }
@@ -547,7 +581,7 @@ TEST_F(TestForDeathTest, ExitMacros) {
 TEST_F(TestForDeathTest, InvalidStyle) {
   testing::GTEST_FLAG(death_test_style) = "rococo";
   EXPECT_NONFATAL_FAILURE({  // NOLINT
-    EXPECT_DEATH(exit(0), "") << "This failure is expected.";
+    EXPECT_DEATH(_exit(0), "") << "This failure is expected.";
   }, "This failure is expected.");
 }
 
@@ -794,7 +828,7 @@ TEST_F(MacroLogicDeathTest, ChildDoesNotDie) {
   // This time there are two calls to Abort: one since the test didn't
   // die, and another from the ReturnSentinel when it's destroyed.  The
   // sentinel normally isn't destroyed if a test doesn't die, since
-  // exit(2) is called in that case by ForkingDeathTest, but not by
+  // _exit(2) is called in that case by ForkingDeathTest, but not by
   // our MockDeathTest.
   ASSERT_EQ(2, factory_->AbortCalls());
   EXPECT_EQ(DeathTest::TEST_DID_NOT_DIE,
@@ -813,18 +847,18 @@ static size_t GetSuccessfulTestPartCount() {
 // Tests that a successful death test does not register a successful
 // test part.
 TEST(SuccessRegistrationDeathTest, NoSuccessPart) {
-  EXPECT_DEATH(exit(1), "");
+  EXPECT_DEATH(_exit(1), "");
   EXPECT_EQ(0u, GetSuccessfulTestPartCount());
 }
 
 TEST(StreamingAssertionsDeathTest, DeathTest) {
-  EXPECT_DEATH(exit(1), "") << "unexpected failure";
-  ASSERT_DEATH(exit(1), "") << "unexpected failure";
+  EXPECT_DEATH(_exit(1), "") << "unexpected failure";
+  ASSERT_DEATH(_exit(1), "") << "unexpected failure";
   EXPECT_NONFATAL_FAILURE({  // NOLINT
-    EXPECT_DEATH(exit(0), "") << "expected failure";
+    EXPECT_DEATH(_exit(0), "") << "expected failure";
   }, "expected failure");
   EXPECT_FATAL_FAILURE({  // NOLINT
-    ASSERT_DEATH(exit(0), "") << "expected failure";
+    ASSERT_DEATH(_exit(0), "") << "expected failure";
   }, "expected failure");
 }
 

test/gtest-filepath_test.cc

@@ -91,6 +91,38 @@ const char* MakeTempDir() {
 }
 #endif  // _WIN32_WCE
 
+#ifndef _WIN32_WCE
+
+TEST(GetCurrentDirTest, ReturnsCurrentDir) {
+  EXPECT_FALSE(FilePath::GetCurrentDir().IsEmpty());
+
+#ifdef GTEST_OS_WINDOWS
+  _chdir(PATH_SEP);
+  const FilePath cwd = FilePath::GetCurrentDir();
+  // Skips the ":".
+  const char* const cwd_without_drive = strchr(cwd.c_str(), ':');
+  ASSERT_TRUE(cwd_without_drive != NULL);
+  EXPECT_STREQ(PATH_SEP, cwd_without_drive + 1);
+#else
+  chdir(PATH_SEP);
+  EXPECT_STREQ(PATH_SEP, FilePath::GetCurrentDir().c_str());
+#endif
+}
+
+#endif  // _WIN32_WCE
+
+TEST(IsEmptyTest, ReturnsTrueForEmptyPath) {
+  EXPECT_TRUE(FilePath("").IsEmpty());
+  EXPECT_TRUE(FilePath(NULL).IsEmpty());
+}
+
+TEST(IsEmptyTest, ReturnsFalseForNonEmptyPath) {
+  EXPECT_FALSE(FilePath("a").IsEmpty());
+  EXPECT_FALSE(FilePath(".").IsEmpty());
+  EXPECT_FALSE(FilePath("a/b").IsEmpty());
+  EXPECT_FALSE(FilePath("a\\b\\").IsEmpty());
+}
+
 // FilePath's functions used by UnitTestOptions::GetOutputFile.
 
 // RemoveDirectoryName "" -> ""

test/gtest_unittest.cc

@@ -1142,7 +1142,8 @@ TEST(ParseInt32FlagTest, ParsesAndReturnsValidValue) {
 }
 
 // For the same reason we are not explicitly testing everything in the
-// Test class, there are no separate tests for the following classes:
+// Test class, there are no separate tests for the following classes
+// (except for some trivial cases):
 //
 //   TestCase, UnitTest, UnitTestResultPrinter.
 //
@@ -1150,6 +1151,11 @@ TEST(ParseInt32FlagTest, ParsesAndReturnsValidValue) {
 //
 //   TEST, TEST_F, RUN_ALL_TESTS
 
+TEST(UnitTestTest, CanGetOriginalWorkingDir) {
+  ASSERT_TRUE(UnitTest::GetInstance()->original_working_dir() != NULL);
+  EXPECT_STRNE(UnitTest::GetInstance()->original_working_dir(), "");
+}
+
 // This group of tests is for predicate assertions (ASSERT_PRED*, etc)
 // of various arities.  They do not attempt to be exhaustive.  Rather,
 // view them as smoke tests that can be easily reviewed and verified.