Crash during erase from map

Admin 1
Viewed 629

I have a crash as consequence of unexpected behaviour.. I want to ask how could I modify the code to defence against this crash what happened.

   void SubscManag::handleNotif(Notif notif)
    {
   
        std::cout << "Current subscribe size: " << subs_.size();
        std::map<int, SubscData>::iterator it;
        {
            std::lock_guard<std::recursive_mutex> lock(mutex_);
            it = subs_.begin();
        }
        for (;;)
        {
            std::lock_guard<std::recursive_mutex> lock(mutex_);
            if (it == subs_.end())
            {
                break;
            }
            if (it->second.hasToBeErased)
            {
                std::cout << "Erase id : " << it->first;
                it = subs_.erase(it);
                continue;
            }
            if (cond)
            {
                // ....
                ++it;
                continue;
            }
            // ....
            ++it;
        }
    }

Crash is at line it = subs_.erase(it); with this

 Erase id : 2
 Erase id : 3
 Erase id : 28473456

So probably Element with id : 28473456 does not exist or was corrupted but how could I defense against crash ? Thank you.

c++14 crash c++11 segmentation-fault c++
1 Answers

You are trying to do lock based concurrency.

Lock based concurrency doesn't compose. Locally correct operations, when concatenated, generate garbage.

Here, the problem is your it is generated in one lock, then you unlock. During that unlock, the it may or may not be valid. Then you relock and presume it is still valid.

You'll either have to lock the entire period you are working on the map, then unlock it and forget almost everything you knew in the lock (no carrying iterators out), rewrite your code based on immutable data structures, or do something else even more radical.

{
  std::lock_guard<std::recursive_mutex> lock(mutex_);
  it = subs_.begin();
}

here you are locking, doing an operation, then ... unlocking the mutex at the end of the scope.

So it is an iterator into the map, and you aren't stopping other threads from editing the map while you hold it. This could invalidate it.

The same thing happens in your for(;;) loop; you lock at the start of each iteration, and then unlock at the end. The map element that it refers to could be deleted, invalidating your it, between one iteration and the next.

Here is a simple utility class:

template<class T>
struct mutex_guarded {
  template<class F>
  auto read(F&& f)const {
    auto l = std::unique_lock<std::mutex>(m);
    return f(t);
  }
  template<class F>
  auto write(F&& f) {
    auto l = std::unique_lock<std::mutex>(m);
    return f(t);
  }
private:
  mutable std::mutex m;
  T t;
};

now, change your std::map<int, SubscData> to mutex_guarded<std::map<int, SubscData>>.

Next:

void SubscManag::handleNotif(Notif notif)
{
  subs_.write([&](auto& subs_) {
    std::cout << "Current subscribe size: " << subs_.size();
    auto it = subs_.begin();
    for (;;)
    {
        if (it == subs_.end())
        {
            break;
        }
        if (it->second.hasToBeErased)
        {
            std::cout << "Erase id : " << it->first;
            it = subs_.erase(it);
            continue;
        }
        if (cond)
        {
            // ....
            ++it;
            continue;
        }
        // ....
        ++it;
      }
  });
}

and you'll have the mutex locked while accessing subs_.

Note that accessing subs_ through other methods remains dangerous. Guarding individual method calls with a mutex is a bad plan. Making "this object can be accessed from any thread" is a recipe for disaster in 999/1000 cases, often delayed, with the initial small case working, then it exploding in deadlocks and race conditions long after the code was written.

mutex_guarded isn't the best solution, but it is better than per-method locks.

user_17746670
Related