Merge lp:~michihenning/unity-api/daemon-strong-exception-guarantee into lp:unity-api

[Michi Henning]

Made daemonize_me() more robust when called at the file descriptor limit and
when memory is low. Provided strong exception guarantee for a few more corner
cases. Fixed coverage test for this to not report nonsense. (Previous version
had race conditions; depending on timing, coverage report was either OK or
reported covered things as uncovered.) Improved test to make sure SIGHUP is
correctly restored. Fixed build errors in release mode due to unused return values.

Made daemonize_me() more robust when called at the file descriptor limit and
when memory is low. Provided strong exception guarantee for a few more corner
cases. Fixed coverage test for this to not report nonsense. (Previous version
had race conditions; depending on timing, coverage report was either OK or
reported covered things as uncovered.) Improved test to make sure SIGHUP is
correctly restored. Fixed build errors in release mode due to unused return values.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:44
http://jenkins.qa.ubuntu.com/job/unity-api-ci/34/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-raring-armhf-ci/34
        deb: http://jenkins.qa.ubuntu.com/job/unity-api-raring-armhf-ci/34/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-raring-i386-ci/34

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-api-ci/34/rebuild

review: Approve (continuous-integration)

[lp:~michihenning/unity-api/daemon-strong-exception-guarantee]

45. By Michi Henning on 2013-05-20

Made daemonize_me() more robust when called at the file descriptor limit and
when memory is low. Provided strong exception guarantee for a few more corner
cases. Fixed coverage test for this to not report nonsense. (Previous version
had race conditions; depending on timing, coverage report was either OK or
reported covered things as uncovered.) Improved test to make sure SIGHUP is
correctly restored. Fixed build errors in release mode due to unused return values.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:45
http://jenkins.qa.ubuntu.com/job/unity-api-ci/35/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-raring-armhf-ci/35
        deb: http://jenkins.qa.ubuntu.com/job/unity-api-raring-armhf-ci/35/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-raring-i386-ci/35

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-api-ci/35/rebuild

review: Approve (continuous-integration)

[Jussi Pakkanen (jpakkane)]

If we really want to be thorough, then the file description closing function needs some work. If the system runs out of memory and throws a bad_alloc, then the cleanup procedure should not do any more allocations. This one does:

- call to close (unavoidable, really)
- creation of temporary for swap

If bad_alloc is thrown when the fd vector is empty, the cleanup part releases no memory and thus goes into an eternal loop (probably).

The only time Linux memory allocation ever fails is when trying to alloc more memory in a single allocation than the computer has (or something like that). In this case when we have a few billion elements in the fd vector. The question then becomes is this an issue that we ever need to care about?

Also, in a test there is this:

 int rc __attribute__((unused)) = system("rm -fr empty; >empty");

Shell code in programs is of the devil. Libc should be used instead. Especially since the return value is ignored.

review: Needs Fixing

[lp:~michihenning/unity-api/daemon-strong-exception-guarantee]

46. By Michi Henning on 2013-05-20

Replaced use of system() to create test files and directories with libc file I/O calls.

47. By Michi Henning on 2013-05-20

Removed recovery if vector throws bad_alloc. If the process is sailing this close to the wind,
it'll probably die in the next millisecond or so anyway, even if we recover here. The added
complexity isn't worth it.

[Michi Henning (michihenning)]

> If we really want to be thorough, then the file description closing function
> needs some work. If the system runs out of memory and throws a bad_alloc, then
> the cleanup procedure should not do any more allocations. This one does:
>
> - call to close (unavoidable, really)
> - creation of temporary for swap

The call to close does not allocate memory, so that's not a problem. The temporary vector is on the stack. If that allocation fails because the stack is full, we won't get bad_alloc. Instead, the kernel will kill the process. The default constructor of vector does not allocate memory either. The initial capacity is zero. (This is not guaranteed by the standard, but I have never seen an implementation that behaves otherwise. Doing otherwise would be monumentally stupid because it would penalize the default construtor with an (expensive) heap allocation.) So, as a recovery strategy, the swap idiom is safe and will work.

