perf(fs): don't canonicalize path when opening file if --allow-all is passed (#28716)

Fixes #28702.

Super artificial benchmark:

```ts
const perf = performance;

async function asyncOpen() {
  const start = perf.now();
  for (let i = 0; i < 100_000; i++) {
    const file = await Deno.open("./foo.txt");
    file.close();
  }
  const end = perf.now();
  console.log(end - start);
}

function syncOpen() {
  const start = perf.now();
  for (let i = 0; i < 100_000; i++) {
    const file = Deno.openSync("./foo.txt");
    file.close();
  }
  const end = perf.now();
  console.log(end - start);
}

if (Deno.args[0]?.trim() === "async") {
  await asyncOpen();
} else {
  syncOpen();
}
```

Results (average of 10 for each):

```
deno sync               1785.59
deno-this-pr sync       491.69
deno async              1839.71
deno-this-pr async      528.78
```

---------

Co-authored-by: David Sherret <dsherret@users.noreply.github.com>
This commit is contained in:
Nathan Whitaker 2025-04-29 16:16:24 -07:00 committed by GitHub
parent 1a171f10df
commit c22d17824b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
14 changed files with 205 additions and 134 deletions

View file

@ -61,7 +61,7 @@ impl<TSys: DenoLibSys> NpmRegistryReadPermissionChecker<TSys> {
Ok(Cow::Borrowed(path))
} else {
permissions
.check_read_path(path)
.check_read_path(Cow::Borrowed(path))
.map_err(JsErrorBox::from_err)
}
}
@ -112,7 +112,7 @@ impl<TSys: DenoLibSys> NpmRegistryReadPermissionChecker<TSys> {
}
permissions
.check_read_path(path)
.check_read_path(Cow::Borrowed(path))
.map_err(JsErrorBox::from_err)
}
}

View file

@ -25,8 +25,8 @@ fn sync_permission_check<'a, P: FetchPermissions + 'static>(
permissions: &'a mut P,
api_name: &'static str,
) -> impl deno_fs::AccessCheckFn + 'a {
move |resolved, path, _options| {
permissions.check_read(resolved, path, api_name)
move |path, _options, _resolve| {
permissions.check_read(path, api_name, _resolve)
}
}

View file

@ -47,6 +47,7 @@ use deno_core::RcRef;
use deno_core::Resource;
use deno_core::ResourceId;
use deno_error::JsErrorBox;
use deno_fs::CheckedPath;
pub use deno_fs::FsError;
use deno_path_util::PathToUrlError;
use deno_permissions::PermissionCheckError;
@ -402,10 +403,10 @@ pub trait FetchPermissions {
#[must_use = "the resolved return value to mitigate time-of-check to time-of-use issues"]
fn check_read<'a>(
&mut self,
resolved: bool,
p: &'a Path,
path: Cow<'a, Path>,
api_name: &str,
) -> Result<Cow<'a, Path>, FsError>;
get_path: &'a dyn deno_fs::GetPath,
) -> Result<deno_fs::CheckedPath<'a>, FsError>;
}
impl FetchPermissions for deno_permissions::PermissionsContainer {
@ -421,23 +422,39 @@ impl FetchPermissions for deno_permissions::PermissionsContainer {
#[inline(always)]
fn check_read<'a>(
&mut self,
resolved: bool,
path: &'a Path,
path: Cow<'a, Path>,
api_name: &str,
) -> Result<Cow<'a, Path>, FsError> {
if resolved {
self
.check_special_file(path, api_name)
.map_err(FsError::NotCapable)?;
return Ok(Cow::Borrowed(path));
get_path: &'a dyn deno_fs::GetPath,
) -> Result<deno_fs::CheckedPath<'a>, FsError> {
if self.allows_all() {
return Ok(deno_fs::CheckedPath::Unresolved(path));
}
deno_permissions::PermissionsContainer::check_read_path(
let (needs_canonicalize, normalized_path) = get_path.normalized(path)?;
let path = deno_permissions::PermissionsContainer::check_read_path(
self,
path,
normalized_path,
Some(api_name),
)
.map_err(|_| FsError::NotCapable("read"))
.map_err(|_| FsError::NotCapable("read"))?;
let path = if needs_canonicalize {
let path = get_path.resolved(&path)?;
Cow::Owned(path)
} else {
path
};
self
.check_special_file(&path, api_name)
.map_err(FsError::NotCapable)?;
if needs_canonicalize {
Ok(CheckedPath::Resolved(path))
} else {
Ok(CheckedPath::Unresolved(path))
}
}
}

