shill: vpn: Implement a ProcessKiller singleton.
This class may be used to terminate and reap child processes
asynchronously and robustly. Also:
- Use ProcessKiller to kill spawned "openvpn" and "l2tpipsec_vpn"
processes.
- Delete the associated OpenVPN tunnel interfaces cleanly after the
openvpn process dies.
- Fix a somewhat harmless bug where shill was sending SIGTERM to
already dead VPN processes from child watch callbacks.
BUG=chromium-os:31535,chromium-os:31571
TEST=unit tests, tested on device
Change-Id: If15f08e76c51dac33a434551ef4ba11ca66d0401
Reviewed-on: https://gerrit.chromium.org/gerrit/24610
Tested-by: Darin Petkov <petkov@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Commit-Ready: Darin Petkov <petkov@chromium.org>
diff --git a/Makefile b/Makefile
index f7012af..48447c7 100644
--- a/Makefile
+++ b/Makefile
@@ -176,6 +176,7 @@
portal_detector.o \
power_manager.o \
power_manager_proxy.o \
+ process_killer.o \
profile.o \
profile_dbus_adaptor.o \
profile_dbus_property_exporter.o \
@@ -301,6 +302,7 @@
mock_portal_detector.o \
mock_power_manager.o \
mock_power_manager_proxy.o \
+ mock_process_killer.o \
mock_profile.o \
mock_property_store.o \
mock_proxy_factory.o \
@@ -336,6 +338,7 @@
openvpn_management_server_unittest.o \
portal_detector_unittest.o \
power_manager_unittest.o \
+ process_killer_unittest.o \
profile_dbus_property_exporter_unittest.o \
profile_unittest.o \
property_accessor_unittest.o \
diff --git a/device_info.h b/device_info.h
index e18e6b9..a2de8c5 100644
--- a/device_info.h
+++ b/device_info.h
@@ -14,6 +14,7 @@
#include <base/file_path.h>
#include <base/memory/ref_counted.h>
#include <base/memory/scoped_ptr.h>
+#include <base/memory/weak_ptr.h>
#include <gtest/gtest_prod.h> // for FRIEND_TEST
#include "shill/byte_string.h"
@@ -30,7 +31,7 @@
class RTNLHandler;
class RTNLMessage;
-class DeviceInfo {
+class DeviceInfo : public base::SupportsWeakPtr<DeviceInfo> {
public:
struct AddressData {
AddressData()
@@ -51,7 +52,7 @@
EventDispatcher *dispatcher,
Metrics *metrics,
Manager *manager);
- ~DeviceInfo();
+ virtual ~DeviceInfo();
void AddDeviceToBlackList(const std::string &device_name);
bool IsDeviceBlackListed(const std::string &device_name);
diff --git a/l2tp_ipsec_driver.cc b/l2tp_ipsec_driver.cc
index 47d61f7..ba7525e 100644
--- a/l2tp_ipsec_driver.cc
+++ b/l2tp_ipsec_driver.cc
@@ -13,10 +13,12 @@
#include "shill/error.h"
#include "shill/manager.h"
#include "shill/nss.h"
+#include "shill/process_killer.h"
#include "shill/scope_logger.h"
#include "shill/vpn.h"
#include "shill/vpn_service.h"
+using base::Closure;
using std::map;
using std::string;
using std::vector;
@@ -74,6 +76,7 @@
device_info_(device_info),
glib_(glib),
nss_(NSS::GetInstance()),
+ process_killer_(ProcessKiller::GetInstance()),
pid_(0),
child_watch_tag_(0) {}
@@ -120,11 +123,9 @@
if (child_watch_tag_) {
glib_->SourceRemove(child_watch_tag_);
child_watch_tag_ = 0;
- CHECK(pid_);
- kill(pid_, SIGTERM);
}
if (pid_) {
- glib_->SpawnClosePID(pid_);
+ process_killer_->Kill(pid_, Closure());
pid_ = 0;
}
if (device_) {
@@ -318,6 +319,7 @@
L2TPIPSecDriver *me = reinterpret_cast<L2TPIPSecDriver *>(data);
me->child_watch_tag_ = 0;
CHECK_EQ(pid, me->pid_);
+ me->pid_ = 0;
me->Cleanup(Service::kStateFailure);
// TODO(petkov): Figure if we need to restart the connection.
}
diff --git a/l2tp_ipsec_driver.h b/l2tp_ipsec_driver.h
index 4f487e0..c266900 100644
--- a/l2tp_ipsec_driver.h
+++ b/l2tp_ipsec_driver.h
@@ -24,6 +24,7 @@
class GLib;
class Metrics;
class NSS;
+class ProcessKiller;
class L2TPIPSecDriver : public VPNDriver,
public RPCTaskDelegate {
@@ -114,6 +115,7 @@
DeviceInfo *device_info_;
GLib *glib_;
NSS *nss_;
+ ProcessKiller *process_killer_;
VPNServiceRefPtr service_;
scoped_ptr<RPCTask> rpc_task_;
diff --git a/l2tp_ipsec_driver_unittest.cc b/l2tp_ipsec_driver_unittest.cc
index 49762a6..ba21cf0 100644
--- a/l2tp_ipsec_driver_unittest.cc
+++ b/l2tp_ipsec_driver_unittest.cc
@@ -17,6 +17,7 @@
#include "shill/mock_manager.h"
#include "shill/mock_metrics.h"
#include "shill/mock_nss.h"
+#include "shill/mock_process_killer.h"
#include "shill/mock_vpn.h"
#include "shill/mock_vpn_service.h"
#include "shill/property_store_inspector.h"
@@ -49,6 +50,7 @@
device_(new MockVPN(&control_, &dispatcher_, &metrics_, &manager_,
kInterfaceName, kInterfaceIndex)) {
driver_->nss_ = &nss_;
+ driver_->process_killer_ = &process_killer_;
}
virtual ~L2TPIPSecDriverTest() {}
@@ -98,6 +100,7 @@
scoped_refptr<MockVPNService> service_;
scoped_refptr<MockVPN> device_;
MockNSS nss_;
+ MockProcessKiller process_killer_;
};
const char L2TPIPSecDriverTest::kInterfaceName[] = "ppp0";
@@ -144,7 +147,7 @@
EXPECT_CALL(glib_, SourceRemove(kTag));
const int kPID = 123456;
driver_->pid_ = kPID;
- EXPECT_CALL(glib_, SpawnClosePID(kPID));
+ EXPECT_CALL(process_killer_, Kill(kPID, _));
driver_->device_ = device_;
driver_->service_ = service_;
EXPECT_CALL(*device_, OnDisconnected());
@@ -352,7 +355,7 @@
const int kPID = 99999;
driver_->child_watch_tag_ = 333;
driver_->pid_ = kPID;
- EXPECT_CALL(glib_, SpawnClosePID(kPID));
+ EXPECT_CALL(process_killer_, Kill(_, _)).Times(0);
L2TPIPSecDriver::OnL2TPIPSecVPNDied(kPID, 2, driver_);
EXPECT_EQ(0, driver_->child_watch_tag_);
EXPECT_EQ(0, driver_->pid_);
diff --git a/mock_process_killer.cc b/mock_process_killer.cc
new file mode 100644
index 0000000..b3c3fb0
--- /dev/null
+++ b/mock_process_killer.cc
@@ -0,0 +1,13 @@
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "shill/mock_process_killer.h"
+
+namespace shill {
+
+MockProcessKiller::MockProcessKiller() {}
+
+MockProcessKiller::~MockProcessKiller() {}
+
+} // namespace shill
diff --git a/mock_process_killer.h b/mock_process_killer.h
new file mode 100644
index 0000000..42b4bab
--- /dev/null
+++ b/mock_process_killer.h
@@ -0,0 +1,27 @@
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SHILL_MOCK_PROCESS_KILLER_H_
+#define SHILL_MOCK_PROCESS_KILLER_H_
+
+#include <gmock/gmock.h>
+
+#include "shill/process_killer.h"
+
+namespace shill {
+
+class MockProcessKiller : public ProcessKiller {
+ public:
+ MockProcessKiller();
+ virtual ~MockProcessKiller();
+
+ MOCK_METHOD2(Kill, void(int pid, const base::Closure &callback));
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(MockProcessKiller);
+};
+
+} // namespace shill
+
+#endif // SHILL_MOCK_PROCESS_KILLER_H_
diff --git a/openvpn_driver.cc b/openvpn_driver.cc
index 0ed44d2..d01e40d 100644
--- a/openvpn_driver.cc
+++ b/openvpn_driver.cc
@@ -20,13 +20,16 @@
#include "shill/manager.h"
#include "shill/nss.h"
#include "shill/openvpn_management_server.h"
+#include "shill/process_killer.h"
#include "shill/rpc_task.h"
#include "shill/scope_logger.h"
#include "shill/sockets.h"
#include "shill/vpn.h"
#include "shill/vpn_service.h"
+using base::Closure;
using base::SplitString;
+using base::WeakPtr;
using std::map;
using std::string;
using std::vector;
@@ -34,6 +37,7 @@
namespace shill {
namespace {
+
const char kOpenVPNForeignOptionPrefix[] = "foreign_option_";
const char kOpenVPNIfconfigBroadcast[] = "ifconfig_broadcast";
const char kOpenVPNIfconfigLocal[] = "ifconfig_local";
@@ -55,6 +59,7 @@
const char kOpenVPNTLSAuthProperty[] = "OpenVPN.TLSAuth";
const char kOpenVPNVerbProperty[] = "OpenVPN.Verb";
const char kVPNMTUProperty[] = "VPN.MTU";
+
} // namespace
// static
@@ -130,6 +135,7 @@
glib_(glib),
management_server_(new OpenVPNManagementServer(this, glib)),
nss_(NSS::GetInstance()),
+ process_killer_(ProcessKiller::GetInstance()),
lsb_release_file_(kLSBReleaseFile),
pid_(0),
child_watch_tag_(0) {}
@@ -150,19 +156,26 @@
if (child_watch_tag_) {
glib_->SourceRemove(child_watch_tag_);
child_watch_tag_ = 0;
- CHECK(pid_);
- kill(pid_, SIGTERM);
- }
- if (pid_) {
- glib_->SpawnClosePID(pid_);
- pid_ = 0;
}
rpc_task_.reset();
+ int interface_index = -1;
if (device_) {
- int interface_index = device_->interface_index();
+ interface_index = device_->interface_index();
device_->OnDisconnected();
device_->SetEnabled(false);
device_ = NULL;
+ }
+ if (pid_) {
+ Closure callback;
+ if (interface_index >= 0) {
+ callback =
+ Bind(&DeleteInterface, device_info_->AsWeakPtr(), interface_index);
+ interface_index = -1;
+ }
+ process_killer_->Kill(pid_, callback);
+ pid_ = 0;
+ }
+ if (interface_index >= 0) {
device_info_->DeleteInterface(interface_index);
}
tunnel_interface_.clear();
@@ -230,10 +243,20 @@
OpenVPNDriver *me = reinterpret_cast<OpenVPNDriver *>(data);
me->child_watch_tag_ = 0;
CHECK_EQ(pid, me->pid_);
+ me->pid_ = 0;
me->Cleanup(Service::kStateFailure);
// TODO(petkov): Figure if we need to restart the connection.
}
+// static
+void OpenVPNDriver::DeleteInterface(WeakPtr<DeviceInfo> device_info,
+ int interface_index) {
+ if (device_info) {
+ LOG(INFO) << "Deleting interface " << interface_index;
+ device_info->DeleteInterface(interface_index);
+ }
+}
+
bool OpenVPNDriver::ClaimInterface(const string &link_name,
int interface_index) {
if (link_name != tunnel_interface_) {
diff --git a/openvpn_driver.h b/openvpn_driver.h
index 1d68f49..ef996f6 100644
--- a/openvpn_driver.h
+++ b/openvpn_driver.h
@@ -21,6 +21,13 @@
#include "shill/sockets.h"
#include "shill/vpn_driver.h"
+namespace base {
+
+template<typename T>
+class WeakPtr;
+
+} // namespace base;
+
namespace shill {
class ControlInterface;
@@ -29,6 +36,7 @@
class Metrics;
class NSS;
class OpenVPNManagementServer;
+class ProcessKiller;
class OpenVPNDriver : public VPNDriver,
public RPCTaskDelegate {
@@ -74,6 +82,7 @@
FRIEND_TEST(OpenVPNDriverTest, Cleanup);
FRIEND_TEST(OpenVPNDriverTest, Connect);
FRIEND_TEST(OpenVPNDriverTest, ConnectTunnelFailure);
+ FRIEND_TEST(OpenVPNDriverTest, DeleteInterface);
FRIEND_TEST(OpenVPNDriverTest, Disconnect);
FRIEND_TEST(OpenVPNDriverTest, GetRouteOptionEntry);
FRIEND_TEST(OpenVPNDriverTest, InitEnvironment);
@@ -141,6 +150,11 @@
// Called when the openpvn process exits.
static void OnOpenVPNDied(GPid pid, gint status, gpointer data);
+ // Standalone callback used to delete the tunnel interface when the openvpn
+ // process dies.
+ static void DeleteInterface(base::WeakPtr<DeviceInfo> device_info,
+ int interface_index);
+
// Inherit from VPNDriver to add custom properties.
virtual KeyValueStore GetProvider(Error *error);
@@ -156,6 +170,7 @@
Sockets sockets_;
scoped_ptr<OpenVPNManagementServer> management_server_;
NSS *nss_;
+ ProcessKiller *process_killer_;
FilePath lsb_release_file_;
VPNServiceRefPtr service_;
diff --git a/openvpn_driver_unittest.cc b/openvpn_driver_unittest.cc
index b0b6396..51f0b4c 100644
--- a/openvpn_driver_unittest.cc
+++ b/openvpn_driver_unittest.cc
@@ -22,6 +22,7 @@
#include "shill/mock_metrics.h"
#include "shill/mock_nss.h"
#include "shill/mock_openvpn_management_server.h"
+#include "shill/mock_process_killer.h"
#include "shill/mock_store.h"
#include "shill/mock_vpn.h"
#include "shill/mock_vpn_service.h"
@@ -32,6 +33,7 @@
#include "shill/vpn.h"
#include "shill/vpn_service.h"
+using base::WeakPtr;
using std::map;
using std::string;
using std::vector;
@@ -62,6 +64,7 @@
management_server_(new NiceMock<MockOpenVPNManagementServer>()) {
driver_->management_server_.reset(management_server_);
driver_->nss_ = &nss_;
+ driver_->process_killer_ = &process_killer_;
}
virtual ~OpenVPNDriverTest() {}
@@ -121,6 +124,7 @@
scoped_refptr<MockVPNService> service_;
scoped_refptr<MockVPN> device_;
MockNSS nss_;
+ MockProcessKiller process_killer_;
// Owned by |driver_|.
NiceMock<MockOpenVPNManagementServer> *management_server_;
@@ -576,10 +580,10 @@
// Stop will be called twice -- once by Cleanup and once by the destructor.
EXPECT_CALL(*management_server_, Stop()).Times(2);
EXPECT_CALL(glib_, SourceRemove(kTag));
- EXPECT_CALL(glib_, SpawnClosePID(kPID));
+ EXPECT_CALL(process_killer_, Kill(kPID, _));
+ EXPECT_CALL(device_info_, DeleteInterface(_)).Times(0);
EXPECT_CALL(*device_, OnDisconnected());
EXPECT_CALL(*device_, SetEnabled(false));
- EXPECT_CALL(device_info_, DeleteInterface(kInterfaceIndex));
EXPECT_CALL(*service_, SetState(Service::kStateFailure));
driver_->Cleanup(Service::kStateFailure);
EXPECT_EQ(0, driver_->child_watch_tag_);
@@ -632,9 +636,13 @@
TEST_F(OpenVPNDriverTest, OnOpenVPNDied) {
const int kPID = 99999;
+ driver_->device_ = device_;
driver_->child_watch_tag_ = 333;
driver_->pid_ = kPID;
- EXPECT_CALL(glib_, SpawnClosePID(kPID));
+ EXPECT_CALL(*device_, OnDisconnected());
+ EXPECT_CALL(*device_, SetEnabled(false));
+ EXPECT_CALL(process_killer_, Kill(_, _)).Times(0);
+ EXPECT_CALL(device_info_, DeleteInterface(kInterfaceIndex));
OpenVPNDriver::OnOpenVPNDied(kPID, 2, driver_);
EXPECT_EQ(0, driver_->child_watch_tag_);
EXPECT_EQ(0, driver_->pid_);
@@ -747,4 +755,18 @@
EXPECT_EQ(0, env.size());
}
+TEST_F(OpenVPNDriverTest, DeleteInterface) {
+ scoped_ptr<MockDeviceInfo> device_info(
+ new MockDeviceInfo(&control_, &dispatcher_, &metrics_, &manager_));
+ EXPECT_CALL(*device_info, DeleteInterface(kInterfaceIndex))
+ .WillOnce(Return(true));
+ WeakPtr<DeviceInfo> weak = device_info->AsWeakPtr();
+ EXPECT_TRUE(weak);
+ OpenVPNDriver::DeleteInterface(weak, kInterfaceIndex);
+ device_info.reset();
+ EXPECT_FALSE(weak);
+ // Expect no crash.
+ OpenVPNDriver::DeleteInterface(weak, kInterfaceIndex);
+}
+
} // namespace shill
diff --git a/process_killer.cc b/process_killer.cc
new file mode 100644
index 0000000..4e1383e
--- /dev/null
+++ b/process_killer.cc
@@ -0,0 +1,58 @@
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "shill/process_killer.h"
+
+using base::Closure;
+using std::map;
+
+namespace shill {
+
+namespace {
+
+base::LazyInstance<ProcessKiller> g_process_killer = LAZY_INSTANCE_INITIALIZER;
+
+} // namespace
+
+ProcessKiller::ProcessKiller() {}
+
+ProcessKiller::~ProcessKiller() {
+ // There's no need to release any GLib child watch sources because this class
+ // is a singleton and will be destroyed when this process exits, i.e., when
+ // GLib is shut down.
+}
+
+// static
+ProcessKiller *ProcessKiller::GetInstance() {
+ return g_process_killer.Pointer();
+}
+
+void ProcessKiller::Kill(int pid, const Closure &callback) {
+ LOG(INFO) << "Killing pid " << pid;
+ if (!callback.is_null()) {
+ callbacks_[pid] = callback;
+ }
+ g_child_watch_add(pid, OnProcessDied, this);
+ // TODO(petkov): Consider sending subsequent periodic signals and raising the
+ // signal to SIGKILL if the process keeps running.
+ kill(pid, SIGTERM);
+}
+
+// static
+void ProcessKiller::OnProcessDied(GPid pid, gint status, gpointer data) {
+ LOG(INFO) << "pid " << pid << " died, status " << status;
+ ProcessKiller *me = reinterpret_cast<ProcessKiller *>(data);
+ map<int, Closure>::iterator callback_it = me->callbacks_.find(pid);
+ if (callback_it == me->callbacks_.end()) {
+ return;
+ }
+ const Closure &callback = callback_it->second;
+ if (!callback.is_null()) {
+ LOG(INFO) << "Running callback for dead pid " << pid;
+ callback.Run();
+ }
+ me->callbacks_.erase(callback_it);
+}
+
+} // namespace shill
diff --git a/process_killer.h b/process_killer.h
new file mode 100644
index 0000000..fb5cab2
--- /dev/null
+++ b/process_killer.h
@@ -0,0 +1,48 @@
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SHILL_PROCESS_KILLER_H_
+#define SHILL_PROCESS_KILLER_H_
+
+#include <map>
+
+#include <base/callback.h>
+#include <base/lazy_instance.h>
+#include <glib.h>
+#include <gtest/gtest_prod.h> // for FRIEND_TEST
+
+namespace shill {
+
+// The ProcessKiller singleton can be used to asynchronously and robustly
+// terminate and reap child processes by their process IDs.
+class ProcessKiller {
+ public:
+ virtual ~ProcessKiller();
+
+ // This is a singleton -- use ProcessKiller::GetInstance()->Foo()
+ static ProcessKiller *GetInstance();
+
+ // Terminates process |pid|. Uses GLib to wait asynchronously for the process
+ // to exit. GLib supports only a single callback per process ID so there
+ // should be no other child watch callbacks registered for this |pid|. If
+ // |callback| is non-null, runs it when the process exits.
+ virtual void Kill(int pid, const base::Closure &callback);
+
+ protected:
+ ProcessKiller();
+
+ private:
+ friend struct base::DefaultLazyInstanceTraits<ProcessKiller>;
+ FRIEND_TEST(ProcessKillerTest, OnProcessDied);
+
+ static void OnProcessDied(GPid pid, gint status, gpointer data);
+
+ std::map<int, base::Closure> callbacks_;
+
+ DISALLOW_COPY_AND_ASSIGN(ProcessKiller);
+};
+
+} // namespace shill
+
+#endif // SHILL_PROCESS_KILLER_H_
diff --git a/process_killer_unittest.cc b/process_killer_unittest.cc
new file mode 100644
index 0000000..477d9a4
--- /dev/null
+++ b/process_killer_unittest.cc
@@ -0,0 +1,48 @@
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "shill/process_killer.h"
+
+#include <base/bind.h>
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+using base::Bind;
+using base::Closure;
+using base::Unretained;
+
+namespace shill {
+
+class ProcessKillerTest : public testing::Test {
+ public:
+ ProcessKillerTest() : process_killer_(ProcessKiller::GetInstance()) {}
+
+ protected:
+ class Target {
+ public:
+ virtual ~Target() {}
+
+ MOCK_METHOD0(Call, void());
+ };
+
+ ProcessKiller *process_killer_;
+};
+
+TEST_F(ProcessKillerTest, OnProcessDied) {
+ const int kPID = 123;
+ EXPECT_TRUE(process_killer_->callbacks_.empty());
+ // Expect no crash.
+ ProcessKiller::OnProcessDied(kPID, 0, process_killer_);
+ process_killer_->callbacks_[kPID] = Closure();
+ // Expect no crash.
+ ProcessKiller::OnProcessDied(kPID, 0, process_killer_);
+ EXPECT_TRUE(process_killer_->callbacks_.empty());
+ Target target;
+ EXPECT_CALL(target, Call());
+ process_killer_->callbacks_[kPID] = Bind(&Target::Call, Unretained(&target));
+ ProcessKiller::OnProcessDied(kPID, 0, process_killer_);
+ EXPECT_TRUE(process_killer_->callbacks_.empty());
+}
+
+} // namespace shill