> If bad_alloc is thrown when the fd vector is empty, the cleanup part releases
> no memory and thus goes into an eternal loop (probably).

Good catch! This one could be easily fixed by counting the number of descriptors that were closed so far, and re-throwing if we haven't made progress since the last time we tried.

> The only time Linux memory allocation ever fails is when trying to alloc more
> memory in a single allocation than the computer has (or something like that).

It's determined by the virtual address space size: setrlimit(RLIMIT_AS, ...), which affects brk() (which is what doles out memory for malloc). That limit can be quite bit lower than the default.

> In this case when we have a few billion elements in the fd vector.

There could be far fewer, such as only a few a hundred.

> The question then becomes is this an issue that we ever need to care about?

That's the real question. If the process is sailing so close to the wind that it can't store a list of its file descriptors anymore, it'll probably go belly-up within the next few milliseconds anyway, even if we recover at this point.

So, there are two ways to fix this. I can add the test for making progress, which will prevent an infinite loop, or I can simply delete the recovery attempt. Pragmatically, I've done the latter.

> Also, in a test there is this:
>
> int rc __attribute__((unused)) = system("rm -fr empty; >empty");
>
> Shell code in programs is of the devil. Libc should be used instead.
> Especially since the return value is ignored.

Not sure why that's the devil. There is only one thing that command can do, which is what's needed for the test. The unused return value issue comes about when compiling in release mode where we use -Werror.

It's interesting that there is no warning for this when compiling in debug mode. Why is this? I would expect the warning in both release and debug mode, seeing that -Werror just says "stop on warnings", but it shouldn't add warnings of its own.

I've replaced the calls to system() regardless. In general though, there are times when using a shell script is easier than any other option if all I need is to set up a few files for testing. Why disallow this? The situation would be no better if I were to mov...

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:47
http://jenkins.qa.ubuntu.com/job/unity-api-ci/36/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-raring-armhf-ci/36
        deb: http://jenkins.qa.ubuntu.com/job/unity-api-raring-armhf-ci/36/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-raring-i386-ci/36

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-api-ci/36/rebuild

review: Approve (continuous-integration)

[Jussi Pakkanen (jpakkane)]

Using embedded shell code to do basic file manipulation is basically the same thing as, though slightly less evil than, this:

http://thedailywtf.com/Articles/The-Int-Divide.aspx

Keeping shell scripts in separate files is better in that it gives the functionality name such as setup_test_files.sh and makes it visible in the source tree. Shellcode is also not portable as it won't work on non-Unix platforms (which is probably not a major issue in this particular piece of code).

review: Approve

[Michi Henning (michihenning)]

> Using embedded shell code to do basic file manipulation is basically the same
> thing as, though slightly less evil than, this:
>
> http://thedailywtf.com/Articles/The-Int-Divide.aspx

I did enjoy reading this anecdote :-) I'm not sure it's a fair comparison though.

> Keeping shell scripts in separate files is better in that it gives the
> functionality name such as setup_test_files.sh and makes it visible in the
> source tree. Shellcode is also not portable as it won't work on non-Unix
> platforms (which is probably not a major issue in this particular piece of
> code)

The shell restriction really isn't an issue for us. I don't think we'll be porting stuff to Windows any time soon.

I like the idea of separate setup scripts, sure. But for many tests, I have to do little things with files and directories. Create a file with a certain content, and second file with some matching content, make a small directory hierarchy, run a test, and tear them down again.

The shell is really good at doing this sort of thing, whereas using C++ streams, libc, or whatever typically sucks in comparison. When you look at the diff for the FileIO test, the embedded system() call was simpler and easier to follow than the equivalent stuff written with libc even though the setup was utterly trivial.

Anyway, I agree with the concern in general: there is a point where embedded calls to system() will become unmaintainable. It's just that I'd rather not institute a blanket ban.

