fix(userspace): clip display rects, bounds-check session app ids and fat16 handles, reject bad fat16 geometry, session kills via process cap

crates/servers/display-server/src/server.rs

@@ -1,7 +1,8 @@
 use server_framework::Server;
 
-use port_framebuffer::Framebuffer;
 use crate::protocol::{DisplayRequest, DisplayResponse};
+use port_framebuffer::Framebuffer;
+use types::display::Dimension;
 
 pub struct DisplayServer<F: Framebuffer> {
     fb: F,
@@ -24,7 +25,15 @@ impl<F: Framebuffer> Server for DisplayServer<F> {
     fn handle(&mut self, request: DisplayRequest) -> DisplayResponse {
         match request {
             DisplayRequest::DrawRect { x, y, w, h, color } => {
-                self.fb.fill_rect(x, y, w, h, color);
+                // Clients choose every coordinate; clip to the screen so a
+                // huge or off-screen rectangle costs nothing and coordinates
+                // never overflow in the driver.
+                let (fw, fh) = (self.fb.width().raw(), self.fb.height().raw());
+                if x.raw() < fw && y.raw() < fh {
+                    let w = Dimension::new(w.raw().min(fw - x.raw()));
+                    let h = Dimension::new(h.raw().min(fh - y.raw()));
+                    self.fb.fill_rect(x, y, w, h, color);
+                }
                 DisplayResponse::Ok
             }
             DisplayRequest::Clear { color } => {

crates/servers/display-server/src/tests/server.rs

@@ -1,11 +1,14 @@
-use driver_framebuffer_mock::MockFramebuffer;
 use crate::protocol::{DisplayRequest, DisplayResponse};
 use crate::server::DisplayServer;
+use driver_framebuffer_mock::MockFramebuffer;
 use server_framework::Server;
 use types::display::{Color, Dimension, ScreenCoord};
 
 fn test_server() -> DisplayServer<MockFramebuffer> {
-    DisplayServer::new(MockFramebuffer::new(Dimension::new(320), Dimension::new(240)))
+    DisplayServer::new(MockFramebuffer::new(
+        Dimension::new(320),
+        Dimension::new(240),
+    ))
 }
 
 #[test]
@@ -32,7 +35,9 @@ fn clear_resets_pixels() {
         h: Dimension::new(10),
         color: Color::from_raw(0xFF),
     });
-    server.handle(DisplayRequest::Clear { color: Color::from_raw(0) });
+    server.handle(DisplayRequest::Clear {
+        color: Color::from_raw(0),
+    });
     assert_eq!(server.framebuffer().pixel_count(), 0);
 }
 
@@ -69,8 +74,67 @@ fn draw_rect_correct_pixel_positions() {
     });
     let fb = server.framebuffer();
     assert_eq!(fb.pixel_count(), 4);
-    assert_eq!(fb.pixel_at(0), (ScreenCoord::new(5), ScreenCoord::new(10), Color::from_raw(0xAB)));
+    assert_eq!(
-    assert_eq!(fb.pixel_at(1), (ScreenCoord::new(6), ScreenCoord::new(10), Color::from_raw(0xAB)));
+        fb.pixel_at(0),
-    assert_eq!(fb.pixel_at(2), (ScreenCoord::new(5), ScreenCoord::new(11), Color::from_raw(0xAB)));
+        (
-    assert_eq!(fb.pixel_at(3), (ScreenCoord::new(6), ScreenCoord::new(11), Color::from_raw(0xAB)));
+            ScreenCoord::new(5),
+            ScreenCoord::new(10),
+            Color::from_raw(0xAB)
+        )
+    );
+    assert_eq!(
+        fb.pixel_at(1),
+        (
+            ScreenCoord::new(6),
+            ScreenCoord::new(10),
+            Color::from_raw(0xAB)
+        )
+    );
+    assert_eq!(
+        fb.pixel_at(2),
+        (
+            ScreenCoord::new(5),
+            ScreenCoord::new(11),
+            Color::from_raw(0xAB)
+        )
+    );
+    assert_eq!(
+        fb.pixel_at(3),
+        (
+            ScreenCoord::new(6),
+            ScreenCoord::new(11),
+            Color::from_raw(0xAB)
+        )
+    );
+}
+
+#[test]
+fn a_huge_rect_is_clipped_to_the_screen() {
+    let mut server = test_server();
+    let resp = server.handle(DisplayRequest::DrawRect {
+        x: ScreenCoord::new(310),
+        y: ScreenCoord::new(230),
+        w: Dimension::new(u16::MAX),
+        h: Dimension::new(u16::MAX),
+        color: Color::from_raw(0xFF),
+    });
+    assert_eq!(resp, DisplayResponse::Ok);
+    assert_eq!(server.framebuffer().pixel_count(), 10 * 10);
+    let (x, y, _) = server.framebuffer().pixel_at(99);
+    assert_eq!((x.raw(), y.raw()), (319, 239));
+}
+
+#[test]
+fn an_off_screen_rect_draws_nothing() {
+    let mut server = test_server();
+    for (x, y) in [(320, 0), (0, 240), (u16::MAX, u16::MAX)] {
+        server.handle(DisplayRequest::DrawRect {
+            x: ScreenCoord::new(x),
+            y: ScreenCoord::new(y),
+            w: Dimension::new(u16::MAX),
+            h: Dimension::new(u16::MAX),
+            color: Color::from_raw(0xFF),
+        });
+    }
+    assert_eq!(server.framebuffer().pixel_count(), 0);
 }

crates/servers/session-server/src/lib.rs

@@ -8,9 +8,10 @@ pub mod server;
 mod tests;
 
 pub fn run() -> ! {
-    let create_cap = types::bounded::CapSlot::new(0).unwrap();
+    // Slot 0 holds the endpoint init delegated. Init has no process
-    let mut srv = server::SessionServer::new(create_cap);
+    // creation authority to hand on yet, so Launch reports an error.
-    srv.register_app("snake", "/apps/snake.elf");
+    let mut srv = server::SessionServer::new(None);
+    let _ = srv.register_app("snake", "/apps/snake.elf");
     let ep = types::bounded::CapSlot::new(0).unwrap();
     server_framework::serve(&mut srv, ep)
 }

crates/servers/session-server/src/server.rs

@@ -1,6 +1,5 @@
 use server_framework::Server;
 use types::bounded::CapSlot;
-use types::id::ProcessId;
 
 use crate::protocol::{AppId, SessionRequest, SessionResponse};
 
@@ -15,8 +14,8 @@ pub struct AppEntry {
 
 #[derive(Clone, Copy)]
 struct RunningApp {
-    app: AppId,
+    /// The capability `Kill` is invoked on.
-    pid: ProcessId,
+    process: CapSlot,
 }
 
 pub struct SessionServer {
@@ -24,11 +23,12 @@ pub struct SessionServer {
     app_count: usize,
     running: [Option<RunningApp>; MAX_APPS],
     active: Option<AppId>,
-    create_cap: CapSlot,
+    /// Lets the server create processes; without one, Launch fails.
+    create_cap: Option<CapSlot>,
 }
 
 impl SessionServer {
-    pub fn new(create_cap: CapSlot) -> Self {
+    pub fn new(create_cap: Option<CapSlot>) -> Self {
         Self {
             apps: [const { None }; MAX_APPS],
             app_count: 0,
@@ -38,11 +38,17 @@ impl SessionServer {
         }
     }
 
-    pub fn register_app(&mut self, name: &'static str, path: &'static str) -> AppId {
+    /// Adds an app to the menu; `None` once the table is full.
+    pub fn register_app(&mut self, name: &'static str, path: &'static str) -> Option<AppId> {
+        let slot = self.apps.get_mut(self.app_count)?;
+        *slot = Some(AppEntry {
+            name,
+            path,
+            registered: true,
+        });
         let id = AppId::new(self.app_count as u16);
-        self.apps[self.app_count] = Some(AppEntry { name, path, registered: true });
         self.app_count += 1;
-        id
+        Some(id)
     }
 
     pub fn app_count(&self) -> usize {
@@ -53,8 +59,10 @@ impl SessionServer {
         self.active
     }
 
-    fn is_running(&self, app: AppId) -> bool {
+    /// The running-table entry for `app`; `None` for ids past the table,
-        self.running[app.raw() as usize].is_some()
+    /// which come straight from clients.
+    fn running_mut(&mut self, app: AppId) -> Option<&mut Option<RunningApp>> {
+        self.running.get_mut(app.raw() as usize)
     }
 
     fn launch_app(&mut self, app: AppId) -> SessionResponse {
@@ -63,31 +71,27 @@ impl SessionServer {
             return SessionResponse::NotFound;
         }
 
-        if self.is_running(app) {
+        if self.apps.get(idx).is_none_or(Option::is_none) {
+            return SessionResponse::NotFound;
+        }
+        if self.running_mut(app).is_none_or(|r| r.is_some()) {
             return SessionResponse::Error;
         }
-
+        let Some(process) = self.create_cap.and_then(libkos::syscall::create_process) else {
-        let _app_entry = match &self.apps[idx] {
+            return SessionResponse::Error;
-            Some(entry) => entry,
-            None => return SessionResponse::NotFound,
         };
-
+        if let Some(r) = self.running_mut(app) {
-        let process_cap = match libkos::syscall::create_process(self.create_cap) {
+            *r = Some(RunningApp { process });
-            Some(cap) => cap,
+        }
-            None => return SessionResponse::Error,
-        };
-
-        let pid = ProcessId::from_raw(process_cap.value() as u16);
-        self.running[idx] = Some(RunningApp { app, pid });
         self.active = Some(app);
 
         SessionResponse::Launched { app }
     }
 
     fn kill_app(&mut self, app: AppId) -> SessionResponse {
-        let idx = app.raw() as usize;
+        match self.running_mut(app).and_then(Option::take) {
-        match self.running[idx].take() {
+            Some(running) => {
-            Some(_running) => {
+                let _ = libkos::syscall::kill(running.process);
                 if self.active == Some(app) {
                     self.active = None;
                 }
@@ -106,7 +110,9 @@ impl Server for SessionServer {
         match request {
             SessionRequest::Launch { app } => self.launch_app(app),
             SessionRequest::Kill { app } => self.kill_app(app),
-            SessionRequest::ListApps => SessionResponse::AppList { count: self.app_count },
+            SessionRequest::ListApps => SessionResponse::AppList {
+                count: self.app_count,
+            },
             SessionRequest::ActiveApp => match self.active {
                 Some(app) => SessionResponse::Active { app },
                 None => SessionResponse::NotFound,

crates/servers/session-server/src/tests/server.rs

@@ -4,9 +4,9 @@ use server_framework::Server;
 
 fn test_server() -> SessionServer {
     let cap = types::bounded::CapSlot::new(0).unwrap();
-    let mut s = SessionServer::new(cap);
+    let mut s = SessionServer::new(Some(cap));
-    s.register_app("snake", "/apps/snake.elf");
+    s.register_app("snake", "/apps/snake.elf").unwrap();
-    s.register_app("clock", "/apps/clock.elf");
+    s.register_app("clock", "/apps/clock.elf").unwrap();
     s
 }
 
@@ -35,7 +35,9 @@ fn launch_sets_active() {
 #[test]
 fn launch_invalid_app_returns_not_found() {
     let mut s = test_server();
-    let resp = s.handle(SessionRequest::Launch { app: AppId::new(99) });
+    let resp = s.handle(SessionRequest::Launch {
+        app: AppId::new(99),
+    });
     assert_eq!(resp, SessionResponse::NotFound);
 }
 
@@ -77,3 +79,36 @@ fn no_active_app_initially() {
     let resp = s.handle(SessionRequest::ActiveApp);
     assert_eq!(resp, SessionResponse::NotFound);
 }
+
+#[test]
+fn ids_past_the_table_are_rejected_not_indexed() {
+    let mut server = test_server();
+    for raw in [2, 16, 17, u16::MAX] {
+        let app = AppId::new(raw);
+        assert_eq!(
+            server.handle(SessionRequest::Kill { app }),
+            SessionResponse::NotFound
+        );
+        assert_eq!(
+            server.handle(SessionRequest::Launch { app }),
+            SessionResponse::NotFound
+        );
+    }
+}
+
+#[test]
+fn the_app_table_fills_up_without_panicking() {
+    let mut server = SessionServer::new(None);
+    let added = (0..20)
+        .filter(|_| server.register_app("x", "/x").is_some())
+        .count();
+    assert_eq!(added, 16);
+}
+
+#[test]
+fn launching_needs_process_creation_authority() {
+    let mut server = SessionServer::new(None);
+    server.register_app("snake", "/apps/snake.elf").unwrap();
+    let resp = server.handle(SessionRequest::Launch { app: AppId::new(0) });
+    assert_eq!(resp, SessionResponse::Error);
+}

crates/servers/storage-server/src/fs/fat16.rs

@@ -1,5 +1,5 @@
-use port_block_device::{BlockDevice, SectorId};
 use crate::filesystem::{DirEntry, FileHandle, FileName, FileSystem};
+use port_block_device::{BlockDevice, SectorId};
 
 const SECTOR_SIZE: usize = 512;
 const MAX_OPEN_FILES: usize = 8;
@@ -15,10 +15,16 @@ struct Bpb {
 }
 
 impl Bpb {
+    /// Reads the BPB, refusing geometry the rest of this module cannot
+    /// work with: the disk is untrusted input like any client request.
     fn parse(sector: &[u8]) -> Option<Self> {
-        if sector.len() < SECTOR_SIZE { return None; }
+        if sector.len() < SECTOR_SIZE {
-        if sector[510] != 0x55 || sector[511] != 0xAA { return None; }
+            return None;
-        Some(Self {
+        }
+        if sector[510] != 0x55 || sector[511] != 0xAA {
+            return None;
+        }
+        let bpb = Self {
             bytes_per_sector: u16::from_le_bytes([sector[11], sector[12]]),
             sectors_per_cluster: sector[13],
             reserved_sectors: u16::from_le_bytes([sector[14], sector[15]]),
@@ -26,10 +32,17 @@ impl Bpb {
             root_entry_count: u16::from_le_bytes([sector[17], sector[18]]),
             total_sectors: u16::from_le_bytes([sector[19], sector[20]]),
             sectors_per_fat: u16::from_le_bytes([sector[22], sector[23]]),
-        })
+        };
+        let usable = bpb.bytes_per_sector as usize == SECTOR_SIZE
+            && bpb.sectors_per_cluster.is_power_of_two()
+            && bpb.num_fats > 0
+            && bpb.sectors_per_fat > 0;
+        usable.then_some(bpb)
     }
 
-    fn fat_start(&self) -> u32 { self.reserved_sectors as u32 }
+    fn fat_start(&self) -> u32 {
+        self.reserved_sectors as u32
+    }
 
     fn root_dir_start(&self) -> u32 {
         self.fat_start() + (self.num_fats as u32) * (self.sectors_per_fat as u32)
@@ -44,8 +57,11 @@ impl Bpb {
         self.root_dir_start() + self.root_dir_sectors()
     }
 
-    fn cluster_to_sector(&self, cluster: u16) -> u32 {
+    /// The first sector of data cluster `cluster`. Clusters 0 and 1 are
-        self.data_start() + ((cluster as u32) - 2) * (self.sectors_per_cluster as u32)
+    /// reserved and hold no data.
+    fn cluster_to_sector(&self, cluster: u16) -> Option<u32> {
+        let index = (cluster as u32).checked_sub(2)?;
+        Some(self.data_start() + index * (self.sectors_per_cluster as u32))
     }
 
     fn cluster_size(&self) -> usize {
@@ -70,9 +86,15 @@ pub struct Fat16<B: BlockDevice> {
 impl<B: BlockDevice> Fat16<B> {
     pub fn mount(mut device: B) -> Option<Self> {
         let mut buf = [0u8; SECTOR_SIZE];
-        if !device.read_sector(SectorId::new(0), &mut buf) { return None; }
+        if !device.read_sector(SectorId::new(0), &mut buf) {
+            return None;
+        }
         let bpb = Bpb::parse(&buf)?;
-        Some(Self { device, bpb, open_files: [const { None }; MAX_OPEN_FILES] })
+        Some(Self {
+            device,
+            bpb,
+            open_files: [const { None }; MAX_OPEN_FILES],
+        })
     }
 
     fn read_fat_entry(&mut self, cluster: u16) -> u16 {
@@ -80,7 +102,9 @@ impl<B: BlockDevice> Fat16<B> {
         let sector = self.bpb.fat_start() + fat_offset / (SECTOR_SIZE as u32);
         let offset = (fat_offset % (SECTOR_SIZE as u32)) as usize;
         let mut buf = [0u8; SECTOR_SIZE];
-        if !self.device.read_sector(SectorId::new(sector), &mut buf) { return 0xFFFF; }
+        if !self.device.read_sector(SectorId::new(sector), &mut buf) {
+            return 0xFFFF;
+        }
         u16::from_le_bytes([buf[offset], buf[offset + 1]])
     }
 
@@ -109,7 +133,9 @@ impl<B: BlockDevice> Fat16<B> {
     fn free_chain(&mut self, start: u16) {
         let mut cluster = start;
         loop {
-            if cluster < 2 || cluster >= 0xFFF8 { break; }
+            if cluster < 2 || cluster >= 0xFFF8 {
+                break;
+            }
             let next = self.read_fat_entry(cluster);
             self.write_fat_entry(cluster, 0);
             cluster = next;
@@ -120,13 +146,25 @@ impl<B: BlockDevice> Fat16<B> {
         let mut buf = [0u8; SECTOR_SIZE];
         let fat_name = to_fat_name(name);
         for s in 0..self.bpb.root_dir_sectors() {
-            if !self.device.read_sector(SectorId::new(self.bpb.root_dir_start() + s), &mut buf) { continue; }
+            if !self
+                .device
+                .read_sector(SectorId::new(self.bpb.root_dir_start() + s), &mut buf)
+            {
+                continue;
+            }
             for i in 0..(SECTOR_SIZE / 32) {
                 let off = i * 32;
-                if buf[off] == 0x00 || buf[off] == 0xE5 { continue; }
+                if buf[off] == 0x00 || buf[off] == 0xE5 {
+                    continue;
+                }
                 if buf[off..off + 11] == fat_name {
                     let cluster = u16::from_le_bytes([buf[off + 26], buf[off + 27]]);
-                    let size = u32::from_le_bytes([buf[off + 28], buf[off + 29], buf[off + 30], buf[off + 31]]);
+                    let size = u32::from_le_bytes([
+                        buf[off + 28],
+                        buf[off + 29],
+                        buf[off + 30],
+                        buf[off + 31],
+                    ]);
                     return Some((cluster, size as usize));
                 }
             }
@@ -139,16 +177,22 @@ impl<B: BlockDevice> Fat16<B> {
         let fat_name = to_fat_name(name);
         for s in 0..self.bpb.root_dir_sectors() {
             let sector = self.bpb.root_dir_start() + s;
-            if !self.device.read_sector(SectorId::new(sector), &mut buf) { continue; }
+            if !self.device.read_sector(SectorId::new(sector), &mut buf) {
+                continue;
+            }
             for i in 0..(SECTOR_SIZE / 32) {
                 let off = i * 32;
                 if buf[off] == 0x00 || buf[off] == 0xE5 {
                     buf[off..off + 11].copy_from_slice(&fat_name);
                     buf[off + 11] = 0x20;
-                    for b in &mut buf[off + 12..off + 26] { *b = 0; }
+                    for b in &mut buf[off + 12..off + 26] {
+                        *b = 0;
+                    }
                     buf[off + 26] = cluster as u8;
                     buf[off + 27] = (cluster >> 8) as u8;
-                    for b in &mut buf[off + 28..off + 32] { *b = 0; }
+                    for b in &mut buf[off + 28..off + 32] {
+                        *b = 0;
+                    }
                     self.device.write_sector(SectorId::new(sector), &buf);
                     return true;
                 }
@@ -162,7 +206,9 @@ impl<B: BlockDevice> Fat16<B> {
         let fat_name = to_fat_name(name);
         for s in 0..self.bpb.root_dir_sectors() {
             let sector = self.bpb.root_dir_start() + s;
-            if !self.device.read_sector(SectorId::new(sector), &mut buf) { continue; }
+            if !self.device.read_sector(SectorId::new(sector), &mut buf) {
+                continue;
+            }
             for i in 0..(SECTOR_SIZE / 32) {
                 let off = i * 32;
                 if buf[off..off + 11] == fat_name {
@@ -181,7 +227,9 @@ impl<B: BlockDevice> Fat16<B> {
         let fat_name = to_fat_name(name);
         for s in 0..self.bpb.root_dir_sectors() {
             let sector = self.bpb.root_dir_start() + s;
-            if !self.device.read_sector(SectorId::new(sector), &mut buf) { continue; }
+            if !self.device.read_sector(SectorId::new(sector), &mut buf) {
+                continue;
+            }
             for i in 0..(SECTOR_SIZE / 32) {
                 let off = i * 32;
                 if buf[off..off + 11] == fat_name {
@@ -199,12 +247,20 @@ impl<B: BlockDevice> Fat16<B> {
         let mut cluster = start;
         for _ in 0..skip_clusters {
             let next = self.read_fat_entry(cluster);
-            if next >= 0xFFF8 { return cluster; }
+            if next >= 0xFFF8 {
+                return cluster;
+            }
             cluster = next;
         }
         cluster
     }
 
+    /// The open-file slot `handle` names; `None` for handles past the
+    /// table, which come straight from clients.
+    fn slot(&mut self, handle: FileHandle) -> Option<&mut Option<OpenFile>> {
+        self.open_files.get_mut(handle.raw() as usize)
+    }
+
     fn alloc_handle(&self) -> Option<FileHandle> {
         for i in 0..MAX_OPEN_FILES {
             if self.open_files[i].is_none() {
@@ -219,15 +275,20 @@ impl<B: BlockDevice> FileSystem for Fat16<B> {
     fn open(&mut self, path: &[u8]) -> Option<FileHandle> {
         let (cluster, size) = self.find_in_root_dir(path)?;
         let handle = self.alloc_handle()?;
-        self.open_files[handle.raw() as usize] = Some(OpenFile {
+        *self.slot(handle)? = Some(OpenFile {
-            first_cluster: cluster, size, offset: 0, name: FileName::from_bytes(path),
+            first_cluster: cluster,
+            size,
+            offset: 0,
+            name: FileName::from_bytes(path),
         });
         Some(handle)
     }
 
     fn read(&mut self, handle: FileHandle, buf: &mut [u8]) -> Option<usize> {
-        let file = *self.open_files[handle.raw() as usize].as_ref()?;
+        let file = (*self.slot(handle)?)?;
-        if file.offset >= file.size { return Some(0); }
+        if file.offset >= file.size {
+            return Some(0);
+        }
 
         let remaining = file.size - file.offset;
         let to_read = buf.len().min(remaining);
@@ -242,28 +303,41 @@ impl<B: BlockDevice> FileSystem for Fat16<B> {
         while read < to_read {
             let sector_in_cluster = skip / SECTOR_SIZE;
             let byte_in_sector = skip % SECTOR_SIZE;
-            let sector = self.bpb.cluster_to_sector(cluster) + sector_in_cluster as u32;
+            let Some(base) = self.bpb.cluster_to_sector(cluster) else {
-            if !self.device.read_sector(SectorId::new(sector), &mut sector_buf) { break; }
+                break;
+            };
+            let sector = base + sector_in_cluster as u32;
+            if !self
+                .device
+                .read_sector(SectorId::new(sector), &mut sector_buf)
+            {
+                break;
+            }
 
             let chunk = (SECTOR_SIZE - byte_in_sector).min(to_read - read);
-            buf[read..read + chunk].copy_from_slice(&sector_buf[byte_in_sector..byte_in_sector + chunk]);
+            buf[read..read + chunk]
+                .copy_from_slice(&sector_buf[byte_in_sector..byte_in_sector + chunk]);
             read += chunk;
             skip += chunk;
 
             if skip >= cs {
                 let next = self.read_fat_entry(cluster);
-                if next >= 0xFFF8 { break; }
+                if next >= 0xFFF8 {
+                    break;
+                }
                 cluster = next;
                 skip = 0;
             }
         }
 
-        self.open_files[handle.raw() as usize].as_mut().map(|f| f.offset += read);
+        if let Some(Some(f)) = self.slot(handle) {
+            f.offset += read;
+        }
         Some(read)
     }
 
     fn write(&mut self, handle: FileHandle, buf: &[u8]) -> Option<usize> {
-        let mut file = *self.open_files[handle.raw() as usize].as_ref()?;
+        let mut file = (*self.slot(handle)?)?;
         let cs = self.bpb.cluster_size();
 
         if file.first_cluster < 2 {
@@ -280,11 +354,16 @@ impl<B: BlockDevice> FileSystem for Fat16<B> {
         while written < buf.len() {
             let sector_in_cluster = pos / SECTOR_SIZE;
             let byte_in_sector = pos % SECTOR_SIZE;
-            let sector = self.bpb.cluster_to_sector(cluster) + sector_in_cluster as u32;
+            let Some(base) = self.bpb.cluster_to_sector(cluster) else {
-            self.device.read_sector(SectorId::new(sector), &mut sector_buf);
+                break;
+            };
+            let sector = base + sector_in_cluster as u32;
+            self.device
+                .read_sector(SectorId::new(sector), &mut sector_buf);
 
             let chunk = (SECTOR_SIZE - byte_in_sector).min(buf.len() - written);
-            sector_buf[byte_in_sector..byte_in_sector + chunk].copy_from_slice(&buf[written..written + chunk]);
+            sector_buf[byte_in_sector..byte_in_sector + chunk]
+                .copy_from_slice(&buf[written..written + chunk]);
             self.device.write_sector(SectorId::new(sector), &sector_buf);
 
             written += chunk;
@@ -297,23 +376,28 @@ impl<B: BlockDevice> FileSystem for Fat16<B> {
                         if let Some(new_c) = self.alloc_cluster() {
                             self.write_fat_entry(cluster, new_c);
                             cluster = new_c;
-                        } else { break; }
+                        } else {
+                            break;
+                        }
                     }
-                } else { cluster = next; }
+                } else {
+                    cluster = next;
+                }
                 pos = 0;
             }
         }
 
         file.offset += written;
-        if file.offset > file.size { file.size = file.offset; }
+        if file.offset > file.size {
-        self.open_files[handle.raw() as usize] = Some(file);
+            file.size = file.offset;
+        }
+        *self.slot(handle)? = Some(file);
         Some(written)
     }
 
     fn close(&mut self, handle: FileHandle) {
-        let file = match self.open_files[handle.raw() as usize].take() {
+        let Some(file) = self.slot(handle).and_then(Option::take) else {
-            Some(f) => f,
+            return;
-            None => return,
         };
         let mut name_buf = [0u8; 32];
         let name_bytes = file.name.as_bytes();
@@ -326,13 +410,29 @@ impl<B: BlockDevice> FileSystem for Fat16<B> {
         let mut buf = [0u8; SECTOR_SIZE];
         let mut count = 0;
         for s in 0..self.bpb.root_dir_sectors() {
-            if !self.device.read_sector(SectorId::new(self.bpb.root_dir_start() + s), &mut buf) { continue; }
+            if !self
+                .device
+                .read_sector(SectorId::new(self.bpb.root_dir_start() + s), &mut buf)
+            {
+                continue;
+            }
             for i in 0..(SECTOR_SIZE / 32) {
-                if count >= entries.len() { return Some(count); }
+                if count >= entries.len() {
+                    return Some(count);
+                }
                 let off = i * 32;
-                if buf[off] == 0x00 { return Some(count); }
+                if buf[off] == 0x00 {
-                if buf[off] == 0xE5 || buf[off + 11] & 0x08 != 0 { continue; }
+                    return Some(count);
-                let size = u32::from_le_bytes([buf[off + 28], buf[off + 29], buf[off + 30], buf[off + 31]]);
+                }
+                if buf[off] == 0xE5 || buf[off + 11] & 0x08 != 0 {
+                    continue;
+                }
+                let size = u32::from_le_bytes([
+                    buf[off + 28],
+                    buf[off + 29],
+                    buf[off + 30],
+                    buf[off + 31],
+                ]);
                 entries[count] = DirEntry {
                     name: from_fat_name(&buf[off..off + 11]),
                     size: size as usize,
@@ -345,15 +445,20 @@ impl<B: BlockDevice> FileSystem for Fat16<B> {
     }
 
     fn create(&mut self, path: &[u8]) -> Option<FileHandle> {
-        if self.find_in_root_dir(path).is_some() { return None; }
+        if self.find_in_root_dir(path).is_some() {
+            return None;
+        }
         let cluster = self.alloc_cluster()?;
         if !self.create_dir_entry(path, cluster) {
             self.write_fat_entry(cluster, 0);
             return None;
         }
         let handle = self.alloc_handle()?;
-        self.open_files[handle.raw() as usize] = Some(OpenFile {
+        *self.slot(handle)? = Some(OpenFile {
-            first_cluster: cluster, size: 0, offset: 0, name: FileName::from_bytes(path),
+            first_cluster: cluster,
+            size: 0,
+            offset: 0,
+            name: FileName::from_bytes(path),
         });
         Some(handle)
     }
@@ -362,20 +467,26 @@ impl<B: BlockDevice> FileSystem for Fat16<B> {
         if let Some(cluster) = self.delete_dir_entry(path) {
             self.free_chain(cluster);
             true
-        } else { false }
+        } else {
+            false
+        }
     }
 
     fn file_size(&mut self, handle: FileHandle) -> Option<usize> {
-        self.open_files[handle.raw() as usize].as_ref().map(|f| f.size)
+        self.slot(handle)?.as_ref().map(|f| f.size)
     }
 
-    fn mkdir(&mut self, _path: &[u8]) -> bool { false }
+    fn mkdir(&mut self, _path: &[u8]) -> bool {
+        false
+    }
 }
 
 fn to_fat_name(name: &[u8]) -> [u8; 11] {
     let mut fat = [b' '; 11];
     let dot_pos = name.iter().position(|&b| b == b'.').unwrap_or(name.len());
-    for i in 0..dot_pos.min(8) { fat[i] = name[i].to_ascii_uppercase(); }
+    for i in 0..dot_pos.min(8) {
+        fat[i] = name[i].to_ascii_uppercase();
+    }
     if dot_pos < name.len() {
         for i in 0..(name.len() - dot_pos - 1).min(3) {
             fat[8 + i] = name[dot_pos + 1 + i].to_ascii_uppercase();
@@ -387,12 +498,27 @@ fn to_fat_name(name: &[u8]) -> [u8; 11] {
 fn from_fat_name(raw: &[u8]) -> FileName {
     let mut buf = [0u8; 32];
     let mut pos = 0;
-    let name_end = raw[..8].iter().rposition(|&b| b != b' ').map(|i| i + 1).unwrap_or(0);
+    let name_end = raw[..8]
-    for i in 0..name_end { buf[pos] = raw[i].to_ascii_lowercase(); pos += 1; }
+        .iter()
-    let ext_end = raw[8..11].iter().rposition(|&b| b != b' ').map(|i| i + 1).unwrap_or(0);
+        .rposition(|&b| b != b' ')
+        .map(|i| i + 1)
+        .unwrap_or(0);
+    for i in 0..name_end {
+        buf[pos] = raw[i].to_ascii_lowercase();
+        pos += 1;
+    }
+    let ext_end = raw[8..11]
+        .iter()
+        .rposition(|&b| b != b' ')
+        .map(|i| i + 1)
+        .unwrap_or(0);
     if ext_end > 0 {
-        buf[pos] = b'.'; pos += 1;
+        buf[pos] = b'.';
-        for i in 0..ext_end { buf[pos] = raw[8 + i].to_ascii_lowercase(); pos += 1; }
+        pos += 1;
+        for i in 0..ext_end {
+            buf[pos] = raw[8 + i].to_ascii_lowercase();
+            pos += 1;
+        }
     }
     FileName::from_bytes(&buf[..pos])
 }

crates/servers/storage-server/src/tests/fat16.rs

@@ -1,21 +1,25 @@
 use crate::adapters::mock_block::MockBlockDevice;
-use port_block_device::{BlockDevice, SectorId};
 use crate::filesystem::{DirEntry, FileSystem};
 use crate::fs::fat16::Fat16;
+use port_block_device::{BlockDevice, SectorId};
 
 fn format_fat16_disk(sectors: usize) -> MockBlockDevice {
     let mut dev = MockBlockDevice::new(sectors);
     let mut boot = [0u8; 512];
 
     // BPB
-    boot[11] = 0x00; boot[12] = 0x02; // bytes_per_sector = 512
+    boot[11] = 0x00;
-    boot[13] = 1;                       // sectors_per_cluster = 1
+    boot[12] = 0x02; // bytes_per_sector = 512
-    boot[14] = 1; boot[15] = 0;        // reserved_sectors = 1
+    boot[13] = 1; // sectors_per_cluster = 1
-    boot[16] = 1;                       // num_fats = 1
+    boot[14] = 1;
-    boot[17] = 16; boot[18] = 0;       // root_entry_count = 16
+    boot[15] = 0; // reserved_sectors = 1
-    boot[19] = sectors as u8;           // total_sectors
+    boot[16] = 1; // num_fats = 1
+    boot[17] = 16;
+    boot[18] = 0; // root_entry_count = 16
+    boot[19] = sectors as u8; // total_sectors
     boot[20] = (sectors >> 8) as u8;
-    boot[22] = 1; boot[23] = 0;        // sectors_per_fat = 1
+    boot[22] = 1;
+    boot[23] = 0; // sectors_per_fat = 1
 
     // boot signature
     boot[510] = 0x55;
@@ -110,3 +114,75 @@ fn create_duplicate_returns_none() {
     fs.create(b"dup.txt").map(|h| fs.close(h));
     assert!(fs.create(b"dup.txt").is_none());
 }
+
+// ---- hostile input: handles from clients, geometry and chains from disk ----
+
+fn with_boot_byte(sectors: usize, index: usize, value: u8) -> MockBlockDevice {
+    let mut dev = format_fat16_disk(sectors);
+    let mut boot = [0u8; 512];
+    dev.read_sector(SectorId::new(0), &mut boot);
+    boot[index] = value;
+    dev.write_sector(SectorId::new(0), &boot);
+    dev
+}
+
+#[test]
+fn mount_rejects_geometry_the_driver_cannot_use() {
+    // sectors_per_cluster 0 or 3, bytes_per_sector 513 or 0, no FATs, no
+    // FAT sectors
+    for (index, value) in [(13, 0), (13, 3), (11, 1), (12, 0), (16, 0), (22, 0)] {
+        let dev = with_boot_byte(32, index, value);
+        assert!(
+            Fat16::mount(dev).is_none(),
+            "byte {index} = {value} accepted"
+        );
+    }
+}
+
+#[test]
+fn handles_past_the_table_are_rejected_not_indexed() {
+    use crate::filesystem::FileHandle;
+    let mut fs = mount_test_disk();
+    let mut buf = [0u8; 16];
+    for raw in [8, 9, u16::MAX] {
+        let h = FileHandle::new(raw);
+        assert_eq!(fs.read(h, &mut buf), None);
+        assert_eq!(fs.write(h, b"x"), None);
+        assert_eq!(fs.file_size(h), None);
+        fs.close(h);
+    }
+}
+
+/// Points a root-directory entry for `name` at `cluster` with `size`.
+fn plant_entry(dev: &mut MockBlockDevice, name: &[u8; 11], cluster: u16, size: u32) {
+    // root dir starts after 1 reserved + 1 FAT sector
+    let mut dir = [0u8; 512];
+    dir[..11].copy_from_slice(name);
+    dir[11] = 0x20;
+    dir[26..28].copy_from_slice(&cluster.to_le_bytes());
+    dir[28..32].copy_from_slice(&size.to_le_bytes());
+    dev.write_sector(SectorId::new(2), &dir);
+}
+
+#[test]
+fn a_file_pointing_at_cluster_zero_reads_nothing() {
+    let mut dev = format_fat16_disk(32);
+    plant_entry(&mut dev, b"BAD     BIN", 0, 4096);
+    let mut fs = Fat16::mount(dev).unwrap();
+    let h = fs.open(b"bad.bin").expect("open");
+    let mut buf = [0u8; 64];
+    assert_eq!(fs.read(h, &mut buf), Some(0));
+}
+
+#[test]
+fn deleting_a_file_with_a_cyclic_chain_terminates() {
+    let mut dev = format_fat16_disk(32);
+    // cluster 2 -> 3 -> 2 -> ...
+    let mut fat = [0u8; 512];
+    fat[4..6].copy_from_slice(&3u16.to_le_bytes());
+    fat[6..8].copy_from_slice(&2u16.to_le_bytes());
+    dev.write_sector(SectorId::new(1), &fat);
+    plant_entry(&mut dev, b"LOOP    BIN", 2, 4096);
+    let mut fs = Fat16::mount(dev).unwrap();
+    assert!(fs.delete(b"loop.bin"));
+}