View file

@ -84,21 +84,26 @@ pub type FileSystemRc = crate::sync::MaybeArc<dyn FileSystem>;
pub trait AccessCheckFn:
for<'a> FnMut(
bool,
&'a Path,
Cow<'a, Path>,
&'a OpenOptions,
) -> FsResult<std::borrow::Cow<'a, Path>>
&'a dyn crate::GetPath,
) -> FsResult<CheckedPath<'a>>
{
}
impl<T> AccessCheckFn for T where
T: for<'a> FnMut(
bool,
&'a Path,
Cow<'a, Path>,
&'a OpenOptions,
) -> FsResult<std::borrow::Cow<'a, Path>>
&'a dyn crate::GetPath,
) -> FsResult<CheckedPath<'a>>
{
}
pub enum CheckedPath<'a> {
Resolved(Cow<'a, Path>),
Unresolved(Cow<'a, Path>),
}
pub type AccessCheckCb<'a> = &'a mut (dyn AccessCheckFn + 'a);
#[async_trait::async_trait(?Send)]

View file

@ -11,6 +11,7 @@ use std::path::PathBuf;
pub use deno_io::fs::FsError;
use deno_permissions::PermissionCheckError;
pub use interface::CheckedPath;
pub use crate::interface::AccessCheckCb;
pub use crate::interface::AccessCheckFn;
@ -31,12 +32,12 @@ pub use crate::sync::MaybeSync;
pub trait FsPermissions {
fn check_open<'a>(
&mut self,
resolved: bool,
read: bool,
write: bool,
path: &'a Path,
path: Cow<'a, Path>,
api_name: &str,
) -> Result<std::borrow::Cow<'a, Path>, FsError>;
get_path: &'a dyn GetPath,
) -> Result<CheckedPath<'a>, FsError>;
#[must_use = "the resolved return value to mitigate time-of-check to time-of-use issues"]
fn check_read(
&mut self,
@ -46,7 +47,7 @@ pub trait FsPermissions {
#[must_use = "the resolved return value to mitigate time-of-check to time-of-use issues"]
fn check_read_path<'a>(
&mut self,
path: &'a Path,
path: Cow<'a, Path>,
api_name: &str,
) -> Result<Cow<'a, Path>, PermissionCheckError>;
fn check_read_all(
@ -68,7 +69,7 @@ pub trait FsPermissions {
#[must_use = "the resolved return value to mitigate time-of-check to time-of-use issues"]
fn check_write_path<'a>(
&mut self,
path: &'a Path,
path: Cow<'a, Path>,
api_name: &str,
) -> Result<Cow<'a, Path>, PermissionCheckError>;
#[must_use = "the resolved return value to mitigate time-of-check to time-of-use issues"]
@ -90,56 +91,69 @@ pub trait FsPermissions {
fn check<'a>(
&mut self,
resolved: bool,
open_options: &OpenOptions,
path: &'a Path,
path: Cow<'a, Path>,
api_name: &str,
) -> Result<std::borrow::Cow<'a, Path>, FsError> {
get_path: &'a dyn GetPath,
) -> Result<CheckedPath<'a>, FsError> {
self.check_open(
resolved,
open_options.read,
open_options.write || open_options.append,
path,
api_name,
get_path,
)
}
fn allows_all(&self) -> bool {
false
}
}
impl FsPermissions for deno_permissions::PermissionsContainer {
fn check_open<'a>(
&mut self,
resolved: bool,
read: bool,
write: bool,
path: &'a Path,
path: Cow<'a, Path>,
api_name: &str,
) -> Result<Cow<'a, Path>, FsError> {
if resolved {
self
.check_special_file(path, api_name)
.map_err(FsError::NotCapable)?;
return Ok(Cow::Borrowed(path));
get_path: &'a dyn GetPath,
) -> Result<CheckedPath<'a>, FsError> {
if self.allows_all() {
return Ok(CheckedPath::Unresolved(path));
}
let (needs_canonicalize, path) = get_path.normalized(path)?;
// If somehow read or write aren't specified, use read
let read = read || !write;
let mut path: Cow<'a, Path> = Cow::Borrowed(path);
if read {
let resolved_path = FsPermissions::check_read_path(self, &path, api_name)
.map_err(|_| FsError::NotCapable("read"))?;
if let Cow::Owned(resolved_path) = resolved_path {
path = Cow::Owned(resolved_path);
}
let path = if read {
FsPermissions::check_read_path(self, path, api_name)
.map_err(|_| FsError::NotCapable("read"))?
} else {
path
};
let path = if write {
FsPermissions::check_write_path(self, path.clone(), api_name)
.map_err(|_| FsError::NotCapable("write"))?
} else {
path
};
let resolved_path = if needs_canonicalize {
Cow::Owned(get_path.resolved(&path)?)
} else {
path
};
self
.check_special_file(&resolved_path, api_name)
.map_err(FsError::NotCapable)?;
if needs_canonicalize {
Ok(CheckedPath::Resolved(resolved_path))
} else {
Ok(CheckedPath::Unresolved(resolved_path))
}
if write {
let resolved_path =
FsPermissions::check_write_path(self, &path, api_name)
.map_err(|_| FsError::NotCapable("write"))?;
if let Cow::Owned(resolved_path) = resolved_path {
path = Cow::Owned(resolved_path);
}
}
Ok(path)
}
fn check_read(
@ -152,7 +166,7 @@ impl FsPermissions for deno_permissions::PermissionsContainer {
fn check_read_path<'a>(
&mut self,
path: &'a Path,
path: Cow<'a, Path>,
api_name: &str,
) -> Result<Cow<'a, Path>, PermissionCheckError> {
deno_permissions::PermissionsContainer::check_read_path(
@ -182,7 +196,7 @@ impl FsPermissions for deno_permissions::PermissionsContainer {
fn check_write_path<'a>(
&mut self,
path: &'a Path,
path: Cow<'a, Path>,
api_name: &str,
) -> Result<Cow<'a, Path>, PermissionCheckError> {
deno_permissions::PermissionsContainer::check_write_path(
@ -224,6 +238,10 @@ impl FsPermissions for deno_permissions::PermissionsContainer {
) -> Result<(), PermissionCheckError> {
deno_permissions::PermissionsContainer::check_write_all(self, api_name)
}
fn allows_all(&self) -> bool {
self.allows_all()
}
}
pub const UNSTABLE_FEATURE_NAME: &str = "fs";
@ -305,3 +323,11 @@ deno_core::extension!(deno_fs,
state.put(options.fs);
},
);
pub trait GetPath {
fn normalized<'a>(
&self,
path: Cow<'a, Path>,
) -> Result<(bool, Cow<'a, Path>), FsError>;
fn resolved(&self, path: &Path) -> Result<PathBuf, FsError>;
}

View file

@ -119,19 +119,19 @@ fn sync_permission_check<'a, P: FsPermissions + 'static>(
permissions: &'a mut P,
api_name: &'static str,
) -> impl AccessCheckFn + 'a {
move |resolved, path, options| {
permissions.check(resolved, options, path, api_name)
move |path, options, resolve| {
permissions.check(options, path, api_name, resolve)
}
}
fn async_permission_check<P: FsPermissions + 'static>(
state: Rc<RefCell<OpState>>,
api_name: &'static str,
) -> impl AccessCheckFn {
move |resolved, path, options| {
) -> impl AccessCheckFn + 'static {
move |path, options, resolve| {
let mut state = state.borrow_mut();
let permissions = state.borrow_mut::<P>();
permissions.check(resolved, options, path, api_name)
permissions.check(options, path, api_name, resolve)
}
}

