From 935bb36fe46da0fb99c36484045a6de137c071ae Mon Sep 17 00:00:00 2001 From: Norman Feske Date: Mon, 8 Mar 2021 17:14:09 +0100 Subject: [PATCH] base: fix child destruction while close requested This patch fixes a corner case where a child is destructed while a asynchronous close request to a sibling server is still pending. The child immediately discarded the session ID as the end of the close-session processing, assuming that this ID is never to be needed again. The session-state continues to exist to handle asynchrous close protocol with the server. However, if the child is destructed at this point (before the server responded to the session request), the destruction of the child would not cover the discharging of the session state because the session state was no longer be part of the client's ID space. So once the asynchronous close response from the server came in, the session state contained stale information, in particular a stale closed_callback pointer. The patch fixes the problem by deferring the discarding of the client ID to the point where the session state is actually destructed. So the session of a pending close response is covered by the child destructor. Thanks to Pirmin Duss for reporting this issue along with a test scenario for reproducing it! Fixes #4039 --- repos/base/src/lib/base/child.cc | 7 +- .../os/recipes/raw/test-init/test-init.config | 83 +++++++++++++++++++ repos/os/src/app/dummy/main.cc | 57 +++++++++---- 3 files changed, 131 insertions(+), 16 deletions(-) diff --git a/repos/base/src/lib/base/child.cc b/repos/base/src/lib/base/child.cc index 8c80318fbd..adadfc05cc 100644 --- a/repos/base/src/lib/base/child.cc +++ b/repos/base/src/lib/base/child.cc @@ -467,7 +467,6 @@ Child::Close_result Child::_close(Session_state &session) _policy.session_state_changed(); - session.discard_id_at_client(); session.service().wakeup(); return CLOSE_PENDING; @@ -926,7 +925,11 @@ void Child::close_all_sessions() auto close_fn = [&] (Session_state &session) { session.closed_callback = nullptr; session.ready_callback = nullptr; - (void)_close(session); + + Close_result const close_result = _close(session); + + if (close_result == CLOSE_PENDING) + session.discard_id_at_client(); }; while (_id_space.apply_any(close_fn)); diff --git a/repos/os/recipes/raw/test-init/test-init.config b/repos/os/recipes/raw/test-init/test-init.config index faac1a84c9..98e6ebf156 100644 --- a/repos/os/recipes/raw/test-init/test-init.config +++ b/repos/os/recipes/raw/test-init/test-init.config @@ -1660,6 +1660,89 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/repos/os/src/app/dummy/main.cc b/repos/os/src/app/dummy/main.cc index 145e169c59..17cfb6c84e 100644 --- a/repos/os/src/app/dummy/main.cc +++ b/repos/os/src/app/dummy/main.cc @@ -39,25 +39,31 @@ struct Dummy::Log_service Sliced_heap _heap { _env.ram(), _env.rm() }; - bool const _verbose; + struct Args + { + bool verbose; + unsigned delay_close_ms; + }; + + Args const _args; struct Session_component : Rpc_object { Session_label const _label; - bool const _verbose; + Args const _args; - Session_component(Session_label const &label, bool verbose) + Session_component(Session_label const &label, Args args) : - _label(label), _verbose(verbose) + _label(label), _args(args) { - if (_verbose) + if (_args.verbose) log("opening session with label ", _label); } ~Session_component() { - if (_verbose) + if (_args.verbose) log("closing session with label ", _label); } @@ -78,16 +84,22 @@ struct Dummy::Log_service { Pd_session &_pd; - bool const _verbose; + Log_service::Args const _service_args; - Root(Entrypoint &ep, Allocator &alloc, Pd_session &pd, bool verbose) + Constructible _timer { }; + + Root(Env &env, Allocator &alloc, Pd_session &pd, Log_service::Args args) : - Root_component(ep, alloc), _pd(pd), _verbose(verbose) - { } + Root_component(env.ep(), alloc), _pd(pd), _service_args(args) + { + if (args.delay_close_ms) + _timer.construct(env); + } Session_component *_create_session(const char *args, Affinity const &) override { - return new (md_alloc()) Session_component(label_from_args(args), _verbose); + return new (md_alloc()) + Session_component(label_from_args(args), _service_args); } void _upgrade_session(Session_component *, const char *args) override @@ -98,11 +110,25 @@ struct Dummy::Log_service if (_pd.avail_ram().value >= ram_quota) log("received session quota upgrade"); } + + void _destroy_session(Session_component *session) override + { + if (_timer.constructed()) { + log("delay close by ", _service_args.delay_close_ms, " ms"); + _timer->msleep(_service_args.delay_close_ms); + } + else + { + log("close session without delay"); + } + + Root_component::_destroy_session(session); + } }; - Root _root { _env.ep(), _heap, _env.pd(), _verbose }; + Root _root { _env, _heap, _env.pd(), _args }; - Log_service(Env &env, bool verbose) : _env(env), _verbose(verbose) + Log_service(Env &env, Args args) : _env(env), _args(args) { _env.parent().announce(_env.ep().manage(_root)); log("created LOG service"); @@ -311,7 +337,10 @@ struct Dummy::Main _log_connections.destruct(); if (node.type() == "log_service") - _log_service.construct(_env, node.attribute_value("verbose", false)); + _log_service.construct(_env, Log_service::Args { + .verbose = node.attribute_value("verbose", false), + .delay_close_ms = node.attribute_value("delay_close_ms", 0U) + }); if (node.type() == "consume_ram") _ram_consumer.consume(node.attribute_value("amount", Number_of_bytes()));