[Jussi Pakkanen (jpakkane)]

I'm totally cool with using the shell, but there needs to be a very good reason for it. Usually because it provides functionality that is prohibitively expensive to replicate in C/C++. Examples include complex globbing, running shell pipelines gotten from (trusted) third party components and so on.

However "it is simpler for me" is not a good reason because that leads to "it is simpler for me to just recode this thing in Java/Vala/ObjC/Perl/Scheme/Rust/Whatever".

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2013-05-17 00:13:21 +0000
+++ CMakeLists.txt	2013-05-20 22:54:25 +0000
@@ -22,13 +22,18 @@
 #  * Switch build type to coverage (use ccmake or cmake-gui)
 #  * Invoke make, make test, make coverage (or ninja if you use that backend)
 #  * Find html report in subdir coveragereport
-#  * Find xml report feasible for jenkins in coverage.xml
+#  * Find xml report suitable for jenkins in coverage.xml
 #####################################################################
 if(cmake_build_type_lower MATCHES coverage)
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} --coverage" )
   set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} --coverage" )
   set(CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} --coverage" )
   set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} --coverage" )
+
+  # This allows us to skip the file descriptor closing test in Daemon_test
+  # when coverage is enabled. (Closing file descriptors
+  # in the test messes with coverage reporting.)
+  add_definitions(-DCOVERAGE_ENABLED)
 endif()
 
 # Make sure we have all the needed symbols

=== modified file 'src/unity/util/internal/DaemonImpl.cpp'
--- src/unity/util/internal/DaemonImpl.cpp	2013-05-13 11:39:49 +0000
+++ src/unity/util/internal/DaemonImpl.cpp	2013-05-20 22:54:25 +0000
@@ -48,6 +48,11 @@
 {
 }
 
+// LCOV_EXCL_START
+
+// This is tested, but only when coverage is disabled, because closing
+// file descriptors interferes with writing the coverage results.
+
 void
 DaemonImpl::
 close_fds() noexcept
@@ -55,6 +60,8 @@
     close_fds_ = true;
 }
 
+// LCOV_EXCL_STOP
+
 void
 DaemonImpl::
 reset_signals() noexcept
