From ee5b5a0bfa6086579cb3207d5390774732756a01 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Sun, 13 Apr 2025 01:52:52 +0200 Subject: [PATCH] Use NonNull to represent valid sys handles --- src/buffer.rs | 42 +++++++++++++++++++++++++----------------- src/memchr.rs | 2 +- src/sys/unix.rs | 38 ++++++++++++++++---------------------- src/sys/windows.rs | 34 +++++++++++++++------------------- 4 files changed, 57 insertions(+), 59 deletions(-) diff --git a/src/buffer.rs b/src/buffer.rs index fe9bcb9..128954e 100644 --- a/src/buffer.rs +++ b/src/buffer.rs @@ -31,7 +31,7 @@ use std::io::Write as _; use std::mem::{self, MaybeUninit}; use std::ops::{Deref, DerefMut, Range}; use std::path::Path; -use std::ptr; +use std::ptr::{self, NonNull}; use std::rc::Rc; use std::slice; use std::str; @@ -2250,7 +2250,7 @@ impl TextBuffer { } enum BackingBuffer { - VirtualMemory(*mut u8, usize), + VirtualMemory(NonNull, usize), Vec(Vec), } @@ -2270,7 +2270,7 @@ impl Drop for BackingBuffer { /// This variant is optimized for large buffers and uses virtual memory. pub struct GapBuffer { /// Pointer to the buffer. - text: *mut u8, + text: NonNull, /// Maximum size of the buffer, including gap. reserve: usize, /// Size of the buffer, including gap. @@ -2296,9 +2296,8 @@ impl GapBuffer { let text; if small { - let mut v = Vec::new(); - text = v.as_mut_ptr(); - buffer = BackingBuffer::Vec(v); + text = NonNull::dangling(); + buffer = BackingBuffer::Vec(Vec::new()); } else { text = unsafe { sys::virtual_reserve(RESERVE)? }; buffer = BackingBuffer::VirtualMemory(text, RESERVE); @@ -2355,10 +2354,15 @@ impl GapBuffer { let move_dst = if left { off + gap_len } else { gap_off }; let move_len = if left { gap_off - off } else { off - gap_off }; - unsafe { ptr::copy(data.add(move_src), data.add(move_dst), move_len) }; + unsafe { + data.add(move_src) + .copy_to_nonoverlapping(data.add(move_dst), move_len) + }; if cfg!(debug_assertions) { - unsafe { slice::from_raw_parts_mut(data.add(off), gap_len).fill(0xCD) }; + unsafe { + slice::from_raw_parts_mut(data.add(off).as_ptr(), gap_len).fill(0xCD) + }; } } @@ -2368,7 +2372,8 @@ impl GapBuffer { // Delete the text if cfg!(debug_assertions) { unsafe { - slice::from_raw_parts_mut(self.text.add(off + self.gap_len), delete).fill(0xCD) + slice::from_raw_parts_mut(self.text.add(off + self.gap_len).as_ptr(), delete) + .fill(0xCD) }; } self.gap_len += delete; @@ -2402,7 +2407,7 @@ impl GapBuffer { }, BackingBuffer::Vec(v) => { v.resize(bytes_new, 0); - self.text = v.as_mut_ptr(); + self.text = unsafe { NonNull::new_unchecked(v.as_mut_ptr()) }; } } @@ -2412,16 +2417,19 @@ impl GapBuffer { let gap_beg = unsafe { self.text.add(off) }; unsafe { ptr::copy( - gap_beg.add(gap_len_old), - gap_beg.add(gap_len_new), + gap_beg.add(gap_len_old).as_ptr(), + gap_beg.add(gap_len_new).as_ptr(), self.text_length - off, ) }; if cfg!(debug_assertions) { unsafe { - slice::from_raw_parts_mut(gap_beg.add(gap_len_old), gap_len_new - gap_len_old) - .fill(0xCD) + slice::from_raw_parts_mut( + gap_beg.add(gap_len_old).as_ptr(), + gap_len_new - gap_len_old, + ) + .fill(0xCD) }; } @@ -2429,7 +2437,7 @@ impl GapBuffer { } self.generation = self.generation.wrapping_add(1); - unsafe { slice::from_raw_parts_mut(self.text.add(off), len) } + unsafe { slice::from_raw_parts_mut(self.text.add(off).as_ptr(), len) } } fn commit_gap(&mut self, len: usize) { @@ -2514,7 +2522,7 @@ impl Document for GapBuffer { len = self.text_length - off; } - unsafe { slice::from_raw_parts(self.text.add(beg), len) } + unsafe { slice::from_raw_parts(self.text.add(beg).as_ptr(), len) } } fn read_backward(&self, off: usize) -> &[u8] { @@ -2534,7 +2542,7 @@ impl Document for GapBuffer { len = off - self.gap_off; } - unsafe { slice::from_raw_parts(self.text.add(beg), len) } + unsafe { slice::from_raw_parts(self.text.add(beg).as_ptr(), len) } } } diff --git a/src/memchr.rs b/src/memchr.rs index 40fec8f..67afa9d 100644 --- a/src/memchr.rs +++ b/src/memchr.rs @@ -474,7 +474,7 @@ mod tests { // 3 pages: uncommitted, committed, uncommitted let ptr = sys::virtual_reserve(page_size * 3).unwrap(); sys::virtual_commit(ptr.add(page_size), page_size).unwrap(); - slice::from_raw_parts_mut(ptr.add(page_size), page_size) + slice::from_raw_parts_mut(ptr.add(page_size).as_ptr(), page_size) }; page.fill(b'a'); diff --git a/src/sys/unix.rs b/src/sys/unix.rs index 5d3da0e..8a4ec42 100644 --- a/src/sys/unix.rs +++ b/src/sys/unix.rs @@ -3,11 +3,9 @@ use crate::helpers; use std::borrow::Cow; use std::ffi::{CStr, CString, c_int, c_void}; use std::fs::{self, File}; -use std::mem; -use std::mem::MaybeUninit; +use std::mem::{self, MaybeUninit}; use std::os::fd::FromRawFd; -use std::ptr; -use std::ptr::{null, null_mut}; +use std::ptr::{self, NonNull, null, null_mut}; use std::thread; use std::time; @@ -323,7 +321,7 @@ pub fn open_stdin_if_redirected() -> Option { /// /// This function is unsafe because it uses raw pointers. /// Don't forget to release the memory when you're done with it or you'll leak it. -pub unsafe fn virtual_reserve(size: usize) -> apperr::Result<*mut u8> { +pub unsafe fn virtual_reserve(size: usize) -> apperr::Result> { unsafe { let ptr = libc::mmap( null_mut(), @@ -333,10 +331,10 @@ pub unsafe fn virtual_reserve(size: usize) -> apperr::Result<*mut u8> { -1, 0, ); - if ptr::eq(ptr, libc::MAP_FAILED) { + if ptr.is_null() || ptr::eq(ptr, libc::MAP_FAILED) { Err(errno_to_apperr(libc::ENOMEM)) } else { - Ok(ptr as *mut u8) + Ok(NonNull::new_unchecked(ptr as *mut u8)) } } } @@ -347,9 +345,9 @@ pub unsafe fn virtual_reserve(size: usize) -> apperr::Result<*mut u8> { /// /// This function is unsafe because it uses raw pointers. /// Make sure to only pass pointers acquired from `virtual_reserve`. -pub unsafe fn virtual_release(base: *mut u8, size: usize) { +pub unsafe fn virtual_release(base: NonNull, size: usize) { unsafe { - libc::munmap(base as *mut libc::c_void, size); + libc::munmap(base.cast().as_ptr(), size); } } @@ -360,10 +358,10 @@ pub unsafe fn virtual_release(base: *mut u8, size: usize) { /// This function is unsafe because it uses raw pointers. /// Make sure to only pass pointers acquired from `virtual_reserve` /// and to pass a size less than or equal to the size passed to `virtual_reserve`. -pub unsafe fn virtual_commit(base: *mut u8, size: usize) -> apperr::Result<()> { +pub unsafe fn virtual_commit(base: NonNull, size: usize) -> apperr::Result<()> { unsafe { let status = libc::mprotect( - base as *mut libc::c_void, + base.cast().as_ptr(), size, libc::PROT_READ | libc::PROT_WRITE, ); @@ -375,14 +373,10 @@ pub unsafe fn virtual_commit(base: *mut u8, size: usize) -> apperr::Result<()> { } } -unsafe fn load_library(name: &CStr) -> apperr::Result<*mut c_void> { +unsafe fn load_library(name: &CStr) -> apperr::Result> { unsafe { - let handle = libc::dlopen(name.as_ptr(), libc::RTLD_LAZY); - if handle.is_null() { - Err(errno_to_apperr(libc::ELIBACC)) - } else { - Ok(handle) - } + NonNull::new(libc::dlopen(name.as_ptr(), libc::RTLD_LAZY)) + .ok_or_else(|| errno_to_apperr(libc::ELIBACC)) } } @@ -394,9 +388,9 @@ unsafe fn load_library(name: &CStr) -> apperr::Result<*mut c_void> { /// of the function you're loading. No type checks whatsoever are performed. // // It'd be nice to constrain T to std::marker::FnPtr, but that's unstable. -pub unsafe fn get_proc_address(handle: *mut c_void, name: &CStr) -> apperr::Result { +pub unsafe fn get_proc_address(handle: NonNull, name: &CStr) -> apperr::Result { unsafe { - let sym = libc::dlsym(handle, name.as_ptr()); + let sym = libc::dlsym(handle.as_ptr(), name.as_ptr()); if sym.is_null() { Err(errno_to_apperr(libc::ELIBACC)) } else { @@ -405,11 +399,11 @@ pub unsafe fn get_proc_address(handle: *mut c_void, name: &CStr) -> apperr::R } } -pub fn load_libicuuc() -> apperr::Result<*mut c_void> { +pub fn load_libicuuc() -> apperr::Result> { unsafe { load_library(c"libicuuc.so") } } -pub fn load_libicui18n() -> apperr::Result<*mut c_void> { +pub fn load_libicui18n() -> apperr::Result> { unsafe { load_library(c"libicui18n.so") } } diff --git a/src/sys/windows.rs b/src/sys/windows.rs index 9a553fb..0281821 100644 --- a/src/sys/windows.rs +++ b/src/sys/windows.rs @@ -1,12 +1,12 @@ use crate::helpers::{CoordType, Size}; use crate::{apperr, helpers}; -use std::ffi::{CStr, OsString}; +use std::ffi::{CStr, OsString, c_void}; use std::fmt::Write as _; use std::fs::{self, File}; use std::mem::MaybeUninit; use std::os::windows::io::FromRawHandle; use std::path::{Path, PathBuf}; -use std::ptr::{self, null, null_mut}; +use std::ptr::{self, NonNull, null, null_mut}; use std::{mem, time}; use windows_sys::Win32::Foundation; use windows_sys::Win32::Globalization; @@ -424,7 +424,7 @@ pub fn canonicalize>(path: P) -> apperr::Result { /// /// This function is unsafe because it uses raw pointers. /// Don't forget to release the memory when you're done with it or you'll leak it. -pub unsafe fn virtual_reserve(size: usize) -> apperr::Result<*mut u8> { +pub unsafe fn virtual_reserve(size: usize) -> apperr::Result> { unsafe { let mut base = null_mut(); @@ -449,9 +449,9 @@ pub unsafe fn virtual_reserve(size: usize) -> apperr::Result<*mut u8> { /// /// This function is unsafe because it uses raw pointers. /// Make sure to only pass pointers acquired from `virtual_reserve`. -pub unsafe fn virtual_release(base: *mut u8, size: usize) { +pub unsafe fn virtual_release(base: NonNull, size: usize) { unsafe { - Memory::VirtualFree(base as *mut _, size, Memory::MEM_RELEASE); + Memory::VirtualFree(base.as_ptr() as *mut _, size, Memory::MEM_RELEASE); } } @@ -462,10 +462,10 @@ pub unsafe fn virtual_release(base: *mut u8, size: usize) { /// This function is unsafe because it uses raw pointers. /// Make sure to only pass pointers acquired from `virtual_reserve` /// and to pass a size less than or equal to the size passed to `virtual_reserve`. -pub unsafe fn virtual_commit(base: *mut u8, size: usize) -> apperr::Result<()> { +pub unsafe fn virtual_commit(base: NonNull, size: usize) -> apperr::Result<()> { unsafe { check_ptr_return(Memory::VirtualAlloc( - base as *mut _, + base.as_ptr() as *mut _, size, Memory::MEM_COMMIT, Memory::PAGE_READWRITE, @@ -474,11 +474,11 @@ pub unsafe fn virtual_commit(base: *mut u8, size: usize) -> apperr::Result<()> { } } -unsafe fn get_module(name: *const u16) -> apperr::Result { +unsafe fn get_module(name: *const u16) -> apperr::Result> { unsafe { check_ptr_return(LibraryLoader::GetModuleHandleW(name)) } } -unsafe fn load_library(name: *const u16) -> apperr::Result { +unsafe fn load_library(name: *const u16) -> apperr::Result> { unsafe { check_ptr_return(LibraryLoader::LoadLibraryExW( name, @@ -496,9 +496,9 @@ unsafe fn load_library(name: *const u16) -> apperr::Result /// of the function you're loading. No type checks whatsoever are performed. // // It'd be nice to constrain T to std::marker::FnPtr, but that's unstable. -pub unsafe fn get_proc_address(handle: Foundation::HMODULE, name: &CStr) -> apperr::Result { +pub unsafe fn get_proc_address(handle: NonNull, name: &CStr) -> apperr::Result { unsafe { - let ptr = LibraryLoader::GetProcAddress(handle, name.as_ptr() as *const u8); + let ptr = LibraryLoader::GetProcAddress(handle.as_ptr(), name.as_ptr() as *const u8); if let Some(ptr) = ptr { Ok(mem::transmute_copy(&ptr)) } else { @@ -507,11 +507,11 @@ pub unsafe fn get_proc_address(handle: Foundation::HMODULE, name: &CStr) -> a } } -pub fn load_libicuuc() -> apperr::Result { +pub fn load_libicuuc() -> apperr::Result> { unsafe { load_library(w!("icuuc.dll")) } } -pub fn load_libicui18n() -> apperr::Result { +pub fn load_libicui18n() -> apperr::Result> { unsafe { load_library(w!("icuin.dll")) } } @@ -621,10 +621,6 @@ fn check_bool_return(ret: Foundation::BOOL) -> apperr::Result<()> { } } -fn check_ptr_return(ret: *mut T) -> apperr::Result<*mut T> { - if ret.is_null() { - Err(get_last_error()) - } else { - Ok(ret) - } +fn check_ptr_return(ret: *mut T) -> apperr::Result> { + NonNull::new(ret).ok_or_else(get_last_error) }