Merge lp:~unity-api-team/indicator-network/lp1411714-14.09 into lp:indicator-network/14.09

[Antti Kaijanmäki]

* Guard access to ofono::Modem properties that have a
  shared_ptr inside them with a mutex.
* simplify GMainLoopDispatcher

The crash was caused by a memory corruption caused by a threading issue.
One example http://pastebin.ubuntu.com/9805338/

    on frame #20 after the control leaves Modem::Private::update()
    the stack is unwinded which results in destructor of SimManager
    to be called on frame #11, although the simmgr shared_ptr inside
    update() appears to be empty (points to 0x0) and the destuctor for
    the SimManager object has already been ran once.

The cause of the corruption is explained here:
http://cppwisdom.quora.com/shared_ptr-is-almost-thread-safe

org::ofono::Interface::Modem has two core::Properties that contain shared_ptr:

    core::Property<NetworkRegistration::Ptr> networkRegistration;
    core::Property<SimManager::Ptr> simManager;

Core::Property<T> defines:

  private:
    mutable T value;

Which in SimManager's case for example becomes:

    mutable std::shared_ptr<SimManager> value;

Core::Property<T> defines the get() and set() as:

    inline virtual const T& get() const
    {
        if (getter)
            mutable_get() = getter();

        return value;
    }

    inline virtual void set(const T& new_value)
    {
        if (value != new_value)
        {
            value = new_value;

            if (setter)
                setter(value);

            signal_changed(value);
        }
    }

Now, in indicator-network we have two threads running:

    thread 1: worker thread for dbus-cpp
    thread 2: thread that runs GMainLoop

As an example the corruption happens when thread 1 does in ofono.h:
    simManager.set(std::shared_ptr<SimManager>());

and at the same time thread 2 does in modem.cpp:
    auto simmgr = d->m_ofonoModem->simManager.get();

set() writes to core::Property::value in one thread while get() reads the value from another simultaneously.

---

The situation is fixed in this MP by guarding the read and write to the properties by a mutex org::ofono::Interface::Modem::_lock.

---

This MP also simplifies the locking around GMainLoopDispatch as the original code was seen as error prone regarding of locking.

[Jussi Pakkanen (jpakkane)]

There are several calls like this:

std::unique_lock<std::mutex> lock(m_ofonoModem->_lock);
auto simmgr = m_ofonoModem->simManager.get();
auto netreg = m_ofonoModem->networkRegistration.get();
lock.unlock();

That is, the caller needs to be aware of the locking procedures. This is a potential future problem waiting to happen because when new calls to this function are added, one or more of them will forget to do this.

Could we instead change the getter function to be something like this:

void get(std::shared_ptr<Foo> &o) {
   // Comment saying why we do this with a link to the web page
   std::unique_lock<...> ...
   o = internal_variable;
}

In this way the get function is guaranteed to be thread safe as it can not be called incorrectly.

review: Needs Fixing

[Pete Woods (pete-woods)]

At a purely functional level, these changes look reasonable to me.

I agree with Jussi's comment, however.

review: Approve

[Jussi Pakkanen (jpakkane)]

Apparently the lock thing is harder than it appears.

review: Approve