@@ -87,9 +94,23 @@
 {
     // Let's start by changing the working directory because that is the most likely thing to fail. If it does
     // fail, we have not modified any other properties of the calling process.
+    // We save the current working dir in case we need to restore it if a fork fails.
+
+    internal::ResourcePtr<int, std::function<void(int)>> old_working_dir(
+        [](int fd)
+        {
+            if (fd != -1)
+            {
+                int rc __attribute__((unused))
+                    = fchdir(fd);
+                close(fd);
+            }
+        }
+    );
 
     if (!working_directory_.empty())
     {
+        old_working_dir.reset(open(".", 0));                // Doesn't matter if this fails
         if (chdir(working_directory_.c_str()) == -1)
         {
             ostringstream msg;
@@ -104,15 +125,19 @@
     {
         case -1:
         {
+            // Strong exception guarantee: if working dir was changed, the old_working_dir
+            // destructor will try to restore it. This will work if at least one spare
+            // descriptor was avalable to start with. (Otherwise, old_working_dir won't
+            // be set and and we won't restore the previous working directory.)
             throw SyscallException("fork() failed", errno); // LCOV_EXCL_LINE
         }
         case 0:
         {
-            break;                  // Child process
+            break;                                          // Child process
         }
         default:
         {
-            exit(EXIT_SUCCESS);     // Parent process, we are done.
+            exit(EXIT_SUCCESS);                             // Parent process, we are done.
         }
     }
 
@@ -124,9 +149,10 @@
 
     // Set the umask if the caller asked for that. No error checking here because umask() cannot fail.
 
+    mode_t old_umask = 0;
     if (set_umask_)
     {
-        umask(umask_);
+        old_umask = umask(umask_);
     }
 
     // We are about to fork a second time, to prevent the process from re-acquiring a control terminal if it
@@ -147,17 +173,31 @@
         case -1:
         {
             // LCOV_EXCL_START
+            if (set_umask_)
+            {
+                umask(old_umask);                            // Strong exception guarantee
+            }
             sigaction(SIGHUP, &old_action, nullptr);         // Strong exception guarantee
-            throw SyscallException("fork() failed", errno);  // Better than trying to muddle on despite the problem.
+
+            // Strong exception guarantee: if working dir was changed, the old_working_dir
+            // destructor will try to restore it. This will work if at least one spare
+            // descriptor was avalable to start with. (Otherwise, old_working_dir won't
+            // be set and and we won't restore the previous working directory.)
+            throw SyscallException("fork() failed", errno);
             // LCOV_EXCL_STOP
         }
         case 0:
         {
-            break;                  // Child process
+            if (old_working_dir.has_resource())
+            {
+                close(old_working_dir.get());                // Reclaim file descriptor straight away
+                old_working_dir.release();                   // Don't restore previous working dir once we are done
+            }
+            break;                                           // Child process
         }
         default:
         {
-            exit(EXIT_SUCCESS);     // Parent process, we are done.
+            exit(EXIT_SUCCESS);                              // Parent process, we are done.
         }
     }
 
@@ -201,67 +241,53 @@
 DaemonImpl::
 close_open_files() noexcept
 {
+    // We close the standard file descriptors first. This allows opendir() to work if we need to close
+    // other files and are at the descriptor limit already.
+
+    close(0);
+    close(1);
+    close(2);
+
+    // LCOV_EXCL_START  // Closing file descriptors interferes with coverage reporting
     if (close_fds_)
     {
         // Close all open files. We use /proc to figure out what files are open.
-        // That's more efficient than calling close() potentially tens of thousands of times, once for each possible
-        // descriptor up to the process limit. We close the standard file descriptors as well because we need to
-        // re-open those later anyway.
+        // That's more efficient than calling close() potentially tens of thousands of times,
+        // once for each possible descriptor up to the process limit.
 
         char const* proc_self_fd = "/proc/self/fd";
 
-        internal::ResourcePtr<DIR*, decltype(&closedir)> dir(opendir(proc_self_fd), closedir);
-        if (dir.get() == nullptr)
+        DIR* dirp;
+        if ((dirp = opendir(proc_self_fd)) == nullptr)
         {
-            return;  // This should never happen but, for diligence, we check anyway. // LCOV_EXCL_LINE
+            return;     // This should never happen but, for diligence, we handle it anyway.
         }
+        internal::ResourcePtr<DIR*, decltype(&closedir)> dir(dirp, closedir);
 
         vector<int> descriptors; // We collect the file descriptors to close here
 
-        // We use the re-entrant version of readdir().
-
-        size_t len = offsetof(struct dirent, d_name) + pathconf(proc_self_fd, _PC_NAME_MAX) + 1;
-        unique_ptr<struct dirent, decltype(&free)> entry(reinterpret_cast<struct dirent*>(malloc(len)), free);
-
         struct dirent* result_p;
-
-        readdir_r(dir.get(), entry.get(), &result_p);  // Read first entry
-
-        while (result_p)
+        while ((result_p = readdir(dir.get())) != nullptr)
         {
-            if (result_p->d_name[0] != '.') // Ignore . and ..
+            // Try to treat the file name as a number. If it doesn't look like a number, we are looking at "." or ".." or,
+            // otherwise, something is seriously wrong because /proc/self/fd is supposed to contain only open file
+            // descriptor numbers. Rather than giving up in that case, we keep going, closing as many file descriptors as we can.
+            size_t pos;
+            int fd = std::stoi(result_p->d_name, &pos);
+            if (result_p->d_name[pos] == '\0')              // The file name did parse as a number
             {
-                // Try to treat the file name as a number. If it doesn't look like a number, something is seriously
-                // wrong because /proc/self/fd is supposed to contain only open file descriptor numbers. Rather than
-                // giving up in that case, we keep going, closing as many file descriptors as we can.
-
-                size_t pos;
-                int fd = std::stoi(result_p->d_name, &pos);
-                if (result_p->d_name[pos] == '\0')              // The file name did parse as a number
-                {
-                    // We can't call close() here because that would modify the directory while we are iterating
-                    // over it, which has undefined behavior.
-                    descriptors.push_back(fd);
-                }
+                // We can't call close() here because that would modify the directory while we are iterating
+                // over it, which has undefined behavior.
+                descriptors.push_back(fd);
             }
-            readdir_r(dir.get(), entry.get(), &result_p);  // Read next entry
         }
 
-        // Close all open descriptors.
-
         for (auto fd : descriptors)
         {
             close(fd);
         }
     }
-    else
-    {
-        // Caller asked for file descriptors to be left alone, so we only close the standard three.
-
-        close(0);
-        close(1);
-        close(2);
-    }
+    // LCOV_EXCL_STOP
 }
 
 } // namespace internal

=== modified file 'test/gtest/unity/util/Daemon/Daemon_test.cpp'
--- test/gtest/unity/util/Daemon/Daemon_test.cpp	2013-03-15 11:25:22 +0000
+++ test/gtest/unity/util/Daemon/Daemon_test.cpp	2013-05-20 22:54:25 +0000
@@ -33,8 +33,8 @@
 // as far as gtest is concerned, everything worked just fine.
 //
 // To deal with this, we create a file Daemon_test.out in the directory the test runs in. If the file is
-// empty after the test, we know that the test succeeded. Otherwise, it contains any failure messages
-// that were detected.
+// empty after the test, we know that the test succeeded. Otherwise, it contains messages
+// for any failure that were detected.
 //
 
 char const* error_file = "Daemon_test.out";
@@ -173,22 +173,13 @@
         error(__FILE__, __LINE__, "test file closed, should be open");
     }
 
