From 8e7aa5449335ffb0ab1ca091d9139e5cf0bccef8 Mon Sep 17 00:00:00 2001
From: Norman Feske <norman.feske@genode-labs.com>
Date: Wed, 12 Apr 2017 11:26:31 +0200
Subject: [PATCH] base: drop session states of vanished clients

For asynchronously provided sessions, the parent has to maintain the
session state as long as the server hasn't explicitly responded to a
close request. For this reason, the lifetime of such session states is
bound to the server, not the client.

When the server responds to a close request, the session state gets
freed. The 'session_response' implementation does not immediately
destroy the session state but delegates the destruction to a client-side
callback, which thereby also notifies the client. However, the code did
not consider the case where the client has completely vanished at
session-response time. In this case, we need to drop the session state
immediately.

Fixes #2391
---
 repos/base/src/lib/base/child.cc |  9 ++++++
 repos/os/run/init.run            | 54 ++++++++++++++++++++++++++++++--
 repos/os/src/test/init/main.cc   |  3 ++
 3 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/repos/base/src/lib/base/child.cc b/repos/base/src/lib/base/child.cc
index cc8daf4ee0..c16d43a6de 100644
--- a/repos/base/src/lib/base/child.cc
+++ b/repos/base/src/lib/base/child.cc
@@ -533,8 +533,17 @@ void Child::session_response(Server::Id id, Session_response response)
 
 			case Parent::SESSION_CLOSED:
 				session.phase = Session_state::CLOSED;
+
+				/*
+				 * If the client exists, reflect the response to the client
+				 * via the 'closed_callback'. If the client has vanished,
+				 * i.e., if the close request was issued by ourself while
+				 * killing a child, we drop the session state immediately.
+				 */
 				if (session.closed_callback)
 					session.closed_callback->session_closed(session);
+				else
+					_revert_quota_and_destroy(session);
 				break;
 
 			case Parent::INVALID_ARGS:
diff --git a/repos/os/run/init.run b/repos/os/run/init.run
index e0de4e040c..57aaa063f1 100644
--- a/repos/os/run/init.run
+++ b/repos/os/run/init.run
@@ -92,6 +92,12 @@ append config {
 
 			<message string="routing to custom log service"/>
 
+			<!-- We let the client manually create a LOG session in addition
+			     to its env LOG session. So there are two sessions at the
+			     server, one initiated by init and one initiated by the
+			     child. Both cases trigger distinct code paths at child-
+			     destruction time. -->
+
 			<init_config version="chained log services">
 				<report ids="yes" requested="yes" provided="yes"/>
 				<parent-provides>
@@ -121,7 +127,10 @@ append config {
 				<start name="client">
 					<binary name="dummy"/>
 					<resource name="RAM" quantum="1M"/>
-					<config> <log string="client started"/> </config>
+					<config>
+						<log string="client started"/>
+						<create_log_connections count="1"/>
+					</config>
 					<route>
 						<service name="LOG"> <child name="indirect_server"/> </service>
 						<any-service> <parent/> </any-service>
@@ -177,7 +186,10 @@ append config {
 				<start name="client">
 					<binary name="dummy"/>
 					<resource name="RAM" quantum="1M"/>
-					<config> <log string="client started"/> </config>
+					<config>
+						<log string="client started"/>
+						<create_log_connections count="1"/>
+					</config>
 					<route>
 						<service name="LOG"> <child name="indirect_server"/> </service>
 						<any-service> <parent/> </any-service>
@@ -199,6 +211,44 @@ append config {
 			</expect_init_state>
 			<sleep ms="100"/>
 
+			<!-- Kill the client and validate that all session states are freed -->
+
+			<init_config version="client removed">
+				<report ids="yes" requested="yes" provided="yes"/>
+				<parent-provides>
+					<service name="ROM"/>
+					<service name="RAM"/>
+					<service name="CPU"/>
+					<service name="PD"/>
+					<service name="LOG"/>
+				</parent-provides>
+				<start name="server">
+					<binary name="dummy"/>
+					<resource name="RAM" quantum="1M"/>
+					<provides> <service name="LOG"/> </provides>
+					<config> <log_service/> </config>
+					<route> <any-service> <parent/> </any-service> </route>
+				</start>
+				<start name="indirect_server">
+					<binary name="dummy"/>
+					<resource name="RAM" quantum="1M"/>
+					<provides> <service name="LOG"/> </provides>
+					<config> <log_service/> </config>
+					<route> <any-service> <parent/> </any-service> </route>
+				</start>
+			</init_config>
+			<sleep ms="150"/>
+			<expect_init_state>
+				<node name="child">
+					<attribute name="name" value="indirect_server"/>
+					<node name="provided">
+						<not>
+							<node name="session"/>
+						</not>
+					</node>
+				</node>
+			</expect_init_state>
+
 
 			<message string="test changing provided services"/>
 
diff --git a/repos/os/src/test/init/main.cc b/repos/os/src/test/init/main.cc
index 83f6c1319e..162fc9a4de 100644
--- a/repos/os/src/test/init/main.cc
+++ b/repos/os/src/test/init/main.cc
@@ -91,6 +91,9 @@ static inline bool Test::xml_matches(Xml_node expected, Xml_node node)
 
 			matches = matches && at_least_one_sub_node_matches;
 		}
+
+		if (condition.type() == "not")
+			matches = matches && !xml_matches(condition, node);
 	});
 	return matches;
 }