block: prevent from dereferencing invalid pointers

Until now, block drivers had to deal with a pointer to the client session component, e.g.: to acknowledge block packets already processed. When a session was closed, the driver object wasn't informed explicitly, which leads to defensive programming, or lastly to a race-condition in test-blk-srv. To prevent from this class of errors, the pointer is now private to the generic block driver base class, and not accessible to the concrete driver implementation. Moreover, the driver gets explicitly informed when a session got invalidated. Ref #113
genodelabs · Feb 6, 2014 · 46f405f · 46f405f
1 parent 418717a
commit 46f405f
Show file tree

Hide file tree

Showing 14 changed files with 274 additions and 197 deletions.
diff --git a/dde_linux/src/lib/usb/storage/storage.cc b/dde_linux/src/lib/usb/storage/storage.cc
@@ -37,18 +37,7 @@ class Storage_device : public Genode::List<Storage_device>::Element,
 		static void _sync_done(struct scsi_cmnd *cmnd) {
 			complete((struct completion *)cmnd->back); }
 
-		static void _async_done(struct scsi_cmnd *cmnd)
-		{
-			Block::Session_component *session = static_cast<Block::Session_component *>(cmnd->session);
-			Block::Packet_descriptor *packet  = static_cast<Block::Packet_descriptor *>(cmnd->packet);
-
-			if (verbose)
-				PDBG("ACK packet for block: %llu status: %d", packet->block_number(), cmnd->result);
-
-			session->ack_packet(*packet);
-			Genode::destroy(Genode::env()->heap(), packet);
-			_scsi_free_command(cmnd);
-		}
+		static void _async_done(struct scsi_cmnd *cmnd);
 
 		void _capacity()
 		{
@@ -88,7 +77,7 @@ class Storage_device : public Genode::List<Storage_device>::Element,
 
 		void _io(Block::sector_t block_nr, Genode::size_t block_count,
 		         Block::Packet_descriptor packet,
-		         Genode::addr_t virt, Genode::addr_t phys, bool read)
+		         Genode::addr_t phys, bool read)
 		{
 			if (block_nr > _block_count)
 				throw Io_error();
@@ -108,7 +97,6 @@ class Storage_device : public Genode::List<Storage_device>::Element,
 			Block::Packet_descriptor *p = new (Genode::env()->heap()) Block::Packet_descriptor();
 			*p = packet;
 			cmnd->packet  = (void *)p;
-			cmnd->session = (void *)session;
 
 			Genode::uint32_t be_block_nr = bswap<Genode::uint32_t>(block_nr);
 			memcpy(&cmnd->cmnd[2], &be_block_nr, 4);
@@ -118,7 +106,7 @@ class Storage_device : public Genode::List<Storage_device>::Element,
 			memcpy(&cmnd->cmnd[7], &be_block_count, 2);
 
 			/* setup command */
-			scsi_setup_buffer(cmnd, block_count * _block_size, (void *)virt, phys);
+			scsi_setup_buffer(cmnd, block_count * _block_size, (void *)0, phys);
 
 			/*
 			 * Required by 'last_sector_hacks' in 'drivers/usb/storage/transprot.c
@@ -156,22 +144,14 @@ class Storage_device : public Genode::List<Storage_device>::Element,
 		void read_dma(Block::sector_t           block_number,
 		              Genode::size_t            block_count,
 		              Genode::addr_t            phys,
-		              Block::Packet_descriptor &packet)
-		{
-			_io(block_number, block_count, packet,
-			    (Genode::addr_t)session->tx_sink()->packet_content(packet),
-			    phys, true);
-		}
+		              Block::Packet_descriptor &packet) {
+			_io(block_number, block_count, packet, phys, true); }
 
 		void write_dma(Block::sector_t           block_number,
 		               Genode::size_t            block_count,
 		               Genode::addr_t            phys,
-		               Block::Packet_descriptor &packet)
-		{
-			_io(block_number, block_count, packet,
-			    (Genode::addr_t)session->tx_sink()->packet_content(packet),
-			    phys, false);
-		}
+		               Block::Packet_descriptor &packet) {
+			_io(block_number, block_count, packet, phys, false); }
 
 		bool dma_enabled() { return true; }
 
@@ -198,12 +178,16 @@ struct Factory : Block::Driver_factory
 };
 
 
+static Storage_device *device = nullptr;
+
+
 void scsi_add_device(struct scsi_device *sdev)
 {
 	using namespace Genode;
 	static bool announce = false;
 
 	static struct Factory factory(sdev);
+	device = &factory.device;
 
 	/*
 	 * XXX  move to 'main'
@@ -215,3 +199,17 @@ void scsi_add_device(struct scsi_device *sdev)
 	}
 }
 
+
+void Storage_device::_async_done(struct scsi_cmnd *cmnd)
+{
+	Block::Packet_descriptor *packet =
+		static_cast<Block::Packet_descriptor *>(cmnd->packet);
+
+	if (verbose)
+		PDBG("ACK packet for block: %llu status: %d",
+		     packet->block_number(), cmnd->result);
+
+	device->ack_packet(*packet);
+	Genode::destroy(Genode::env()->heap(), packet);
+	_scsi_free_command(cmnd);
+}
diff --git a/gems/src/server/http_blk/main.cc b/gems/src/server/http_blk/main.cc
@@ -56,7 +56,7 @@ class Driver : public Block::Driver
 		{
 			_http.cmd_get(block_nr * _block_size, block_count * _block_size,
 			              (addr_t)buffer);
-			session->ack_packet(packet);
+			ack_packet(packet);
 		}
 	};
 

diff --git a/os/include/block/component.h b/os/include/block/component.h
@@ -18,7 +18,6 @@
 #include <root/component.h>
 #include <os/signal_rpc_dispatcher.h>
 #include <os/server.h>
-#include <block_session/rpc_object.h>
 #include <block/driver.h>
 
 namespace Block {
@@ -64,7 +63,7 @@ class Block::Session_component_base
 
 
 class Block::Session_component : public Block::Session_component_base,
-                                 public Block::Session_rpc_object
+                                 public Block::Driver_session
 {
 	private:
 
@@ -183,7 +182,7 @@ class Block::Session_component : public Block::Session_component_base,
 		Session_component(Driver_factory           &driver_factory,
 		                  Server::Entrypoint       &ep, size_t buf_size)
 		: Session_component_base(driver_factory, buf_size),
-		  Session_rpc_object(_rq_ds, ep.rpc_ep()),
+		  Driver_session(_rq_ds, ep.rpc_ep()),
 		  _rq_phys(Dataspace_client(_rq_ds).phys_addr()),
 		  _sink_ack(ep, *this, &Session_component::_ready_to_ack),
 		  _sink_submit(ep, *this, &Session_component::_packet_avail),
@@ -193,9 +192,11 @@ class Block::Session_component : public Block::Session_component_base,
 			_tx.sigh_ready_to_ack(_sink_ack);
 			_tx.sigh_packet_avail(_sink_submit);
 
-			_driver.session = this;
+			_driver.session(this);
 		}
 
+		~Session_component() { _driver.session(nullptr); }
+
 		/**
 		 * Acknowledges a packet processed by the driver to the client
 		 *
@@ -204,7 +205,7 @@ class Block::Session_component : public Block::Session_component_base,
 		 *
 		 * \throw Ack_congestion
 		 */
-		void ack_packet(Packet_descriptor &packet, bool success = true)
+		void ack_packet(Packet_descriptor &packet, bool success)
 		{
 			packet.succeeded(success);
 			_ack_packet(packet);