-    // Daemonize again, asking for open files to be closed. The file we opened earlier should be closed.
-
-    d->close_fds();
-    d->daemonize_me();
-    check_std_descriptors();
-
-    if (is_open(fd))
-    {
-        error(__FILE__, __LINE__, "test file open, should be closed");
-    }
+    close(fd);
 }
 
 // Dummy signal handler
 
 void
-usr2_handler(int)
+hup_handler(int)
 {
 }
 
@@ -198,7 +189,7 @@
 
     Daemon::UPtr d = Daemon::create();
 
-    // Set SIGUSR1 to be ignored and set SIGUSR2 to be caught.
+    // Set SIGUSR1 to be ignored and set SIGHUP to be caught.
 
     struct sigaction usr1_action;
     memset(&usr1_action, 0, sizeof(usr1_action));           // To stop valgrind complaints
@@ -209,12 +200,12 @@
         abort();
     }
 
-    struct sigaction usr2_action;
-    memset(&usr2_action, 0, sizeof(usr1_action));           // To stop valgrind complaints
-    usr2_action.sa_handler = usr2_handler;
-    if (sigaction(SIGUSR2, &usr2_action, nullptr) == -1)
+    struct sigaction hup_action;
+    memset(&hup_action, 0, sizeof(hup_action));           // To stop valgrind complaints
+    hup_action.sa_handler = hup_handler;
+    if (sigaction(SIGHUP, &hup_action, nullptr) == -1)
     {
-        error(__FILE__, __LINE__, "cannot catch SIGUSR2");
+        error(__FILE__, __LINE__, "cannot catch SIGHUP");
         abort();
     }
 
@@ -232,14 +223,14 @@
         error(__FILE__, __LINE__, "SIGUSR1 should have been left alone, but wasn't");
     }
 
-    if (sigaction(SIGUSR2, &usr2_action, &prev_action) == -1)
+    if (sigaction(SIGHUP, &hup_action, &prev_action) == -1)
     {
-        error(__FILE__, __LINE__, "cannot restore SIGUSR2");
+        error(__FILE__, __LINE__, "cannot restore SIGHUP");
         abort();
     }
