Mailing-List: contact commits-help@kudu.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@kudu.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: mpercy@apache.org
To: commits@kudu.apache.org
Date: Mon, 06 Mar 2017 19:54:29 -0000
Message-Id: <104e58c7ddf4450b8c4a7b025665ac63@git.apache.org>
In-Reply-To: <3558eff627504a13a5c55729f3917f16@git.apache.org>
References: <3558eff627504a13a5c55729f3917f16@git.apache.org>
Subject: [2/2] kudu git commit: Ignore SIGPIPE earlier in startup process
archived-at: Mon, 06 Mar 2017 19:54:30 -0000

Ignore SIGPIPE earlier in startup process

This change resolves a race during startup where we are not protected
from SIGPIPE from the time we start the process until the time we start
the squeasel web server, which sets the disposition of SIGPIPE to
SIG_IGN.

This also factors some of the signal-handling helper functions into a
new set of util files, signal.{h,cc}.

Change-Id: I040bd38ff31451ed9e25e7cf2127c869cf08a628
Reviewed-on: http://gerrit.cloudera.org:8080/6262
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <todd@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/f65feff6
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/f65feff6
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/f65feff6

Branch: refs/heads/master
Commit: f65feff685099d0b166c4faf219c528476df4578
Parents: fff5cbd
Author: Mike Percy <mpercy@apache.org>
Authored: Fri Mar 3 17:15:35 2017 -0800
Committer: Mike Percy <mpercy@apache.org>
Committed: Mon Mar 6 19:52:37 2017 +0000

----------------------------------------------------------------------
 src/kudu/util/CMakeLists.txt |  1 +
 src/kudu/util/logging.cc     |  5 +++++
 src/kudu/util/logging.h      |  3 ++-
 src/kudu/util/signal.cc      | 47 +++++++++++++++++++++++++++++++++++++++
 src/kudu/util/signal.h       | 42 ++++++++++++++++++++++++++++++++++
 src/kudu/util/subprocess.cc  | 43 +++++++----------------------------
 src/kudu/util/test_main.cc   |  5 +++++
 7 files changed, 110 insertions(+), 36 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/f65feff6/src/kudu/util/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/src/kudu/util/CMakeLists.txt b/src/kudu/util/CMakeLists.txt
