From b585583ec7afebd8b2fdbc2ea318f955570c1333 Mon Sep 17 00:00:00 2001 From: Stefan Kalkowski Date: Wed, 23 Sep 2015 08:37:12 +0200 Subject: [PATCH] core: do not destroy in object pool's apply scope Destroying an object within the scope of a lambda/functor executed in the object pool's apply function leads potentially to memory corruption. Within the scope the corresponding object is locked and unlocked when leaving the scope. Therefore, it is illegal to free the object's memory meanwhile. This commit eliminates several places in core that destroyed wrongly in the object pool's scope. Fix #1713 --- .../src/core/cap_session_component.cc | 6 ++- .../src/core/include/cpu_session_component.h | 2 +- .../src/core/signal_session_component.cc | 16 +++--- .../src/core/include/cpu_session_component.h | 2 +- .../src/core/include/cpu_session_component.h | 2 +- .../src/core/signal_session_component.cc | 26 +++++----- repos/base/src/core/cpu_session_component.cc | 25 +++++---- .../src/core/include/cpu_session_component.h | 2 +- .../src/core/include/ram_session_component.h | 2 +- repos/base/src/core/ram_session_component.cc | 52 +++++++++---------- .../base/src/core/signal_session_component.cc | 6 ++- repos/dde_linux/src/lib/wifi/pci_driver.cc | 6 +-- .../platform/spec/x86/pci_session_component.h | 16 +++--- 13 files changed, 90 insertions(+), 73 deletions(-) diff --git a/repos/base-foc/src/core/cap_session_component.cc b/repos/base-foc/src/core/cap_session_component.cc index 69d14ef96..e2a210044 100644 --- a/repos/base-foc/src/core/cap_session_component.cc +++ b/repos/base-foc/src/core/cap_session_component.cc @@ -153,13 +153,15 @@ void Cap_session_component::free(Native_capability cap) /* proof whether the capability was created by this cap_session */ if (static_cast(cap.idx())->session() != this) return; - _pool.apply(cap, [this] (Entry *e) { + Entry * entry; + _pool.apply(cap, [&] (Entry *e) { + entry = e; if (e) { _pool.remove(e); - destroy(_md_alloc, e); } else PWRN("Could not find capability to be deleted"); }); + if (entry) destroy(_md_alloc, entry); } diff --git a/repos/base-foc/src/core/include/cpu_session_component.h b/repos/base-foc/src/core/include/cpu_session_component.h index 06b097e9f..9c3ceda0b 100644 --- a/repos/base-foc/src/core/include/cpu_session_component.h +++ b/repos/base-foc/src/core/include/cpu_session_component.h @@ -190,7 +190,7 @@ namespace Genode { * by themselves and call this function to perform the actual * killing. */ - void _unsynchronized_kill_thread(Cpu_thread_component *thread); + void _unsynchronized_kill_thread(Thread_capability cap); public: diff --git a/repos/base-hw/src/core/signal_session_component.cc b/repos/base-hw/src/core/signal_session_component.cc index 1cb4c7ae4..f53f6432e 100644 --- a/repos/base-hw/src/core/signal_session_component.cc +++ b/repos/base-hw/src/core/signal_session_component.cc @@ -52,16 +52,18 @@ Signal_receiver_capability Signal_session_component::alloc_receiver() void Signal_session_component::free_receiver(Signal_receiver_capability cap) { /* look up ressource info */ + Receiver * receiver; auto lambda = [&] (Receiver *r) { - if (!r) { + receiver = r; + if (!receiver) { PERR("unknown signal receiver"); throw Kill_receiver_failed(); } /* release resources */ - _receivers.remove(r); - destroy(&_receivers_slab, r); + _receivers.remove(receiver); }; _receivers.apply(cap, lambda); + destroy(&_receivers_slab, receiver); } @@ -93,16 +95,18 @@ Signal_session_component::alloc_context(Signal_receiver_capability src, void Signal_session_component::free_context(Signal_context_capability cap) { /* look up ressource info */ + Context * context; auto lambda = [&] (Context *c) { - if (!c) { + context = c; + if (!context) { PERR("unknown signal context"); throw Kill_context_failed(); } /* release resources */ - _contexts.remove(c); - destroy(&_contexts_slab, c); + _contexts.remove(context); }; _contexts.apply(cap, lambda); + destroy(&_contexts_slab, context); } diff --git a/repos/base-linux/src/core/include/cpu_session_component.h b/repos/base-linux/src/core/include/cpu_session_component.h index ee57872ed..99854c9fd 100644 --- a/repos/base-linux/src/core/include/cpu_session_component.h +++ b/repos/base-linux/src/core/include/cpu_session_component.h @@ -183,7 +183,7 @@ namespace Genode { * by themselves and call this function to perform the actual * killing. */ - void _unsynchronized_kill_thread(Cpu_thread_component *thread); + void _unsynchronized_kill_thread(Thread_capability cap); public: diff --git a/repos/base-nova/src/core/include/cpu_session_component.h b/repos/base-nova/src/core/include/cpu_session_component.h index 1ea59c140..21b45a287 100644 --- a/repos/base-nova/src/core/include/cpu_session_component.h +++ b/repos/base-nova/src/core/include/cpu_session_component.h @@ -194,7 +194,7 @@ namespace Genode { * by themselves and call this function to perform the actual * killing. */ - void _unsynchronized_kill_thread(Cpu_thread_component *thread); + void _unsynchronized_kill_thread(Thread_capability cap); public: diff --git a/repos/base-nova/src/core/signal_session_component.cc b/repos/base-nova/src/core/signal_session_component.cc index 23e48513e..9883e854c 100644 --- a/repos/base-nova/src/core/signal_session_component.cc +++ b/repos/base-nova/src/core/signal_session_component.cc @@ -87,20 +87,22 @@ Signal_context_capability Signal_session_component::alloc_context(long imprint) void Signal_session_component::free_context(Signal_context_capability context_cap) { - auto lambda = [&] (Signal_context_component *context) { - if (!context) { - PWRN("%p - specified signal-context capability has wrong type %lx", - this, context_cap.local_name()); - return; - } - - _signal_queue.remove(context); - destroy(&_contexts_slab, context); - - Nova::revoke(Nova::Obj_crd(context_cap.local_name(), 0)); - cap_map()->remove(context_cap.local_name(), 0); + Signal_context_component *context; + auto lambda = [&] (Signal_context_component *c) { + context = c; + if (context) _signal_queue.remove(context); }; _signal_queue.apply(context_cap, lambda); + + if (!context) { + PWRN("%p - specified signal-context capability has wrong type %lx", + this, context_cap.local_name()); + return; + } + destroy(&_contexts_slab, context); + + Nova::revoke(Nova::Obj_crd(context_cap.local_name(), 0)); + cap_map()->remove(context_cap.local_name(), 0); } diff --git a/repos/base/src/core/cpu_session_component.cc b/repos/base/src/core/cpu_session_component.cc index ea5a89528..4f25a9fbc 100644 --- a/repos/base/src/core/cpu_session_component.cc +++ b/repos/base/src/core/cpu_session_component.cc @@ -83,9 +83,14 @@ Thread_capability Cpu_session_component::create_thread(size_t weight, } -void Cpu_session_component::_unsynchronized_kill_thread(Cpu_thread_component *thread) +void Cpu_session_component::_unsynchronized_kill_thread(Thread_capability thread_cap) { - _thread_ep->dissolve(thread); + Cpu_thread_component *thread; + _thread_ep->apply(thread_cap, [&] (Cpu_thread_component *t) { + if ((thread = t)) _thread_ep->dissolve(thread); }); + + if (!thread) return; + _thread_list.remove(thread); _trace_sources.remove(thread->trace_source()); @@ -94,8 +99,10 @@ void Cpu_session_component::_unsynchronized_kill_thread(Cpu_thread_component *th _decr_weight(thread->weight()); - Lock::Guard lock_guard(_thread_alloc_lock); - destroy(&_thread_alloc, thread); + { + Lock::Guard lock_guard(_thread_alloc_lock); + destroy(&_thread_alloc, thread); + } _trace_control_area.free(trace_control_index); } @@ -103,13 +110,9 @@ void Cpu_session_component::_unsynchronized_kill_thread(Cpu_thread_component *th void Cpu_session_component::kill_thread(Thread_capability thread_cap) { - auto lambda = [this] (Cpu_thread_component *thread) { - if (!thread) return; + Lock::Guard lock_guard(_thread_list_lock); - Lock::Guard lock_guard(_thread_list_lock); - _unsynchronized_kill_thread(thread); - }; - _thread_ep->apply(thread_cap, lambda); + _unsynchronized_kill_thread(thread_cap); } @@ -469,7 +472,7 @@ void Cpu_session_component::_deinit_threads() */ for (Cpu_thread_component *thread; (thread = _thread_list.first()); ) - _unsynchronized_kill_thread(thread); + _unsynchronized_kill_thread(thread->cap()); } diff --git a/repos/base/src/core/include/cpu_session_component.h b/repos/base/src/core/include/cpu_session_component.h index 63b7c37f3..7e4ea6364 100644 --- a/repos/base/src/core/include/cpu_session_component.h +++ b/repos/base/src/core/include/cpu_session_component.h @@ -230,7 +230,7 @@ namespace Genode { * by themselves and call this function to perform the actual * killing. */ - void _unsynchronized_kill_thread(Cpu_thread_component *thread); + void _unsynchronized_kill_thread(Thread_capability cap); public: diff --git a/repos/base/src/core/include/ram_session_component.h b/repos/base/src/core/include/ram_session_component.h index ef250159c..82eb3ad8a 100644 --- a/repos/base/src/core/include/ram_session_component.h +++ b/repos/base/src/core/include/ram_session_component.h @@ -81,7 +81,7 @@ namespace Genode { /** * Free dataspace */ - void _free_ds(Dataspace_component *ds); + void _free_ds(Dataspace_capability ds_cap); /** * Transfer quota to another RAM session diff --git a/repos/base/src/core/ram_session_component.cc b/repos/base/src/core/ram_session_component.cc index 1ae4effee..2ef658fb0 100644 --- a/repos/base/src/core/ram_session_component.cc +++ b/repos/base/src/core/ram_session_component.cc @@ -35,31 +35,37 @@ addr_t Ram_session_component::phys_addr(Ram_dataspace_capability ds) } -void Ram_session_component::_free_ds(Dataspace_component *ds) +void Ram_session_component::_free_ds(Dataspace_capability ds_cap) { - if (!ds) return; - if (!ds->owner(this)) return; + Dataspace_component *ds; + _ds_ep->apply(ds_cap, [&] (Dataspace_component *c) + { + ds = c; - size_t ds_size = ds->size(); + if (!ds) return; + if (!ds->owner(this)) return; - /* tell entry point to forget the dataspace */ - _ds_ep->dissolve(ds); + size_t ds_size = ds->size(); - /* remove dataspace from all RM sessions */ - ds->detach_from_rm_sessions(); + /* tell entry point to forget the dataspace */ + _ds_ep->dissolve(ds); - /* destroy native shared memory representation */ - _revoke_ram_ds(ds); + /* remove dataspace from all RM sessions */ + ds->detach_from_rm_sessions(); - /* free physical memory that was backing the dataspace */ - _ram_alloc->free((void *)ds->phys_addr(), ds_size); + /* destroy native shared memory representation */ + _revoke_ram_ds(ds); + + /* free physical memory that was backing the dataspace */ + _ram_alloc->free((void *)ds->phys_addr(), ds_size); + + /* adjust payload */ + Lock::Guard lock_guard(_ref_members_lock); + _payload -= ds_size; + }); /* call dataspace destructors and free memory */ destroy(&_ds_slab, ds); - - /* adjust payload */ - Lock::Guard lock_guard(_ref_members_lock); - _payload -= ds_size; } @@ -218,15 +224,8 @@ Ram_dataspace_capability Ram_session_component::alloc(size_t ds_size, Cache_attr } -void Ram_session_component::free(Ram_dataspace_capability ds_cap) -{ - auto lambda = [this] (Dataspace_component *ds) { - if (!ds) return; - _free_ds(ds); - }; - - _ds_ep->apply(ds_cap, lambda); -} +void Ram_session_component::free(Ram_dataspace_capability ds_cap) { + _free_ds(ds_cap); } int Ram_session_component::ref_account(Ram_session_capability ram_session_cap) @@ -291,7 +290,8 @@ Ram_session_component::Ram_session_component(Rpc_entrypoint *ds_ep, Ram_session_component::~Ram_session_component() { /* destroy all dataspaces */ - for (Dataspace_component *ds; (ds = _ds_slab()->first_object()); _free_ds(ds)); + for (Dataspace_component *ds; (ds = _ds_slab()->first_object()); + _free_ds(ds->cap())); if (_payload != 0) PWRN("Remaining payload of %zu in ram session to destroy", _payload); diff --git a/repos/base/src/core/signal_session_component.cc b/repos/base/src/core/signal_session_component.cc index cf82e0f08..645c971a9 100644 --- a/repos/base/src/core/signal_session_component.cc +++ b/repos/base/src/core/signal_session_component.cc @@ -71,15 +71,17 @@ Signal_context_capability Signal_session_component::alloc_context(long imprint) void Signal_session_component::free_context(Signal_context_capability context_cap) { - _context_ep->apply(context_cap, [this] (Signal_context_component *context) { + Signal_context_component *context; + _context_ep->apply(context_cap, [&] (Signal_context_component *c) { + context = c; if (!context) { PWRN("specified signal-context capability has wrong type"); return; } _context_ep->dissolve(context); - destroy(&_contexts_slab, context); }); + destroy(&_contexts_slab, context); } diff --git a/repos/dde_linux/src/lib/wifi/pci_driver.cc b/repos/dde_linux/src/lib/wifi/pci_driver.cc index 7c44aa346..a10ef26b8 100644 --- a/repos/dde_linux/src/lib/wifi/pci_driver.cc +++ b/repos/dde_linux/src/lib/wifi/pci_driver.cc @@ -484,10 +484,10 @@ void Lx::backend_free(Genode::Ram_dataspace_capability cap) auto lambda = [&] (Memory_object_base *o) { object = o; if (object) { - o->free(); - memory_pool.remove(o); + object->free(); + memory_pool.remove(object); } }; memory_pool.apply(cap, lambda); - destroy(env()->heap(), object); + if (object) destroy(env()->heap(), object); } diff --git a/repos/os/src/drivers/platform/spec/x86/pci_session_component.h b/repos/os/src/drivers/platform/spec/x86/pci_session_component.h index 0845f5d96..e3a790463 100644 --- a/repos/os/src/drivers/platform/spec/x86/pci_session_component.h +++ b/repos/os/src/drivers/platform/spec/x86/pci_session_component.h @@ -559,8 +559,10 @@ namespace Platform { void release_device(Device_capability device_cap) { - auto lambda = [&] (Device_component *device) + Device_component * device; + auto lambda = [&] (Device_component *d) { + device = d; if (!device) return; @@ -573,15 +575,17 @@ namespace Platform { _device_list.remove(device); _ep->dissolve(device); - - if (device->config().valid()) - destroy(_device_slab, device); - else - destroy(_md_alloc, device); }; /* lookup device component for previous device */ _ep->apply(device_cap, lambda); + + if (!device) return; + + if (device->config().valid()) + destroy(_device_slab, device); + else + destroy(_md_alloc, device); } Genode::Io_mem_dataspace_capability assign_device(Device_component * device)