ExternalVendorCode/genode
1
0
Fork 0
mirror of https://github.com/genodelabs/genode.git synced 2025-02-07 11:50:24 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

base: handle race in Genode::Registry class

Browse Source 
The race may happen when element objects get destructed by another thread then
the thread handling the for_each loop. In this case it may happen that the
object is already destructed (left the ~Element destructor) but the thread
handling the loop touches the invalid memory afterwards (the Element lock).

detected during issue #2299

Fixes #2320
This commit is contained in:
Alexander Boettcher 2017-03-12 00:15:54 +01:00 committed by Christian Helmuth
parent 6d6474ba0e
commit 391339a4bb
2 changed files with 33 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
repos/base/include/base/registry.h
View File

@ -29,7 +29,12 @@ class Genode::Registry_base
{ {
private: private:
enum Keep { KEEP, DISCARD }; struct Notify {
enum Keep { KEEP, DISCARD } keep;
void * thread;
Notify(Keep k, void *t) : keep(k), thread(t) { }
};
protected: protected:
@ -49,7 +54,7 @@ class Genode::Registry_base
/* /*
* Assigned by 'Registry::_for_each' * Assigned by 'Registry::_for_each'
*/ */
Keep *_keep_ptr = nullptr; Notify *_notify_ptr = nullptr;
protected: protected:
@ -77,7 +82,7 @@ class Genode::Registry_base
void _insert(Element &); void _insert(Element &);
void _remove(Element &); void _remove(Element &);
Element *_processed(Keep, List<Element> &, Element &, Element *); Element *_processed(Notify &, List<Element> &, Element &, Element *);
protected: protected:

38
repos/base/src/lib/base/registry.cc
View File

@ -12,6 +12,7 @@
*/ */
#include <base/registry.h> #include <base/registry.h>
#include <base/thread.h>
using namespace Genode; using namespace Genode;
@ -28,15 +29,26 @@ Registry_base::Element::~Element()
{ {
{ {
Lock::Guard guard(_lock); Lock::Guard guard(_lock);
if (_keep_ptr && _registry._curr == this) { if (_notify_ptr && _registry._curr == this) {
/* /*
* The destructor is called from the functor of a * The destructor is called from the functor of a
* 'Registry::for_each' loop with the element temporarily dequeued. * 'Registry::for_each' loop with the element temporarily dequeued.
* We flag the element to be not re-inserted into the list. * We flag the element to be not re-inserted into the list.
*/ */
*_keep_ptr = DISCARD; _notify_ptr->keep = Notify::Keep::DISCARD;
return;
/* done if and only if running in the context of same thread */
if (Thread::myself() == _notify_ptr->thread)
return;
/*
* We synchronize on the _lock of the _registry, by invoking
* the _remove method below. This ensures that the object leaves
* the destructor not before the registry lost the pointer to this
* object. The actual removal attempt will be ignored by the list
* implementation, since the current object was removed already.
*/
} }
} }
_registry._remove(*this); _registry._remove(*this);
@ -59,31 +71,31 @@ void Registry_base::_remove(Element &element)
} }
Registry_base::Element *Registry_base::_processed(Keep const keep, Registry_base::Element *Registry_base::_processed(Notify &notify,
List<Element> &processed, List<Element> &processed,
Element &e, Element *at) Element &e, Element *at)
{ {
_curr = nullptr; _curr = nullptr;
/* if 'e' was dropped from the list, keep the current re-insert position */ /* if 'e' was dropped from the list, keep the current re-insert position */
if (keep == DISCARD) if (notify.keep == Notify::Keep::DISCARD)
return at; return at;
/* make sure that the critical section of '~Element' is completed */ /* make sure that the critical section of '~Element' is completed */
Lock::Guard guard(e._lock); Lock::Guard guard(e._lock);
/* here we know that 'e' still exists */
e._notify_ptr = nullptr;
/* /*
* If '~Element' was preempted between the condition check and the * If '~Element' was preempted between the condition check and the
* assignment of keep = DISCARD, the above check would miss the DISCARD * assignment of keep = DISCARD, the above check would miss the DISCARD
* flag. Now, with the acquired lock, we know that the 'keep' value is * flag. Now, with the acquired lock, we know that the 'keep' value is
* up to date. * up to date.
*/ */
if (keep == DISCARD) if (notify.keep == Notify::Keep::DISCARD)
return at; return at;
/* here we know that 'e' still exists */
e._keep_ptr = nullptr;
/* insert 'e' into the list of processed elements */ /* insert 'e' into the list of processed elements */
processed.insert(&e, at); processed.insert(&e, at);
@ -103,12 +115,12 @@ void Registry_base::_for_each(Untyped_functor &functor)
while (Element *e = _elements.first()) { while (Element *e = _elements.first()) {
Keep keep = KEEP; Notify notify(Notify::Keep::KEEP, Thread::myself());
{ {
/* tell the element where to report its status */ /* tell the element where to report its status */
Lock::Guard guard(e->_lock); Lock::Guard guard(e->_lock);
_curr = e; _curr = e;
e->_keep_ptr = &keep; e->_notify_ptr = &notify;
} }
/* /*
@ -122,9 +134,9 @@ void Registry_base::_for_each(Untyped_functor &functor)
try { functor.call(e->_obj); } try { functor.call(e->_obj); }
/* propagate exceptions while keeping registry consistent */ /* propagate exceptions while keeping registry consistent */
catch (...) { at = _processed(keep, processed, *e, at); throw; } catch (...) { at = _processed(notify, processed, *e, at); throw; }
at = _processed(keep, processed, *e, at); at = _processed(notify, processed, *e, at);
} }
/* use list of processed elements as '_elements' list */ /* use list of processed elements as '_elements' list */