From 944b175a6e89789dc4b392db4c8b3732e1dc68dd Mon Sep 17 00:00:00 2001
From: Ron Williams <ron.williams.redox@gmail.com>
Date: Wed, 3 Jan 2024 16:39:58 +0000
Subject: [PATCH] fix symbolic links plus general cleanup

---
 Cargo.toml                  |   2 +-
 src/mount/redox/resource.rs | 164 +++++++++++++++++++++++++++---------
 src/mount/redox/scheme.rs   | 138 ++++++++++++++++++------------
 3 files changed, 213 insertions(+), 91 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 3f37a08..d875c25 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -6,7 +6,7 @@ version = "0.5.13"
 license-file = "LICENSE"
 readme = "README.md"
 authors = ["Jeremy Soller <jackpot51@gmail.com>"]
-edition = "2018"
+edition = "2021"
 
 [lib]
 name = "redoxfs"
diff --git a/src/mount/redox/resource.rs b/src/mount/redox/resource.rs
index 8940a64..5c5f5af 100644
--- a/src/mount/redox/resource.rs
+++ b/src/mount/redox/resource.rs
@@ -5,13 +5,13 @@ use std::time::{SystemTime, UNIX_EPOCH};
 use alloc::collections::BTreeMap;
 use range_tree::RangeTree;
 
-use syscall::{MAP_PRIVATE, PAGE_SIZE, EBADFD};
 use syscall::data::{Map, Stat, TimeSpec};
-use syscall::error::{Error, Result, EBADF, EINVAL, EISDIR, ENOMEM, EPERM};
+use syscall::error::{Error, Result, EBADF, EINVAL, EISDIR, EPERM};
 use syscall::flag::{
     MapFlags, F_GETFL, F_SETFL, MODE_PERM, O_ACCMODE, O_APPEND, O_RDONLY, O_RDWR, O_WRONLY,
     PROT_READ, PROT_WRITE, SEEK_CUR, SEEK_END, SEEK_SET,
 };
+use syscall::{EBADFD, PAGE_SIZE};
 
 use crate::{Disk, Node, Transaction, TreePtr};
 
