[jira] [Created] (KUDU-1719) Heap use-after-free and deadlock on Messenger::Init() failure

["Sailesh Mukil (JIRA)" <ji...@apache.org>]

Sailesh Mukil created KUDU-1719:
-----------------------------------

             Summary: Heap use-after-free and deadlock on Messenger::Init() failure
                 Key: KUDU-1719
                 URL: https://issues.apache.org/jira/browse/KUDU-1719
             Project: Kudu
          Issue Type: Bug
    Affects Versions: 1.0.1
            Reporter: Sailesh Mukil


{code}
Status MessengerBuilder::Build(shared_ptr<Messenger> *msgr) {
  ...
  gscoped_ptr<Messenger> new_msgr(new Messenger(*this));
  Status build_status = new_msgr->Init();
  if (!build_status.ok()) {
    new_msgr->AllExternalReferencesDropped();
    return build_status;
  }
  ...
}
{code}

When new_msgr->Init() fails, AllExternalReferencesDropped() will be called which does a retain_self_.reset() which is a shared pointer to itself. If that was the last shared pointer reference to itself, it will call the destructor ~Messenger(). However, when the gscoped pointer 'new_msgr' goes out of scope, it too will call the destructor, but by this point, all the members have already been freed causing a use-after-free.

----

When Messenger::Init() fails before all the reactors have been Init()'ed, the reactors that haven't been Init()'ed do not give up their reference to the Messenger object. This is because this reference is only given up in ReactorThread::RunThread(), which is not called unless reactor->Init() is called:
{code}
void ReactorThread::RunThread() {
  ...
  DVLOG(6) << "Calling ReactorThread::RunThread()...";
  loop_.run(0);
  VLOG(1) << name() << " thread exiting.";

  // No longer need the messenger. This causes the messenger to
  // get deleted when all the reactors exit.
  reactor_->messenger_.reset();
}
{code}

So when the gscoped_ptr 'new_msgr' goes out of scope, it will call the destructor which calls STLDeleteElements(&reactors_); where the Messenger references are finally dropped. But when the last Messenger reference is dropped, that too will call the destructor ~Messenger() which gets stuck on the lock:
{code}
Messenger::~Messenger() {
  std::lock_guard<percpu_rwlock> guard(lock_);  // Gets stuck on this lock
  CHECK(closing_) << "Should have already shut down";
  STLDeleteElements(&reactors_); // This will call the ~Messenger() destructor again
}
{code}

The easy fix for this is to make the gscoped_ptr 'new_msgr' a raw pointer instead, and not explicitly free it since it will be freed by the shared pointers pointing to it. (But this may look like buggy code, not sure if there's a better way)



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)