From 9093c293cbc6aa683f80e39e3af2924a1f86b62e Mon Sep 17 00:00:00 2001
From: Norman Feske <norman.feske@genode-labs.com>
Date: Mon, 8 Mar 2021 19:10:16 +0100
Subject: [PATCH] sandbox: destroy 'Session_state' of local services

This patch adds the missing destruction of session-state objects of
local services when closing a session. Because of the missing
destruction, those session-state object remained part of the server
ID space. This becomes a problem once the backing store of the session
state object vanishes, that is when the client child gets removed from
the sandbox. Hence, the removal of a child with an open session to a
local service would lead to the corruption of the server ID space.

This patch adds the missing session.destroy() call.

Fixes #4044
---
 repos/os/run/sandbox.run            | 15 +++++++---
 repos/os/src/lib/sandbox/library.cc |  6 ++--
 repos/os/src/test/sandbox/main.cc   | 46 ++++++++++++++++++++++-------
 3 files changed, 49 insertions(+), 18 deletions(-)

diff --git a/repos/os/run/sandbox.run b/repos/os/run/sandbox.run
index e802d4c3df..930231e6c6 100644
--- a/repos/os/run/sandbox.run
+++ b/repos/os/run/sandbox.run
@@ -1,4 +1,4 @@
-build "core init test/sandbox app/dummy"
+build "core init timer test/sandbox app/dummy"
 
 create_boot_directory
 
@@ -9,19 +9,26 @@ install_config {
 			<service name="PD"/>
 			<service name="CPU"/>
 			<service name="ROM"/>
+			<service name="IRQ"/>
+			<service name="IO_MEM"/>
+			<service name="IO_PORT"/>
 		</parent-provides>
 		<default-route>
-			<any-service> <parent/> </any-service>
+			<any-service> <parent/> <any-child/> </any-service>
 		</default-route>
 		<default caps="500"/>
+		<start name="timer">
+			<resource name="RAM" quantum="1M"/>
+			<provides> <service name="Timer"/> </provides>
+		</start>
 		<start name="test-sandbox">
 			<resource name="RAM" quantum="10M"/>
 		</start>
 	</config>
 }
 
-build_boot_image { core ld.lib.so init test-sandbox sandbox.lib.so dummy }
+build_boot_image { core ld.lib.so init timer test-sandbox sandbox.lib.so dummy }
 
 append qemu_args " -nographic "
 
-run_genode_until "child \"dummy\" exited with exit value 0.*\n" 20
+run_genode_until "version=\"5\".*\n" 20
diff --git a/repos/os/src/lib/sandbox/library.cc b/repos/os/src/lib/sandbox/library.cc
index 28b60c491f..2c5ba133b9 100644
--- a/repos/os/src/lib/sandbox/library.cc
+++ b/repos/os/src/lib/sandbox/library.cc
@@ -574,9 +574,7 @@ void Genode::Sandbox::Local_service_base::_for_each_session_to_close(Close_fn &c
 		case Close_response::CLOSED:
 
 			session.phase = Session_state::CLOSED;
-
-			if (session.closed_callback)
-				session.id_at_parent.construct(session, pending_callbacks);
+			session.id_at_parent.construct(session, pending_callbacks);
 			break;
 
 		case Close_response::DEFERRED:
@@ -593,6 +591,8 @@ void Genode::Sandbox::Local_service_base::_for_each_session_to_close(Close_fn &c
 
 		if (session.closed_callback)
 			session.closed_callback->session_closed(session);
+		else
+			session.destroy();
 	}));
 }
 
diff --git a/repos/os/src/test/sandbox/main.cc b/repos/os/src/test/sandbox/main.cc
index c4120ddd06..288c28add1 100644
--- a/repos/os/src/test/sandbox/main.cc
+++ b/repos/os/src/test/sandbox/main.cc
@@ -16,6 +16,7 @@
 #include <base/session_object.h>
 #include <os/buffered_xml.h>
 #include <os/sandbox.h>
+#include <timer_session/connection.h>
 
 namespace Test {
 
@@ -58,6 +59,25 @@ struct Test::Main : Sandbox::Local_service_base::Wakeup
 
 	Log_service _log_service { _sandbox, *this };
 
+	/*
+	 * Periodically update the child version to stress the child destruction,
+	 * the closing of the session to the local LOG service, and and the child
+	 * re-creation.
+	 */
+	unsigned _dummy_version = 1;
+
+	void _handle_timer(Duration)
+	{
+		_dummy_version++;
+
+		_update_sandbox_config();
+	}
+
+	Timer::Connection _timer { _env };
+
+	Timer::Periodic_timeout<Main> _timeout_handler {
+		_timer, *this, &Main::_handle_timer, Microseconds(250*1000) };
+
 	void _generate_sandbox_config(Xml_generator &xml) const
 	{
 		xml.node("parent-provides", [&] () {
@@ -75,6 +95,7 @@ struct Test::Main : Sandbox::Local_service_base::Wakeup
 		xml.node("start", [&] () {
 			xml.attribute("name", "dummy");
 			xml.attribute("caps", 100);
+			xml.attribute("version", _dummy_version);
 			xml.node("resource", [&] () {
 				xml.attribute("name", "RAM");
 				xml.attribute("quantum", "2M");
@@ -86,10 +107,8 @@ struct Test::Main : Sandbox::Local_service_base::Wakeup
 				xml.node("create_log_connections", [&] () {
 					xml.attribute("ram_upgrade", "100K");
 					xml.attribute("count", "1"); });
-				xml.node("destroy_log_connections", [&] () { });
 				xml.node("log", [&] () {
 					xml.attribute("string", "done"); });
-				xml.node("exit", [&] () { });
 			});
 
 			xml.node("route", [&] () {
@@ -104,6 +123,19 @@ struct Test::Main : Sandbox::Local_service_base::Wakeup
 		});
 	}
 
+	void _update_sandbox_config()
+	{
+		Buffered_xml const config { _heap, "config", [&] (Xml_generator &xml) {
+			_generate_sandbox_config(xml); } };
+
+		config.with_xml_node([&] (Xml_node const &config) {
+
+			log("generated config: ", config);
+
+			_sandbox.apply_config(config);
+		});
+	}
+
 	/**
 	 * Sandbox::Local_service_base::Wakeup interface
 	 */
@@ -134,15 +166,7 @@ struct Test::Main : Sandbox::Local_service_base::Wakeup
 
 	Main(Env &env) : _env(env)
 	{
-		Buffered_xml const config { _heap, "config", [&] (Xml_generator &xml) {
-			_generate_sandbox_config(xml); } };
-
-		config.with_xml_node([&] (Xml_node const &config) {
-
-			log("generated config: ", config);
-
-			_sandbox.apply_config(config);
-		});
+		_update_sandbox_config();
 	}
 };