@@ -34,9 +34,22 @@ pub trait Resource<D: Disk> {
 
     fn seek(&mut self, offset: isize, whence: usize, tx: &mut Transaction<D>) -> Result<isize>;
 
-    fn fmap(&mut self, fmaps: &mut Fmaps, flags: MapFlags, size: usize, offset: u64, tx: &mut Transaction<D>) -> Result<usize>;
+    fn fmap(
+        &mut self,
+        fmaps: &mut Fmaps,
+        flags: MapFlags,
+        size: usize,
+        offset: u64,
+        tx: &mut Transaction<D>,
+    ) -> Result<usize>;
 
-    fn funmap(&mut self, fmaps: &mut Fmaps, offset: u64, size: usize, tx: &mut Transaction<D>) -> Result<usize>;
+    fn funmap(
+        &mut self,
+        fmaps: &mut Fmaps,
+        offset: u64,
+        size: usize,
+        tx: &mut Transaction<D>,
+    ) -> Result<usize>;
 
     fn fchmod(&mut self, mode: u16, tx: &mut Transaction<D>) -> Result<usize> {
         let mut node = tx.read_tree(self.node_ptr())?;
@@ -208,10 +221,23 @@ impl<D: Disk> Resource<D> for DirResource {
         Ok(self.seek)
     }
 
-    fn fmap(&mut self, _fmaps: &mut Fmaps, _flags: MapFlags, _size: usize, _offset: u64, _tx: &mut Transaction<D>) -> Result<usize> {
+    fn fmap(
+        &mut self,
+        _fmaps: &mut Fmaps,
+        _flags: MapFlags,
+        _size: usize,
+        _offset: u64,
+        _tx: &mut Transaction<D>,
+    ) -> Result<usize> {
         Err(Error::new(EBADF))
     }
-    fn funmap(&mut self, _fmaps: &mut Fmaps, _offset: u64, _size: usize, _tx: &mut Transaction<D>) -> Result<usize> {
+    fn funmap(
+        &mut self,
+        _fmaps: &mut Fmaps,
+        _offset: u64,
+        _size: usize,
+        _tx: &mut Transaction<D>,
+    ) -> Result<usize> {
         Err(Error::new(EBADF))
     }
 
@@ -263,13 +289,8 @@ impl Fmap {
 
         let buf = slice::from_raw_parts_mut(address, unaligned_size);
 
-        let count = match tx.read_node(
-            node_ptr,
-            offset,
-            buf,
-            atime.as_secs(),
-            atime.subsec_nanos(),
-        ) {
+        let count = match tx.read_node(node_ptr, offset, buf, atime.as_secs(), atime.subsec_nanos())
+        {
             Ok(ok) => ok,
             Err(err) => {
                 let _ = syscall::funmap(address as usize, aligned_size);
@@ -287,7 +308,14 @@ impl Fmap {
         })
     }
 
-    pub unsafe fn sync<D: Disk>(&mut self, node_ptr: TreePtr<Node>, base: *mut u8, offset: u64, size: usize, tx: &mut Transaction<D>) -> Result<()> {
+    pub unsafe fn sync<D: Disk>(
+        &mut self,
+        node_ptr: TreePtr<Node>,
+        base: *mut u8,
+        offset: u64,
+        size: usize,
+        tx: &mut Transaction<D>,
+    ) -> Result<()> {
         if self.flags & PROT_WRITE == PROT_WRITE {
             let mtime = SystemTime::now().duration_since(UNIX_EPOCH).unwrap();
             tx.write_node(
@@ -426,7 +454,14 @@ impl<D: Disk> Resource<D> for FileResource {
         Ok(self.seek)
     }
 
-    fn fmap(&mut self, fmaps: &mut Fmaps, flags: MapFlags, unaligned_size: usize, offset: u64, tx: &mut Transaction<D>) -> Result<usize> {
+    fn fmap(
+        &mut self,
+        fmaps: &mut Fmaps,
+        flags: MapFlags,
+        unaligned_size: usize,
+        offset: u64,
+        tx: &mut Transaction<D>,
+    ) -> Result<usize> {
         //dbg!(&self.fmaps);
         let accmode = self.flags & O_ACCMODE;
         if flags.contains(PROT_READ) && !(accmode == O_RDWR || accmode == O_RDONLY) {
@@ -444,21 +479,28 @@ impl<D: Disk> Resource<D> for FileResource {
 
         // TODO: Pass entry directory to Resource trait functions, since the node_ptr can be
         // obtained by the caller.
-        let fmap_info = fmaps.get_mut(&self.node_ptr.id()).ok_or(Error::new(EBADFD))?;
+        let fmap_info = fmaps
+            .get_mut(&self.node_ptr.id())
+            .ok_or(Error::new(EBADFD))?;
 
         let new_size = (offset as usize + aligned_size).next_multiple_of(PAGE_SIZE);
         if new_size > fmap_info.size {
             fmap_info.base = if fmap_info.base.is_null() {
                 unsafe {
-                    syscall::fmap(!0, &Map {
-                        size: new_size,
-                        // PRIVATE/SHARED doesn't matter once the pages are passed in the fmap
-                        // handler.
-                        flags: MapFlags::PROT_READ | MapFlags::PROT_WRITE | MapFlags::MAP_PRIVATE,
-
-                        offset: 0,
-                        address: 0,
-                    })? as *mut u8
+                    syscall::fmap(
+                        !0,
+                        &Map {
+                            size: new_size,
+                            // PRIVATE/SHARED doesn't matter once the pages are passed in the fmap
+                            // handler.
+                            flags: MapFlags::PROT_READ
+                                | MapFlags::PROT_WRITE
+                                | MapFlags::MAP_PRIVATE,
+
+                            offset: 0,
+                            address: 0,
+                        },
+                    )? as *mut u8
                 }
             } else {
                 unsafe {
@@ -468,14 +510,16 @@ impl<D: Disk> Resource<D> for FileResource {
                         fmap_info.size,
                         0,
                         new_size,
-                        syscall::MremapFlags::empty().bits() | (PROT_READ | PROT_WRITE).bits()
+                        syscall::MremapFlags::empty().bits() | (PROT_READ | PROT_WRITE).bits(),
                     )? as *mut u8
                 }
             };
             fmap_info.size = new_size;
         }
 
-        let affected_fmaps = fmap_info.ranges.remove_and_unused(offset..offset + aligned_size as u64);
+        let affected_fmaps = fmap_info
+            .ranges
+            .remove_and_unused(offset..offset + aligned_size as u64);
 
         for (range, v_opt) in affected_fmaps {
             //dbg!(&range);
@@ -483,9 +527,20 @@ impl<D: Disk> Resource<D> for FileResource {
                 fmap.rc += 1;
                 fmap.flags |= flags;
 
-                fmap_info.ranges.insert(range.start, range.end - range.start, fmap);
+                fmap_info
+                    .ranges
+                    .insert(range.start, range.end - range.start, fmap);
             } else {
-                let map = unsafe { Fmap::new(self.node_ptr, flags, unaligned_size, offset, fmap_info.base, tx)? };
+                let map = unsafe {
+                    Fmap::new(
+                        self.node_ptr,
+                        flags,
+                        unaligned_size,
+                        offset,
+                        fmap_info.base,
+                        tx,
+                    )?
+                };
                 fmap_info.ranges.insert(offset, aligned_size as u64, map);
             }
         }
@@ -494,11 +549,20 @@ impl<D: Disk> Resource<D> for FileResource {
         Ok(fmap_info.base as usize + offset as usize)
     }
 
-    fn funmap(&mut self, fmaps: &mut Fmaps, offset: u64, size: usize, tx: &mut Transaction<D>) -> Result<usize> {
-        let fmap_info = fmaps.get_mut(&self.node_ptr.id()).ok_or(Error::new(EBADFD))?;
+    fn funmap(
+        &mut self,
+        fmaps: &mut Fmaps,
+        offset: u64,
+        size: usize,
+        tx: &mut Transaction<D>,
+    ) -> Result<usize> {
+        let fmap_info = fmaps
+            .get_mut(&self.node_ptr.id())
+            .ok_or(Error::new(EBADFD))?;
 
         //dbg!(&self.fmaps);
         //dbg!(self.fmaps.conflicts(offset..offset + size as u64).collect::<Vec<_>>());
+        #[allow(unused_mut)]
         let mut affected_fmaps = fmap_info.ranges.remove(offset..offset + size as u64);
 
         for (range, mut fmap) in affected_fmaps {
@@ -506,11 +570,19 @@ impl<D: Disk> Resource<D> for FileResource {
 
             //log::info!("SYNCING {}..{}", range.start, range.end);
             unsafe {
-                fmap.sync(self.node_ptr, fmap_info.base, range.start, (range.end - range.start) as usize, tx)?;
+                fmap.sync(
+                    self.node_ptr,
+                    fmap_info.base,
+                    range.start,
+                    (range.end - range.start) as usize,
+                    tx,
+                )?;
             }
 
             if fmap.rc > 0 {
-                fmap_info.ranges.insert(range.start, range.end - range.start, fmap);
+                fmap_info
+                    .ranges
+                    .insert(range.start, range.end - range.start, fmap);
             }
         }
         //dbg!(&self.fmaps);
@@ -537,7 +609,13 @@ impl<D: Disk> Resource<D> for FileResource {
         if let Some(fmap_info) = fmaps.get_mut(&self.node_ptr.id()) {
             for (range, fmap) in fmap_info.ranges.iter_mut() {
                 unsafe {
-                    fmap.sync(self.node_ptr, fmap_info.base, range.start, (range.end - range.start) as usize, tx)?;
+                    fmap.sync(
+                        self.node_ptr,
+                        fmap_info.base,
+                        range.start,
+                        (range.end - range.start) as usize,
+                        tx,
+                    )?;
                 }
             }
         }
@@ -623,9 +701,15 @@ impl range_tree::Value for Fmap {
             Err(self)
         }
     }
-    fn split(self, prev_range: Option<core::ops::Range<Self::K>>, range: core::ops::Range<Self::K>, next_range: Option<core::ops::Range<Self::K>>) -> (Option<Self>, Self, Option<Self>) {
+    #[allow(unused_variables)]
+    fn split(
+        self,
+        prev_range: Option<core::ops::Range<Self::K>>,
+        range: core::ops::Range<Self::K>,
+        next_range: Option<core::ops::Range<Self::K>>,
+    ) -> (Option<Self>, Self, Option<Self>) {
         (
-            prev_range.map(|range| Fmap {
+            prev_range.map(|_range| Fmap {
                 rc: self.rc,
                 flags: self.flags,
                 last_page_tail: 0,
@@ -633,9 +717,13 @@ impl range_tree::Value for Fmap {
             Fmap {
                 rc: self.rc,
                 flags: self.flags,
-                last_page_tail: if next_range.is_none() { self.last_page_tail } else { 0 },
+                last_page_tail: if next_range.is_none() {
+                    self.last_page_tail
+                } else {
+                    0
+                },
             },
-            next_range.map(|range| Fmap {
+            next_range.map(|_range| Fmap {
                 rc: self.rc,
                 flags: self.flags,
                 last_page_tail: self.last_page_tail,
diff --git a/src/mount/redox/scheme.rs b/src/mount/redox/scheme.rs
index 0e53709..e8bed08 100644
--- a/src/mount/redox/scheme.rs
+++ b/src/mount/redox/scheme.rs
@@ -3,8 +3,7 @@ use std::str;
 use std::sync::atomic::{AtomicUsize, Ordering};
 use std::time::{SystemTime, UNIX_EPOCH};
 
-use syscall::{EBADFD, MunmapFlags};
-use syscall::data::{Map, Stat, StatVfs, TimeSpec};
+use syscall::data::{Stat, StatVfs, TimeSpec};
 use syscall::error::{
     Error, Result, EACCES, EBADF, EBUSY, EEXIST, EINVAL, EISDIR, ELOOP, ENOENT, ENOTDIR, ENOTEMPTY,
     EPERM, EXDEV,
@@ -14,6 +13,7 @@ use syscall::flag::{
     O_RDWR, O_STAT, O_SYMLINK, O_TRUNC, O_WRONLY,
 };
 use syscall::scheme::SchemeMut;
+use syscall::{MunmapFlags, EBADFD};
 
 use crate::{Disk, FileSystem, Node, Transaction, TreeData, TreePtr, BLOCK_SIZE};
 
@@ -46,12 +46,18 @@ impl<D: Disk> FileScheme<D> {
         url: &str,
         node: TreeData<Node>,
         nodes: &mut Vec<(TreeData<Node>, String)>,
-    ) -> Result<Vec<u8>> {
+    ) -> Result<String> {
         let atime = SystemTime::now().duration_since(UNIX_EPOCH).unwrap();
+        let scheme = format!("{}:", scheme_name);
 
+        // symbolic link is relative to this part of the url
+        let mut working_dir = dirname(url).unwrap_or(&scheme).to_string();
+        // node of the link
         let mut node = node;
+
         for _ in 0..32 {
             // XXX What should the limit be?
+            assert!(node.data().is_symlink());
             let mut buf = [0; 4096];
             let count = tx.read_node(
                 node.ptr(),
@@ -60,24 +66,25 @@ impl<D: Disk> FileScheme<D> {
                 atime.as_secs(),
                 atime.subsec_nanos(),
             )?;
-            let scheme = format!("{}:", scheme_name);
-            let canon = canonicalize(url.as_bytes(), &buf[0..count]);
-            let path = str::from_utf8(&canon[scheme.len()..])
-                .unwrap_or("")
-                .trim_matches('/');
+            let target = canonicalize(
+                &working_dir,
+                str::from_utf8(&buf[..count]).map_err(|_| Error::new(EINVAL))?,
+            )
+            .ok_or(Error::new(EINVAL))?;
+            if !target.starts_with(&scheme) {
+                return Err(Error::new(EXDEV));
+            }
+            let target_path = &target[scheme.len()..];
             nodes.clear();
             if let Some((next_node, next_node_name)) =
-                Self::path_nodes(scheme_name, tx, path, uid, gid, nodes)?
+                Self::path_nodes(scheme_name, tx, target_path, uid, gid, nodes)?
             {
                 if !next_node.data().is_symlink() {
-                    if canon.starts_with(scheme.as_bytes()) {
-                        nodes.push((next_node, next_node_name));
-                        return Ok(canon[scheme.len()..].to_vec());
-                    } else {
-                        return Err(Error::new(EXDEV));
-                    }
+                    nodes.push((next_node, next_node_name));
+                    return Ok(target_path.to_string());
                 }
                 node = next_node;
+                working_dir = dirname(&target).ok_or(Error::new(EINVAL))?.to_string();
             } else {
                 return Err(Error::new(ENOENT));
             }
@@ -141,42 +148,62 @@ impl<D: Disk> FileScheme<D> {
     }
 }
 
+/// given a path with a prefix, return the containing directory (or scheme)
+fn dirname(path: &str) -> Option<&str> {
+    if let Some(separator_index) = path.trim_end_matches('/').rfind('/') {
+        return Some(&path[..=separator_index]);
+    }
+    // there was no path separator, just use the scheme
+    if let Some(scheme) = scheme_with_separator(path) {
+        return Some(scheme);
+    }
+    None
+}
+
+/// given a path with a scheme prefix, return the prefix including the separator
+fn scheme_with_separator(path: &str) -> Option<&str> {
+    if let Some(sep_idx) = path.find(':') {
+        let scheme = &path[..=sep_idx];
+        if scheme.find('/').is_none() {
+            return Some(&scheme);
+        }
+    }
+    None
+}
+
 /// Make a relative path absolute
 /// Given a cwd of "scheme:/path"
 /// This function will turn "foo" into "scheme:/path/foo"
 /// "/foo" will turn into "scheme:/foo"
 /// "bar:/foo" will be used directly, as it is already absolute
-pub fn canonicalize(current: &[u8], path: &[u8]) -> Vec<u8> {
+pub fn canonicalize(current: &str, path: &str) -> Option<String> {
     // This function is modified from a version in the kernel
-    let mut canon = if path.iter().position(|&b| b == b':').is_none() {
-        let cwd = &current[0..current.iter().rposition(|x| *x == '/' as u8).unwrap_or(0)];
-
-        let mut canon = if !path.starts_with(b"/") {
-            let mut c = cwd.to_vec();
-            if !c.ends_with(b"/") {
-                c.push(b'/');
-            }
-            c
+    let mut canon = if scheme_with_separator(path).is_none() {
+        let mut canon = if !path.starts_with('/') {
+            let mut cwd = current.trim_end_matches('/').to_string();
+            cwd.push('/');
+            cwd
         } else {
-            cwd[..cwd.iter().position(|&b| b == b':').map_or(1, |i| i + 1)].to_vec()
+            scheme_with_separator(current)?.to_string()
         };
 
-        canon.extend_from_slice(&path);
+        canon.push_str(path);
         canon
     } else {
-        path.to_vec()
+        path.to_string()
     };
 
     // NOTE: assumes the scheme does not include anything like "../" or "./"
     let mut result = {
         let parts = canon
-            .split(|&c| c == b'/')
-            .filter(|&part| part != b".")
+            .split('/')
             .rev()
             .scan(0, |nskip, part| {
-                if part == b"." {
+                if part == "" {
+                    Some(None)
+                } else if part == "." {
                     Some(None)
-                } else if part == b".." {
+                } else if part == ".." {
                     *nskip += 1;
                     Some(None)
                 } else {
@@ -189,25 +216,22 @@ pub fn canonicalize(current: &[u8], path: &[u8]) -> Vec<u8> {
                 }
             })
             .filter_map(|x| x)
+            .filter(|x| !x.is_empty())
             .collect::<Vec<_>>();
-        parts.iter().rev().fold(Vec::new(), |mut vec, &part| {
-            vec.extend_from_slice(part);
-            vec.push(b'/');
-            vec
+        parts.iter().rev().fold(String::new(), |mut string, &part| {
+            string.push_str(part);
+            string.push('/');
+            string
         })
     };
     result.pop(); // remove extra '/'
 
     // replace with the root of the scheme if it's empty
     if result.len() == 0 {
-        let pos = canon
-            .iter()
-            .position(|&b| b == b':')
-            .map_or(canon.len(), |p| p + 1);
-        canon.truncate(pos);
-        canon
+        canon.truncate(scheme_with_separator(&canon)?.len());
+        Some(canon)
     } else {
-        result
+        Some(result)
     }
 }
 
@@ -283,9 +307,7 @@ impl<D: Disk> SchemeMut for FileScheme<D> {
                             &mut resolve_nodes,
                         )
                     })?;
-                    let resolved_utf8 =
-                        str::from_utf8(&resolved).map_err(|_| Error::new(EINVAL))?;
-                    return self.open(resolved_utf8, flags, uid, gid);
+                    return self.open(&resolved, flags, uid, gid);
                 } else if !node.data().is_symlink() && flags & O_SYMLINK == O_SYMLINK {
                     return Err(Error::new(EINVAL));
                 } else {
@@ -401,8 +423,10 @@ impl<D: Disk> SchemeMut for FileScheme<D> {
                 }
             }
         };
-        self.fmap.entry(resource.node_ptr().id()).or_insert_with(Default::default).open_fds += 1;
-
+        self.fmap
+            .entry(resource.node_ptr().id())
+            .or_insert_with(Default::default)
+            .open_fds += 1;
 
         let id = self.next_id.fetch_add(1, Ordering::SeqCst);
         self.files.insert(id, resource);
@@ -536,7 +560,10 @@ impl<D: Disk> SchemeMut for FileScheme<D> {
             return Err(Error::new(EBADF));
         };
 
-        self.fmap.get_mut(&resource.node_ptr().id()).ok_or(Error::new(EBADFD))?.open_fds += 1;
+        self.fmap
+            .get_mut(&resource.node_ptr().id())
+            .ok_or(Error::new(EBADFD))?
+            .open_fds += 1;
         let id = self.next_id.fetch_add(1, Ordering::SeqCst);
         self.files.insert(id, resource);
 
@@ -790,6 +817,7 @@ impl<D: Disk> SchemeMut for FileScheme<D> {
 
         self.fs.tx(|tx| file.fmap(fmaps, flags, size, offset, tx))
     }
+    #[allow(unused_variables)]
     fn munmap(&mut self, id: usize, offset: u64, size: usize, flags: MunmapFlags) -> Result<usize> {
         println!("Munmap {}, {} {}", id, size, offset);
         let file = self.files.get_mut(&id).ok_or(Error::new(EBADF))?;
@@ -801,9 +829,15 @@ impl<D: Disk> SchemeMut for FileScheme<D> {
     fn close(&mut self, id: usize) -> Result<usize> {
         // println!("Close {}", id);
         let file = self.files.remove(&id).ok_or(Error::new(EBADF))?;
-        let file_info = self.fmap.get_mut(&file.node_ptr().id()).ok_or(Error::new(EBADFD))?;
-
-        file_info.open_fds = file_info.open_fds.checked_sub(1).expect("open_fds not tracked correctly");
+        let file_info = self
+            .fmap
+            .get_mut(&file.node_ptr().id())
+            .ok_or(Error::new(EBADFD))?;
+
+        file_info.open_fds = file_info
+            .open_fds
+            .checked_sub(1)
+            .expect("open_fds not tracked correctly");
 
         // TODO: If open_fds reaches zero and there are no hardlinks (directory entries) to any
         // particular inode, remove that inode here.
-- 
GitLab