View file

@ -21,9 +21,11 @@ use deno_io::StdFileResourceInner;
use deno_path_util::normalize_path;
use crate::interface::AccessCheckCb;
use crate::interface::CheckedPath;
use crate::interface::FsDirEntry;
use crate::interface::FsFileType;
use crate::FileSystem;
use crate::GetPath;
use crate::OpenOptions;
#[derive(Debug, Default, Clone)]
@ -1079,43 +1081,15 @@ pub fn open_options_with_access_check(
access_check: Option<AccessCheckCb>,
) -> FsResult<(PathBuf, fs::OpenOptions)> {
if let Some(access_check) = access_check {
let path_bytes = path.as_os_str().as_encoded_bytes();
let is_windows_device_path = cfg!(windows)
&& path_bytes.starts_with(br"\\.\")
&& !path_bytes.contains(&b':');
let path = if is_windows_device_path {
// On Windows, normalize_path doesn't work with device-prefix-style
// paths. We pass these through.
path.to_owned()
} else if path.is_absolute() {
normalize_path(path)
} else {
let cwd = current_dir()?;
normalize_path(cwd.join(path))
let path = Cow::Borrowed(path);
let maybe_resolved = (*access_check)(path, &options, &StdGetPath)?;
let path = maybe_resolved;
let (_resolved, path) = match path {
CheckedPath::Resolved(path) => (true, path),
CheckedPath::Unresolved(path) => (false, path),
};
(*access_check)(false, &path, &options)?;
// On Linux, /proc may contain magic links that we don't want to resolve
let is_linux_special_path = cfg!(target_os = "linux")
&& (path.starts_with("/proc") || path.starts_with("/dev"));
let needs_canonicalization =
!is_windows_device_path && !is_linux_special_path;
let path = if needs_canonicalization {
match path.canonicalize() {
Ok(path) => path,
Err(_) => {
if let (Some(parent), Some(filename)) =
(path.parent(), path.file_name())
{
parent.canonicalize()?.join(filename)
} else {
return Err(std::io::ErrorKind::NotFound.into());
}
}
}
} else {
path
};
(*access_check)(true, &path, &options)?;
let mut opts: fs::OpenOptions = open_options(options);
#[cfg(windows)]
@ -1131,12 +1105,12 @@ pub fn open_options_with_access_check(
// with the exception of /proc/ which is too special, and /dev/std* which might point to
// proc.
use std::os::unix::fs::OpenOptionsExt;
if needs_canonicalization {
if _resolved {
opts.custom_flags(libc::O_NOFOLLOW);
}
}
Ok((path, opts))
Ok((path.into_owned(), opts))
} else {
// for unix
#[allow(unused_mut)]
@ -1150,3 +1124,48 @@ pub fn open_options_with_access_check(
Ok((path.to_path_buf(), opts))
}
}
struct StdGetPath;
impl GetPath for StdGetPath {
fn normalized<'a>(
&self,
path: Cow<'a, Path>,
) -> FsResult<(bool, Cow<'a, Path>)> {
let path_bytes = path.as_os_str().as_encoded_bytes();
let is_windows_device_path = cfg!(windows)
&& path_bytes.starts_with(br"\\.\")
&& !path_bytes.contains(&b':');
let path = if is_windows_device_path {
// On Windows, normalize_path doesn't work with device-prefix-style
// paths. We pass these through.
path
} else if path.is_absolute() {
Cow::Owned(normalize_path(path))
} else {
let cwd = current_dir()?;
Cow::Owned(normalize_path(cwd.join(path)))
};
// On Linux, /proc may contain magic links that we don't want to resolve
let is_linux_special_path = cfg!(target_os = "linux")
&& (path.starts_with("/proc") || path.starts_with("/dev"));
let needs_canonicalization =
!is_windows_device_path && !is_linux_special_path;
Ok((needs_canonicalization, path))
}
fn resolved(&self, path: &Path) -> Result<PathBuf, FsError> {
match path.canonicalize() {
Ok(path) => Ok(path),
Err(_) => {
if let (Some(parent), Some(filename)) =
(path.parent(), path.file_name())
{
Ok(parent.canonicalize()?.join(filename))
} else {
Err(std::io::ErrorKind::NotFound.into())
}
}
}
}
}

View file

@ -46,7 +46,7 @@ pub trait SqliteDbHandlerPermissions {
#[must_use = "the resolved return value to mitigate time-of-check to time-of-use issues"]
fn check_write<'a>(
&mut self,
p: &'a Path,
p: Cow<'a, Path>,
api_name: &str,
) -> Result<Cow<'a, Path>, PermissionCheckError>;
}
@ -64,7 +64,7 @@ impl SqliteDbHandlerPermissions for deno_permissions::PermissionsContainer {
#[inline(always)]
fn check_write<'a>(
&mut self,
p: &'a Path,
p: Cow<'a, Path>,
api_name: &str,
) -> Result<Cow<'a, Path>, PermissionCheckError> {
deno_permissions::PermissionsContainer::check_write_path(self, p, api_name)
@ -131,7 +131,7 @@ impl<P: SqliteDbHandlerPermissions> DatabaseHandler for SqliteDbHandler<P> {
.check_read(&path, "Deno.openKv")
.map_err(JsErrorBox::from_err)?;
let path = permissions
.check_write(&path, "Deno.openKv")
.check_write(Cow::Owned(path), "Deno.openKv")
.map_err(JsErrorBox::from_err)?;
Ok(Some(path.to_string_lossy().to_string()))
}

View file

@ -45,7 +45,7 @@ pub trait NetPermissions {
#[must_use = "the resolved return value to mitigate time-of-check to time-of-use issues"]
fn check_write_path<'a>(
&mut self,
p: &'a Path,
p: Cow<'a, Path>,
api_name: &str,
) -> Result<Cow<'a, Path>, PermissionCheckError>;
#[must_use = "the resolved return value to mitigate time-of-check to time-of-use issues"]
@ -88,7 +88,7 @@ impl NetPermissions for deno_permissions::PermissionsContainer {
#[inline(always)]
fn check_write_path<'a>(
&mut self,
path: &'a Path,
path: Cow<'a, Path>,
api_name: &str,
) -> Result<Cow<'a, Path>, PermissionCheckError> {
deno_permissions::PermissionsContainer::check_write_path(

View file

@ -1334,10 +1334,10 @@ mod tests {
fn check_write_path<'a>(
&mut self,
p: &'a Path,
p: Cow<'a, Path>,
_api_name: &str,
) -> Result<Cow<'a, Path>, PermissionCheckError> {
Ok(Cow::Borrowed(p))
Ok(p)
}
fn check_vsock(

View file

@ -102,11 +102,11 @@ where
.borrow_mut::<NP>()
.check_read(&address_path, "Deno.connect()")
.map_err(NetError::Permission)?;
_ = state_
state_
.borrow_mut::<NP>()
.check_write_path(&address_path, "Deno.connect()")
.map_err(NetError::Permission)?;
address_path
.check_write_path(Cow::Owned(address_path), "Deno.connect()")
.map_err(NetError::Permission)?
};
let unix_stream = UnixStream::connect(&address_path).await?;
let local_addr = unix_stream.local_addr()?;
@ -188,8 +188,8 @@ where
let address_path = permissions
.check_read(&address_path, &api_call_expr)
.map_err(NetError::Permission)?;
_ = permissions
.check_write_path(&address_path, &api_call_expr)
let address_path = permissions
.check_write_path(Cow::Owned(address_path), &api_call_expr)
.map_err(NetError::Permission)?;
let listener = UnixListener::bind(address_path)?;
let local_addr = listener.local_addr()?;
@ -210,8 +210,8 @@ where
let address_path = permissions
.check_read(&address_path, "Deno.listenDatagram()")
.map_err(NetError::Permission)?;
_ = permissions
.check_write_path(&address_path, "Deno.listenDatagram()")
let address_path = permissions
.check_write_path(Cow::Owned(address_path), "Deno.listenDatagram()")
.map_err(NetError::Permission)?;
let socket = UnixDatagram::bind(address_path)?;
let local_addr = socket.local_addr()?;

View file

@ -77,7 +77,7 @@ pub trait NodePermissions {
#[must_use = "the resolved return value to mitigate time-of-check to time-of-use issues"]
fn check_read_path<'a>(
&mut self,
path: &'a Path,
path: Cow<'a, Path>,
) -> Result<Cow<'a, Path>, PermissionCheckError>;
fn query_read_all(&mut self) -> bool;
fn check_sys(
@ -124,7 +124,7 @@ impl NodePermissions for deno_permissions::PermissionsContainer {
fn check_read_path<'a>(
&mut self,
path: &'a Path,
path: Cow<'a, Path>,
) -> Result<Cow<'a, Path>, PermissionCheckError> {
deno_permissions::PermissionsContainer::check_read_path(self, path, None)
}

View file

@ -2610,13 +2610,13 @@ impl PermissionsContainer {
#[inline(always)]
pub fn check_read_path<'a>(
&self,
path: &'a Path,
path: Cow<'a, Path>,
api_name: Option<&str>,
) -> Result<Cow<'a, Path>, PermissionCheckError> {
let mut inner = self.inner.lock();
let inner = &mut inner.read;
if inner.is_allow_all() {
Ok(Cow::Borrowed(path))
Ok(path)
} else {
let desc = PathQueryDescriptor {
requested: path.to_string_lossy().into_owned(),
@ -2698,13 +2698,13 @@ impl PermissionsContainer {
#[inline(always)]
pub fn check_write_path<'a>(
&self,
path: &'a Path,
path: Cow<'a, Path>,
api_name: &str,
) -> Result<Cow<'a, Path>, PermissionCheckError> {
let mut inner = self.inner.lock();
let inner = &mut inner.write;
if inner.is_allow_all() {
Ok(Cow::Borrowed(path))
Ok(path)
} else {
let desc = PathQueryDescriptor {
requested: path.to_string_lossy().into_owned(),
@ -3396,6 +3396,10 @@ impl PermissionsContainer {
),
)
}
pub fn allows_all(&self) -> bool {
matches!(self.inner.lock().all.state, PermissionState::Granted)
}
}
const fn unit_permission_from_flag_bools(

View file

@ -44,10 +44,10 @@ impl deno_fetch::FetchPermissions for Permissions {
fn check_read<'a>(
&mut self,
_resolved: bool,
_p: &'a Path,
_p: Cow<'a, Path>,
_api_name: &str,
) -> Result<Cow<'a, Path>, FsError> {
_get_path: &'a dyn deno_fs::GetPath,
) -> Result<deno_fs::CheckedPath<'a>, FsError> {
unreachable!("snapshotting!")
}
}
@ -88,7 +88,7 @@ impl deno_node::NodePermissions for Permissions {
}
fn check_read_path<'a>(
&mut self,
_path: &'a Path,
_path: Cow<'a, Path>,
) -> Result<Cow<'a, Path>, PermissionCheckError> {
unreachable!("snapshotting!")
}
@ -145,7 +145,7 @@ impl deno_net::NetPermissions for Permissions {
fn check_write_path<'a>(
&mut self,
_p: &'a Path,
_p: Cow<'a, Path>,
_api_name: &str,
) -> Result<Cow<'a, Path>, PermissionCheckError> {
unreachable!("snapshotting!")
@ -164,12 +164,12 @@ impl deno_net::NetPermissions for Permissions {
impl deno_fs::FsPermissions for Permissions {
fn check_open<'a>(
&mut self,
_resolved: bool,
_read: bool,
_write: bool,
_path: &'a Path,
_path: Cow<'a, Path>,
_api_name: &str,
) -> Result<Cow<'a, Path>, FsError> {
_get_path: &'a dyn deno_fs::GetPath,
) -> Result<deno_fs::CheckedPath<'a>, FsError> {
unreachable!("snapshotting!")
}
@ -231,7 +231,7 @@ impl deno_fs::FsPermissions for Permissions {
fn check_read_path<'a>(
&mut self,
_path: &'a Path,
_path: Cow<'a, Path>,
_api_name: &str,
) -> Result<Cow<'a, Path>, PermissionCheckError> {
unreachable!("snapshotting!")
@ -239,7 +239,7 @@ impl deno_fs::FsPermissions for Permissions {
fn check_write_path<'a>(
&mut self,
_path: &'a Path,
_path: Cow<'a, Path>,
_api_name: &str,
) -> Result<Cow<'a, Path>, PermissionCheckError> {
unreachable!("snapshotting!")
@ -257,7 +257,7 @@ impl deno_kv::sqlite::SqliteDbHandlerPermissions for Permissions {
fn check_write<'a>(
&mut self,
_path: &'a Path,
_path: Cow<'a, Path>,
_api_name: &str,
) -> Result<Cow<'a, Path>, PermissionCheckError> {
unreachable!("snapshotting!")