commit 1c1dd351729e4d60ab24fdbfaf4fc439f14876c2
author mukesh agrawal <quiche@chromium.org> Wed May 08 15:58:34 2013 -0700
committer ChromeBot <chrome-bot@google.com> Wed May 08 18:41:15 2013 -0700
tree 378e0d93199266d1c9c471c9f53ff92fbe038e7d
parent 3fd54bd6e0c9bb81f4324cf96b53d35e7f134813

shill: fix crash in IPConfig::UpdateProperties

If UpdateProperties reports failure to |update_callback_|, the
device owning the IPConfig object may drop its reference to
the IPConfig. We'll then crash, when we try to EmitChanges.

Avoid the crash by having UpdateProperties hold a reference
to the IPConfig object for the duration of the method call.

BUG=chromium:237576
TEST=new unit test

Change-Id: Ia3ce1a4729429ec75d4aab6b4c84b67f573d9187
Reviewed-on: https://gerrit.chromium.org/gerrit/50509
Reviewed-by: Paul Stewart <pstew@chromium.org>
Commit-Queue: mukesh agrawal <quiche@chromium.org>
Tested-by: mukesh agrawal <quiche@chromium.org>

ipconfig.cc

diff --git a/ipconfig.cc b/ipconfig.cc
index 4d8fede..ce8c9c1 100644
--- a/ipconfig.cc
+++ b/ipconfig.cc
@@ -122,6 +122,11 @@
 }
 
 void IPConfig::UpdateProperties(const Properties &properties, bool success) {
+  // Take a reference of this instance to make sure we don't get destroyed in
+  // the middle of this call. (The |update_callback_| may cause a reference
+  // to be dropped. See, e.g., EthernetService::Disconnect and
+  // Ethernet::DropConnection.)
+  IPConfigRefPtr me = this;
   properties_ = properties;
   if (!update_callback_.is_null()) {
     update_callback_.Run(this, success);

ipconfig_unittest.cc

diff --git a/ipconfig_unittest.cc b/ipconfig_unittest.cc
index 3308ada..91b18bb 100644
--- a/ipconfig_unittest.cc
+++ b/ipconfig_unittest.cc
@@ -34,6 +34,9 @@
 class IPConfigTest : public Test {
  public:
   IPConfigTest() : ipconfig_(new IPConfig(&control_, kDeviceName)) {}
+  void DropRef(const IPConfigRefPtr &/*ipconfig*/, bool /*config_success*/) {
+    ipconfig_ = NULL;
+  }
 
  protected:
   IPConfigMockAdaptor *GetAdaptor() {
@@ -146,6 +149,14 @@
   }
 }
 
+TEST_F(IPConfigTest, UpdatePropertiesWithDropRef) {
+  // The UpdateCallback should be able to drop a reference to the
+  // IPConfig object without crashing.
+  ipconfig_->RegisterUpdateCallback(
+      Bind(&IPConfigTest::DropRef, Unretained(this)));
+  UpdateProperties(IPConfig::Properties(), true);
+}
+
 TEST_F(IPConfigTest, PropertyChanges) {
   IPConfigMockAdaptor *adaptor = GetAdaptor();