Jika/redox-kernel
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Set address space/files when closing, not writing.

Browse Source 
This fixes file descriptor leaks. Suppose relibc is just about to set
the address space. For this, it needs to write the address space fd to
the selection fd. To avoid having to close them in the kernel, it rather
memorizes what the file descriptors refer to internally, and then do the
actual operation when they are gone, i.e. when closing.
This commit is contained in:
4lDO2
2022-07-07 12:11:58 +02:00
parent fa48c7aa97
commit 240d91f951
3 changed files with 56 additions and 50 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
src/scheme/mod.rs
View File

@@ -302,11 +302,6 @@ pub fn schemes_mut() -> RwLockWriteGuard<'static, SchemeList> {
#[allow(unused_variables)]
pub trait KernelScheme: Scheme + Send + Sync + 'static {
fn kfmap(&self, number: usize, map: &syscall::data::Map, target_context: &Arc<RwLock<Context>>) -> Result<usize> {
log::error!("Returning ENOSYS since kfmap can only be called on UserScheme schemes");
Err(Error::new(ENOSYS))
}
fn as_filetable(&self, number: usize) -> Result<Arc<RwLock<Vec<Option<FileDescriptor>>>>> {
Err(Error::new(EBADF))
}

83
src/scheme/proc.rs
View File

