# HG changeset patch
# User Jed Davis <jld@mozilla.com>
# Date 1744054912 0
# Node ID 1c57033291daf61fa566e95fb6a29a4c48b100cb
# Parent  7f93ad9d921857cd1da56df9a59f80e45c119b04
Bug 1957749 - Detach Linux sandbox broker threads if needed to avoid memory leak. r=gerard-majax

Differential Revision: https://phabricator.services.mozilla.com/D244284

diff --git a/security/sandbox/linux/broker/SandboxBroker.cpp b/security/sandbox/linux/broker/SandboxBroker.cpp
--- a/security/sandbox/linux/broker/SandboxBroker.cpp
+++ b/security/sandbox/linux/broker/SandboxBroker.cpp
@@ -110,16 +110,21 @@ void SandboxBroker::Terminate() {
 
   // Join() on the same thread while working with errno EDEADLK is technically
   // not POSIX compliant:
   // https://pubs.opengroup.org/onlinepubs/9799919799/functions/pthread_join.html#:~:text=refers%20to%20the%20calling%20thread
   if (mThread != pthread_self()) {
     shutdown(mFileDesc, SHUT_RD);
     // The thread will now get EOF even if the client hasn't exited.
     PlatformThread::Join(mThread);
+  } else {
+    // Nothing is waiting for this thread, so detach it to avoid
+    // memory leaks.
+    int rv = pthread_detach(pthread_self());
+    MOZ_ALWAYS_TRUE(rv == 0);
   }
 
   // Now that the thread has exited, the fd will no longer be accessed.
   close(mFileDesc);
   // Having ensured that this object outlives the thread, this
   // destructor can now return.
 
   mFileDesc = -1;
diff --git a/security/sandbox/linux/gtest/TestBroker.cpp b/security/sandbox/linux/gtest/TestBroker.cpp
--- a/security/sandbox/linux/gtest/TestBroker.cpp
+++ b/security/sandbox/linux/gtest/TestBroker.cpp
@@ -21,16 +21,17 @@
 #include <sys/types.h>
 #include <time.h>
 #include <unistd.h>
 
 #include "mozilla/Atomics.h"
 #include "mozilla/PodOperations.h"
 #include "mozilla/UniquePtr.h"
 #include "mozilla/ipc/FileDescriptor.h"
+#include "nsIThreadManager.h"
 
 namespace mozilla {
 
 class SandboxBrokerTest : public ::testing::Test {
   static const int MAY_ACCESS = SandboxBroker::MAY_ACCESS;
   static const int MAY_READ = SandboxBroker::MAY_READ;
   static const int MAY_WRITE = SandboxBroker::MAY_WRITE;
   static const int MAY_CREATE = SandboxBroker::MAY_CREATE;
@@ -641,20 +642,23 @@ SandboxBrokerSigStress::DoSomething()
 #endif
 
 // Check for fd leaks when creating/destroying a broker instance (bug
 // 1719391).
 //
 // (This uses a different test group because it doesn't use the
 // fixture class, and gtest doesn't allow mixing TEST and TEST_F in
 // the same group.)
-TEST(SandboxBrokerMisc, LeakCheck)
+TEST(SandboxBrokerMisc, FileDescLeak)
 {
   // If this value is increased in the future, check that it won't
-  // cause the test to take an excessive amount of time:
+  // cause the test to take an excessive amount of time.
+  // (Alternately, this test could be changed to run a smaller number
+  // of cycles and check for increased fd usage afterwards; some care
+  // would be needed to avoid false positives.)
   static constexpr size_t kCycles = 4096;
   struct rlimit oldLimit;
   bool changedLimit = false;
 
   // At the time of this writing, we raise the fd soft limit to 4096
   // (or to the hard limit, if less than that), but we don't lower it
   // if it was higher than 4096.  To allow for that case, or if
   // Gecko's preferred limit changes, the limit is reduced while this
@@ -683,9 +687,45 @@ TEST(SandboxBrokerMisc, LeakCheck)
     broker->Terminate();
   }
 
   if (changedLimit) {
     ASSERT_EQ(setrlimit(RLIMIT_NOFILE, &oldLimit), 0) << strerror(errno);
   }
 }
 
+// Bug 1958444: In theory this test could fail intermittently: it
+// creates a large number of threads which will do a small amount of
+// work and then exit, but intentionally doesn't join them, so there
+// could be a large number of threads actually live at once, even if
+// they would be cleaned up correctly when they exit.
+//
+// To test locally, delete the `DISABLED_` prefix and rebuild.
+TEST(SandboxBrokerMisc, DISABLED_StackLeak)
+{
+  // The idea is that kCycles * DEFAULT_STACK_SIZE is more than 4GiB
+  // so this fails on 32-bit if the thread stack is leaked.  This
+  // isn't ideal; it would be better to either use resource limits
+  // (maybe RLIMIT_AS) or measure address space usage (getrusage or
+  // procfs), to detect such bugs with a smaller amount of leakage.
+  static constexpr size_t kCycles = 16384;
+
+  if (nsIThreadManager::DEFAULT_STACK_SIZE) {
+    EXPECT_GE(nsIThreadManager::DEFAULT_STACK_SIZE, size_t(256 * 1024));
+  }
+
+  pid_t pid = getpid();
+  for (size_t i = 0; i < kCycles; ++i) {
+    auto policy = MakeUnique<SandboxBroker::Policy>();
+    // Currently nothing in `Create` tries to check for or
+    // special-case an empty policy, but just in case:
+    policy->AddPath(SandboxBroker::MAY_READ, "/dev/null",
+                    SandboxBroker::Policy::AddAlways);
+    ipc::FileDescriptor fd;
+    RefPtr<SandboxBroker> broker =
+        SandboxBroker::Create(std::move(policy), pid, fd);
+    ASSERT_TRUE(broker != nullptr) << "iter " << i;
+    ASSERT_TRUE(fd.IsValid()) << "iter " << i;
+    broker = nullptr;
+  }
+}
+
 }  // namespace mozilla