-    if (prev_action.sa_handler != usr2_action.sa_handler)
+    if (prev_action.sa_handler != hup_action.sa_handler)
     {
-        error(__FILE__, __LINE__, "SIGUSR2 should have been left alone, but wasn't");
+        error(__FILE__, __LINE__, "SIGHUP should have been left alone, but wasn't");
     }
 
     // Daemonize again, resetting signals, so we can check that they are at the defaults.
@@ -257,14 +248,14 @@
         error(__FILE__, __LINE__, "SIGUSR1 should have been reset, but wasn't");
     }
 
-    if (sigaction(SIGUSR2, &usr2_action, &prev_action) == -1)
+    if (sigaction(SIGHUP, &hup_action, &prev_action) == -1)
     {
-        error(__FILE__, __LINE__, "cannot set SIGUSR2");
+        error(__FILE__, __LINE__, "cannot set SIGHUP");
         abort();
     }
     if (prev_action.sa_handler != SIG_DFL)
     {
-        error(__FILE__, __LINE__, "SIGUSR2 should have been reset, but wasn't");
+        error(__FILE__, __LINE__, "SIGHUP should have been reset, but wasn't");
     }
 }
 
@@ -351,3 +342,43 @@
         error(__FILE__, __LINE__, "re-acquired control terminal but should not have been able to");
     }
 }
+
+// Test that file descriptors are closed.
+// We test this only when coverage is disabled because
+// closing descriptors interferes with coverage reporting.
+
+#if !defined(COVERAGE_ENABLED)
+
+TEST(Daemon, file_close)
+{
+    Daemon::UPtr d = Daemon::create();
+
+    // Open a file so we can check that the file is closed after daemonizing.
+
+    int fd = open(".", O_RDONLY);
+    if (fd == -1)
+    {
+        abort();
+    }
+
+    int fd2 = open(".", O_RDONLY);
+    if (fd2 == -1)
+    {
+        abort();
+    }
+
+    d->close_fds();
+    d->daemonize_me();
+    check_std_descriptors();
+
+    if (is_open(fd))
+    {
+        error(__FILE__, __LINE__, "fd open, should be closed");
+    }
+    if (is_open(fd2))
+    {
+        error(__FILE__, __LINE__, "fd2 open, should be closed");
+    }
+}
+
+#endif

=== modified file 'test/gtest/unity/util/FileIO/FileIO_test.cpp'
--- test/gtest/unity/util/FileIO/FileIO_test.cpp	2013-05-13 12:37:12 +0000
+++ test/gtest/unity/util/FileIO/FileIO_test.cpp	2013-05-20 22:54:25 +0000
@@ -29,8 +29,13 @@
 
 TEST(FileIO, basic)
 {
-    int i __attribute__((unused))
-        = system("rm -f testfile; echo \"some chars\" >testfile");
+    FILE* f;
+
+    remove("testfile");
+    f = fopen("testfile", "w");
+    EXPECT_NE(f, nullptr);
+    fputs("some chars\n", f);
+    fclose(f);
 
     string s = read_text_file("testfile");
     EXPECT_EQ("some chars\n", s);
@@ -39,7 +44,11 @@
     string contents("some chars\n");
     EXPECT_EQ(vector<uint8_t>(contents.begin(), contents.end()), v);
 
-    system("rm -fr empty; >empty");
+    remove("empty");
+    f = fopen("empty", "w");
+    EXPECT_NE(f, nullptr);
+    fclose(f);
+
     s = read_text_file("empty");
     EXPECT_TRUE(s.empty());
 }
@@ -59,8 +68,9 @@
 
     try
     {
-        int i __attribute__((unused))
-            = system("rm -fr testdir; mkdir testdir");
+        remove("testdir");
+        int rc = mkdir("testdir", 0777);
+        EXPECT_NE(-1, rc);
         read_text_file("testdir");
         FAIL();
     }