index 685f01f..7d049a9 100644
--- a/src/kudu/util/CMakeLists.txt
+++ b/src/kudu/util/CMakeLists.txt
@@ -175,6 +175,7 @@ set(UTIL_SRCS
   rw_mutex.cc
   rwc_lock.cc
   ${SEMAPHORE_CC}
+  signal.cc
   slice.cc
   spinlock_profiling.cc
   status.cc

http://git-wip-us.apache.org/repos/asf/kudu/blob/f65feff6/src/kudu/util/logging.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/logging.cc b/src/kudu/util/logging.cc
index f45285d..c62bfe7 100644
--- a/src/kudu/util/logging.cc
+++ b/src/kudu/util/logging.cc
@@ -41,6 +41,7 @@
 #include "kudu/util/env_util.h"
 #include "kudu/util/flag_tags.h"
 #include "kudu/util/minidump.h"
+#include "kudu/util/signal.h"
 #include "kudu/util/status.h"
 
 DEFINE_string(log_filename, "",
@@ -265,6 +266,10 @@ void InitGoogleLoggingSafe(const char* arg) {
   // Sink logging: off.
   initial_stderr_severity = FLAGS_stderrthreshold;
 
+  // Ignore SIGPIPE early in the startup process so that threads writing to TLS
+  // sockets do not crash when writing to a closed socket. See KUDU-1910.
+  IgnoreSigPipe();
+
   // For minidump support. Must be called before logging threads started.
   CHECK_OK(BlockSigUSR1());
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/f65feff6/src/kudu/util/logging.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/logging.h b/src/kudu/util/logging.h
index 1640793..682431e 100644
--- a/src/kudu/util/logging.h
+++ b/src/kudu/util/logging.h
@@ -258,7 +258,8 @@ class Env;
 // glog doesn't allow multiple invocations of InitGoogleLogging. This method conditionally
 // calls InitGoogleLogging only if it hasn't been called before.
 //
-// It also takes care of installing the google failure signal handler.
+// It also takes care of installing the google failure signal handler and
+// setting the signal handler for SIGPIPE to SIG_IGN.
 void InitGoogleLoggingSafe(const char* arg);
 
 // Like InitGoogleLoggingSafe() but stripped down: no signal handlers are

http://git-wip-us.apache.org/repos/asf/kudu/blob/f65feff6/src/kudu/util/signal.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/signal.cc b/src/kudu/util/signal.cc
new file mode 100644
index 0000000..2de3000
--- /dev/null
+++ b/src/kudu/util/signal.cc
@@ -0,0 +1,47 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "kudu/util/signal.h"
+
+#include "kudu/util/logging.h"
+
+namespace kudu {
+
+void SetSignalHandler(int signal, SignalHandlerCallback handler) {
+  struct sigaction act;
+  act.sa_handler = handler;
+  sigemptyset(&act.sa_mask);
+  act.sa_flags = 0;
+  PCHECK(sigaction(signal, &act, nullptr) == 0);
+}
+
+void IgnoreSigPipe() {
+  SetSignalHandler(SIGPIPE, SIG_IGN);
+}
+
+void ResetSigPipeHandlerToDefault() {
+  SetSignalHandler(SIGPIPE, SIG_DFL);
+}
+
+// We unblock all signal masks since they are inherited.
+void ResetAllSignalMasksToUnblocked() {
+  sigset_t signals;
+  PCHECK(sigfillset(&signals) == 0);
+  PCHECK(sigprocmask(SIG_UNBLOCK, &signals, nullptr) == 0);
+}
+
+} // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/f65feff6/src/kudu/util/signal.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/signal.h b/src/kudu/util/signal.h
new file mode 100644
index 0000000..0c88a80
--- /dev/null
+++ b/src/kudu/util/signal.h
@@ -0,0 +1,42 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <signal.h>
+
+namespace kudu {
+
+#if defined(__linux__)
+typedef sighandler_t SignalHandlerCallback;
+#else
+typedef sig_t SignalHandlerCallback;
+#endif
+
+// Set a process-wide signal handler.
+void SetSignalHandler(int signal, SignalHandlerCallback handler);
+
+// Set the disposition of SIGPIPE to SIG_IGN.
+void IgnoreSigPipe();
+
+// Set the disposition of SIGPIPE to SIG_DFL.
+void ResetSigPipeHandlerToDefault();
+
+// Unblock all signal masks.
+void ResetAllSignalMasksToUnblocked();
+
+} // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/f65feff6/src/kudu/util/subprocess.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/subprocess.cc b/src/kudu/util/subprocess.cc
index f69abe8..73d9672 100644
--- a/src/kudu/util/subprocess.cc
+++ b/src/kudu/util/subprocess.cc
@@ -46,6 +46,7 @@
 #include "kudu/gutil/strings/substitute.h"
 #include "kudu/util/debug-util.h"
 #include "kudu/util/errno.h"
+#include "kudu/util/signal.h"
 #include "kudu/util/status.h"
 
 using std::string;
@@ -72,42 +73,11 @@ static const char* kProcSelfFd =
 #if defined(__linux__)
 #define READDIR readdir64
 #define DIRENT dirent64
-typedef sighandler_t SignalHandlerCallback;
 #else
 #define READDIR readdir
 #define DIRENT dirent
-typedef sig_t SignalHandlerCallback;
 #endif
 
-// Convenience wrapper for sigaction().
-void SetSignalHandler(int signal, SignalHandlerCallback handler) {
-  struct sigaction act;
-  act.sa_handler = handler;
-  sigemptyset(&act.sa_mask);
-  act.sa_flags = 0;
-  PCHECK(sigaction(signal, &act, nullptr) == 0);
-}
-
-void IgnoreSigPipe() {
-  SetSignalHandler(SIGPIPE, SIG_IGN);
-}
-
-void ResetSigPipeHandlerToDefault() {
-  SetSignalHandler(SIGPIPE, SIG_DFL);
-}
-
-void EnsureSigPipeIgnored() {
-  static GoogleOnceType once = GOOGLE_ONCE_INIT;
-  GoogleOnceInit(&once, &IgnoreSigPipe);
-}
-
-// We unblock all signal masks since they are inherited.
-void ResetAllSignalMasksToUnblocked() {
-  sigset_t signals;
-  PCHECK(sigfillset(&signals) == 0);
-  PCHECK(sigprocmask(SIG_UNBLOCK, &signals, nullptr) == 0);
-}
-
 // Since opendir() calls malloc(), this must be called before fork().
 // This function is not async-signal-safe.
 Status OpenProcFdDir(DIR** dir) {
@@ -339,7 +309,9 @@ Status Subprocess::Start() {
   if (argv_.empty()) {
     return Status::InvalidArgument("argv must have at least one elem");
   }
-  EnsureSigPipeIgnored();
+
+  // We explicitly set SIGPIPE to SIG_IGN here because we are using UNIX pipes.
+  IgnoreSigPipe();
 
   vector<char*> argv_ptrs;
   for (const string& arg : argv_) {
@@ -429,9 +401,10 @@ Status Subprocess::Start() {
 
     // Ensure we are not ignoring or blocking signals in the child process.
     ResetAllSignalMasksToUnblocked();
-    // Reset SIGPIPE to its default disposition because we previously set it to
-    // SIG_IGN via EnsureSigPipeIgnored(). At the time of writing, we don't
-    // explicitly ignore any other signals in Kudu.
+
+    // Reset the disposition of SIGPIPE to SIG_DFL because we routinely set its
+    // disposition to SIG_IGN via IgnoreSigPipe(). At the time of writing, we
+    // don't explicitly ignore any other signals in Kudu.
     ResetSigPipeHandlerToDefault();
 
     // Set the environment for the subprocess. This is more portable than

http://git-wip-us.apache.org/repos/asf/kudu/blob/f65feff6/src/kudu/util/test_main.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/test_main.cc b/src/kudu/util/test_main.cc
index 74fe4e9..aa64387 100644
--- a/src/kudu/util/test_main.cc
+++ b/src/kudu/util/test_main.cc
@@ -25,6 +25,7 @@
 #include "kudu/util/pstack_watcher.h"
 #include "kudu/util/flags.h"
 #include "kudu/util/minidump.h"
+#include "kudu/util/signal.h"
 #include "kudu/util/status.h"
 #include "kudu/util/test_util.h"
 
@@ -71,6 +72,10 @@ int main(int argc, char **argv) {
   // need to block SIGUSR1 explicitly in order to test minidump generation.
   CHECK_OK(kudu::BlockSigUSR1());
 
+  // Ignore SIGPIPE for all tests so that threads writing to TLS
+  // sockets do not crash when writing to a closed socket. See KUDU-1910.
+  kudu::IgnoreSigPipe();
+
   // InitGoogleTest() must precede ParseCommandLineFlags(), as the former
   // removes gtest-related flags from argv that would trip up the latter.
   ::testing::InitGoogleTest(&argc, argv);