@@ -120,7 +120,20 @@ enum Operation {
Filetable { filetable: Arc<RwLock<Vec<Option<FileDescriptor>>>> },
AddrSpace { addrspace: Arc<RwLock<AddrSpace>> },
CurrentAddrSpace,
// "operations CAN change". The reason we split changing the address space into two handle
// types, is that we would rather want the actual switch to occur when closing, as opposed to
// when writing. This is so that we can actually guarantee that no file descriptors are leaked.
AwaitingAddrSpaceChange {
new: Arc<RwLock<AddrSpace>>,
new_sp: usize,
new_ip: usize,
},
CurrentFiletable,
AwaitingFiletableChange(Arc<RwLock<Vec<Option<FileDescriptor>>>>),
// TODO: Remove this once openat is implemented, or allow openat-via-dup via e.g. the top-level
// directory.
OpenViaDup,
@@ -530,8 +543,7 @@ impl Scheme for ProcScheme {
// TODO: Define a struct somewhere?
const RECORD_SIZE: usize = mem::size_of::<usize>() * 4;
let start = core::cmp::min(buf.len(), *offset);
let records = buf[start..].array_chunks_mut::<RECORD_SIZE>();
let records = buf.array_chunks_mut::<RECORD_SIZE>();
let addrspace = addrspace.read();
let mut bytes_read = 0;
@@ -682,8 +694,7 @@ impl Scheme for ProcScheme {
// TODO: Find a better way to switch address spaces, since they also require switching
// the instruction and stack pointer. Maybe remove `<pid>/regs` altogether and replace it
// with `<pid>/ctx`
Operation::CurrentAddrSpace | Operation::CurrentFiletable => return Err(Error::new(EBADF)),
Operation::OpenViaDup | Operation::GrantHandle { .. } => return Err(Error::new(EBADF)),
_ => return Err(Error::new(EBADF)),
}
}
@@ -996,12 +1007,8 @@ impl Scheme for ProcScheme {
let mut filetable = hopefully_this_scheme.as_filetable(number)?;
let stopper = if info.pid == context::context_id() { with_context_mut } else { try_stop_context };
self.handles.write().get_mut(&id).ok_or(Error::new(EBADF))?.info.operation = Operation::AwaitingFiletableChange(filetable);
stopper(info.pid, |context: &mut Context| {
context.files = filetable;
Ok(())
})?;
Ok(mem::size_of::<usize>())
}
Operation::CurrentAddrSpace { .. } => {
@@ -1013,25 +1020,11 @@ impl Scheme for ProcScheme {
let (hopefully_this_scheme, number) = extract_scheme_number(addrspace_fd)?;
let space = hopefully_this_scheme.as_addrspace(number)?;
let callback = |context: &mut Context| unsafe {
if let Some(saved_regs) = ptrace::regs_for_mut(context) {
saved_regs.iret.rip = ip;
saved_regs.iret.rsp = sp;
} else {
context.clone_entry = Some([ip, sp]);
}
self.handles.write().get_mut(&id).ok_or(Error::new(EBADF))?.info.operation = Operation::AwaitingAddrSpaceChange { new: space, new_sp: sp, new_ip: ip };
context.set_addr_space(space);
Ok(())
};
if info.pid == context::context_id() {
with_context_mut(info.pid, callback)?;
} else {
try_stop_context(info.pid, callback)?;
}
Ok(3 * mem::size_of::<usize>())
}
Operation::OpenViaDup | Operation::GrantHandle { .. } => return Err(Error::new(EBADF)),
_ => return Err(Error::new(EBADF)),
}
}
@@ -1080,7 +1073,7 @@ impl Scheme for ProcScheme {
Operation::CurrentFiletable => "current-filetable",
Operation::OpenViaDup => "open-via-dup",
Operation::GrantHandle { .. } => return Err(Error::new(EOPNOTSUPP)),
_ => return Err(Error::new(EOPNOTSUPP)),
});
read_from(buf, &path.as_bytes(), &mut 0)
@@ -1111,18 +1104,38 @@ impl Scheme for ProcScheme {
let mut handle = self.handles.write().remove(&id).ok_or(Error::new(EBADF))?;
handle.continue_ignored_children();
if let Operation::Trace = handle.info.operation {
ptrace::close_session(handle.info.pid);
let stop_context = if handle.info.pid == context::context_id() { with_context_mut } else { try_stop_context };
if handle.info.flags & O_EXCL == O_EXCL {
syscall::kill(handle.info.pid, SIGKILL)?;
}
match handle.info.operation {
Operation::AwaitingAddrSpaceChange { new, new_sp, new_ip } => stop_context(handle.info.pid, |context: &mut Context| unsafe {
if let Some(saved_regs) = ptrace::regs_for_mut(context) {
saved_regs.iret.rip = new_ip;
saved_regs.iret.rsp = new_sp;
} else {
context.clone_entry = Some([new_ip, new_sp]);
}
let contexts = context::contexts();
if let Some(context) = contexts.get(handle.info.pid) {
let mut context = context.write();
context.ptrace_stop = false;
context.set_addr_space(new);
Ok(())
})?,
Operation::AwaitingFiletableChange(new) => with_context_mut(handle.info.pid, |context: &mut Context| {
context.files = new;
Ok(())
})?,
Operation::Trace => {
ptrace::close_session(handle.info.pid);
if handle.info.flags & O_EXCL == O_EXCL {
syscall::kill(handle.info.pid, SIGKILL)?;
}
let contexts = context::contexts();
if let Some(context) = contexts.get(handle.info.pid) {
let mut context = context.write();
context.ptrace_stop = false;
}
}
_ => (),
}
Ok(0)
}

18
src/scheme/user.rs
View File

@@ -279,9 +279,13 @@ impl UserInner {
Ok(0)
}
fn fmap_inner(&self, file: usize, map: &Map, context_lock: &Arc<RwLock<Context>>) -> Result<usize> {
fn fmap_inner(&self, file: usize, map: &Map) -> Result<usize> {
let (pid, uid, gid, context_weak, desc) = {
let context_lock = Arc::clone(context::contexts().current().ok_or(Error::new(ESRCH))?);
let context = context_lock.read();
if map.size % PAGE_SIZE != 0 {
log::warn!("Unaligned map size for context {:?}", context.name.try_read().as_deref());
}
// TODO: Faster, cleaner mechanism to get descriptor
let scheme = self.scheme_id.load(Ordering::SeqCst);
let mut desc_res = Err(Error::new(EBADF));
@@ -298,9 +302,8 @@ impl UserInner {
}
}
let desc = desc_res?;
(context.id, context.euid, context.egid, Arc::downgrade(context_lock), desc)
(context.id, context.euid, context.egid, Arc::downgrade(&context_lock), desc)
};
drop(context_lock);
let address = self.capture(map)?;
@@ -433,7 +436,7 @@ impl Scheme for UserScheme {
fn fmap(&self, file: usize, map: &Map) -> Result<usize> {
let inner = self.inner.upgrade().ok_or(Error::new(ENODEV))?;
inner.fmap_inner(file, map, &Arc::clone(context::contexts().current().ok_or(Error::new(ESRCH))?))
inner.fmap_inner(file, map)
}
fn funmap(&self, grant_address: usize, size: usize) -> Result<usize> {
@@ -535,9 +538,4 @@ impl Scheme for UserScheme {
inner.call(SYS_CLOSE, file, 0, 0)
}
}
impl crate::scheme::KernelScheme for UserScheme {
fn kfmap(&self, number: usize, map: &Map, target_context: &Arc<RwLock<Context>>) -> Result<usize> {
let inner = self.inner.upgrade().ok_or(Error::new(ENODEV))?;
inner.fmap_inner(number, map, target_context)
}
}
impl crate::scheme::KernelScheme for UserScheme {}