=== modified file 'src/dbus-cpp/services/ofono.h'
--- src/dbus-cpp/services/ofono.h	2014-10-23 21:46:41 +0000
+++ src/dbus-cpp/services/ofono.h	2015-01-23 11:25:44 +0000
@@ -1034,8 +1034,9 @@
                         break;
                     case Property::Interfaces::Type::NetworkRegistration:
                     {
+                        std::unique_lock<std::mutex> lock(_lock);
                         if (std::find(newInterfaces.begin(), newInterfaces.end(), known) == newInterfaces.end() &&
-                            networkRegistration->get()) {
+                            networkRegistration.get()) {
                             networkRegistration.set(std::shared_ptr<NetworkRegistration>());
                         }
                         break;
@@ -1046,8 +1047,9 @@
                         break;
                     case Property::Interfaces::Type::SimManager:
                     {
+                        std::unique_lock<std::mutex> lock(_lock);
                         if (std::find(newInterfaces.begin(), newInterfaces.end(), known) == newInterfaces.end() &&
-                            simManager->get()) {
+                            simManager.get()) {
                             simManager.set(std::shared_ptr<SimManager>());
                         }
                         break;
@@ -1080,7 +1082,8 @@
                         break;
                     case Property::Interfaces::Type::NetworkRegistration:
                     {
-                        if (!networkRegistration->get()) {
+                        std::unique_lock<std::mutex> lock(_lock);
+                        if (!networkRegistration.get()) {
                             networkRegistration.set(std::make_shared<NetworkRegistration>(this->object));
                         }
                         break;
@@ -1091,7 +1094,8 @@
                         break;
                     case Property::Interfaces::Type::SimManager:
                     {
-                        if (!simManager->get()) {
+                        std::unique_lock<std::mutex> lock(_lock);
+                        if (!simManager.get()) {
                             simManager.set(std::make_shared<SimManager>(this->object));
                         }
                         break;
@@ -1174,6 +1178,8 @@
 
         std::shared_ptr<core::dbus::Signal<Signal::PropertyChanged, Signal::PropertyChanged::ArgumentType>> propertyChanged;
 
+        // this lock must be acquired for any access to networkRegistration or simManager
+        std::mutex _lock;
         core::Property<NetworkRegistration::Ptr> networkRegistration;
         core::Property<SimManager::Ptr>          simManager;
 

=== modified file 'src/indicator/modem.cpp'
--- src/indicator/modem.cpp	2014-12-09 10:05:45 +0000
+++ src/indicator/modem.cpp	2015-01-23 11:25:44 +0000
@@ -65,20 +65,21 @@
         });
     });
 
-    simManagerChanged(m_ofonoModem->simManager.get());
+    std::unique_lock<std::mutex> lock(m_ofonoModem->_lock);
+    auto simmgr = m_ofonoModem->simManager.get();
+    auto netreg = m_ofonoModem->networkRegistration.get();
+    lock.unlock();
+
+    simManagerChanged(simmgr);
     m_ofonoModem->simManager.changed().connect([that](org::ofono::Interface::SimManager::Ptr simmgr)
     {
-        GMainLoopDispatch([that, simmgr](){
-            that->simManagerChanged(simmgr);
-        });
+        that->simManagerChanged(simmgr);
     });
 
-    networkRegistrationChanged(m_ofonoModem->networkRegistration.get());
+    networkRegistrationChanged(netreg);
     m_ofonoModem->networkRegistration.changed().connect([that](org::ofono::Interface::NetworkRegistration::Ptr netreg)
     {
-        GMainLoopDispatch([that, netreg](){
-            that->networkRegistrationChanged(netreg);
-        });
+        that->networkRegistrationChanged(netreg);
     });
 
     /// @todo hook up with system-settings to allow changing the identifier.
@@ -93,14 +94,17 @@
     } else {
         m_simIdentifier.set(path);
     }
-
 }
 
 
 void
 Modem::Private::update()
 {
+    std::unique_lock<std::mutex> lock(m_ofonoModem->_lock);
     auto simmgr = m_ofonoModem->simManager.get();
+    auto netreg = m_ofonoModem->networkRegistration.get();
+    lock.unlock();
+
     if (simmgr) {
         // update requiredPin
         switch(simmgr->pinRequired.get())
@@ -155,7 +159,6 @@
         m_simStatus.set(SimStatus::not_available);
     }
 
-    auto netreg = m_ofonoModem->networkRegistration.get();
     if (netreg) {
         m_operatorName.set(netreg->operatorName.get());
         m_status.set(netreg->status.get());
@@ -172,75 +175,55 @@
 void
 Modem::Private::networkRegistrationChanged(org::ofono::Interface::NetworkRegistration::Ptr netreg)
 {
+    auto that = shared_from_this();
     if (netreg) {
-        auto that = shared_from_this();
         netreg->operatorName.changed().connect([that](const std::string &)
         {
-            GMainLoopDispatch([that]()
-            {
-                that->update();
-            });
+            GMainLoopDispatch([that]() { that->update(); });
         });
         netreg->status.changed().connect([that](org::ofono::Interface::NetworkRegistration::Status)
         {
-            GMainLoopDispatch([that]()
-            {
-                that->update();
-            });
+            GMainLoopDispatch([that]() { that->update(); });
         });
 
         netreg->strength.changed().connect([that](std::int8_t)
         {
-            GMainLoopDispatch([that]()
-            {
-                that->update();
-            });
+            GMainLoopDispatch([that]() { that->update(); });
         });
 
         netreg->technology.changed().connect([that](org::ofono::Interface::NetworkRegistration::Technology)
         {
-            GMainLoopDispatch([that]()
-            {
-                that->update();
-            });
+            GMainLoopDispatch([that]() { that->update(); });
         });
 
     }
-    update();
+
+    GMainLoopDispatch([that]() { that->update(); });
 }
 
 
 void
 Modem::Private::simManagerChanged(org::ofono::Interface::SimManager::Ptr simmgr)
 {
+    auto that = shared_from_this();
     if (simmgr) {
-        auto that = shared_from_this();
         simmgr->present.changed().connect([that](bool)
         {
-            GMainLoopDispatch([that]()
-            {
-                that->update();
-            });
+            GMainLoopDispatch([that]() { that->update(); });
         });
 
         simmgr->pinRequired.changed().connect([that](org::ofono::Interface::SimManager::PinType)
         {
-            GMainLoopDispatch([that]()
-            {
-                that->update();
-            });
+            GMainLoopDispatch([that]() { that->update(); });
         });
 
         simmgr->retries.changed().connect([that](std::map<org::ofono::Interface::SimManager::PinType, std::uint8_t>)
         {
-            GMainLoopDispatch([that]()
-            {
-                that->update();
-            });
+            GMainLoopDispatch([that]() { that->update(); });
         });
 
     }
-    update();
+    GMainLoopDispatch([that]() { that->update(); });
 }
 
 Modem::Modem(org::ofono::Interface::Modem::Ptr ofonoModem)
@@ -261,7 +244,11 @@
 bool
 Modem::enterPin(PinType type, const std::string &pin)
 {
-    if (!d->m_ofonoModem->simManager.get()) {
+    std::unique_lock<std::mutex> lock(d->m_ofonoModem->_lock);
+    auto simmgr = d->m_ofonoModem->simManager.get();
+    lock.unlock();
+
+    if (!simmgr) {
         throw std::runtime_error(std::string(__PRETTY_FUNCTION__) + ": no simManager.");
     }
 
@@ -269,11 +256,11 @@
     case PinType::none:
         return true;
     case PinType::pin:
-        return d->m_ofonoModem->simManager.get()->enterPin(org::ofono::Interface::SimManager::PinType::pin,
-                                                           pin);
+        return simmgr->enterPin(org::ofono::Interface::SimManager::PinType::pin,
+                                pin);
     case PinType::puk:
-        return d->m_ofonoModem->simManager.get()->enterPin(org::ofono::Interface::SimManager::PinType::puk,
-                                                           pin);
+        return simmgr->enterPin(org::ofono::Interface::SimManager::PinType::puk,
+                                pin);
         break;
     }
 
@@ -284,7 +271,11 @@
 bool
 Modem::resetPin(PinType type, const std::string &puk, const std::string &pin)
 {
-    if (!d->m_ofonoModem->simManager.get()) {
+    std::unique_lock<std::mutex> lock(d->m_ofonoModem->_lock);
+    auto simmgr = d->m_ofonoModem->simManager.get();
+    lock.unlock();
+
+    if (!simmgr) {
         throw std::runtime_error(std::string(__PRETTY_FUNCTION__) + ": no simManager.");
     }
 
@@ -292,9 +283,9 @@
     case PinType::none:
         return true;
     case PinType::puk:
-        return d->m_ofonoModem->simManager.get()->resetPin(org::ofono::Interface::SimManager::PinType::puk,
-                                                           puk,
-                                                           pin);
+        return simmgr->resetPin(org::ofono::Interface::SimManager::PinType::puk,
+                                puk,
+                                pin);
     default:
         throw std::runtime_error(std::string(__PRETTY_FUNCTION__) + ": Not Supported.");
     }
@@ -303,7 +294,11 @@
 bool
 Modem::changePin(PinType type, const std::string &oldPin, const std::string &newPin)
 {
-    if (!d->m_ofonoModem->simManager.get()) {
+    std::unique_lock<std::mutex> lock(d->m_ofonoModem->_lock);
+    auto simmgr = d->m_ofonoModem->simManager.get();
+    lock.unlock();
+
+    if (!simmgr) {
         throw std::runtime_error(std::string(__PRETTY_FUNCTION__) + ": no simManager.");
     }
 
@@ -311,13 +306,13 @@
     case PinType::none:
         return true;
     case PinType::pin:
-        return d->m_ofonoModem->simManager.get()->changePin(org::ofono::Interface::SimManager::PinType::pin,
-                                                            oldPin,
-                                                            newPin);
+        return simmgr->changePin(org::ofono::Interface::SimManager::PinType::pin,
+                                 oldPin,
+                                 newPin);
     case PinType::puk:
-        return d->m_ofonoModem->simManager.get()->changePin(org::ofono::Interface::SimManager::PinType::puk,
-                                                            oldPin,
-                                                            newPin);
+        return simmgr->changePin(org::ofono::Interface::SimManager::PinType::puk,
+                                 oldPin,
+                                 newPin);
         break;
     }
 

=== modified file 'src/menumodel-cpp/gio-helpers/util.cpp'
--- src/menumodel-cpp/gio-helpers/util.cpp	2014-12-09 14:47:21 +0000
+++ src/menumodel-cpp/gio-helpers/util.cpp	2015-01-23 11:25:44 +0000
@@ -19,43 +19,40 @@
 
 #include "util.h"
 
+// guards access to __funcs
 std::mutex GMainLoopDispatch::_lock;
-std::list<GMainLoopDispatch::Func *> GMainLoopDispatch::_funcs;
+std::list<GMainLoopDispatch::Func> GMainLoopDispatch::__funcs;
 
 gboolean
 GMainLoopDispatch::dispatch_cb(gpointer)
 {
     std::unique_lock<std::mutex> lock(_lock);
-    for (auto func : _funcs) {
-        lock.unlock();
-        (*func)();
-        lock.lock();
-        delete func;
+    auto funcs = __funcs;
+    __funcs.clear();
+    lock.unlock();
+
+    for (auto &func : funcs) {
+        func();
     }
-    _funcs.clear();
     return G_SOURCE_REMOVE;
 }
 
 GMainLoopDispatch::GMainLoopDispatch(Func func, int priority, bool force_delayed)
 {
-    std::unique_lock<std::mutex> lock(_lock);
-
     if (!force_delayed &&
          g_main_context_acquire(g_main_context_default())) {
-        // already running inside GMainLoop..
-        // free the lock and dispatch immediately.
-        lock.unlock();
+        // already running inside GMainLoop.
         func();
         g_main_context_release(g_main_context_default());
     } else {
-        std::function<void()> *funcPtr = new std::function<void()>(func);
-        if (_funcs.empty()) {
+        std::unique_lock<std::mutex> lock(_lock);
+        if (__funcs.empty()) {
             g_idle_add_full(priority,
                             GSourceFunc(GMainLoopDispatch::dispatch_cb),
                             NULL,
                             NULL);
         }
-        _funcs.push_back(funcPtr);
+        __funcs.push_back(func);
     }
 }
 

=== modified file 'src/menumodel-cpp/gio-helpers/util.h'
--- src/menumodel-cpp/gio-helpers/util.h	2014-12-09 14:47:21 +0000
+++ src/menumodel-cpp/gio-helpers/util.h	2015-01-23 11:25:44 +0000
@@ -74,7 +74,7 @@
     typedef std::function<void()> Func;
     static std::mutex _lock;
     // list keeps the order of the functions
-    static std::list<Func *> _funcs;
+    static std::list<Func> __funcs;
 
     //GMainLoopDispatch() = delete;
     GMainLoopDispatch(Func func, int priority=G_PRIORITY_HIGH, bool force_delayed=false);