Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion docs/ARCHITECTURE.md
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,12 @@ For day-to-day workflow and commands, see [`CLAUDE.md`](../CLAUDE.md) and
region. TTL lives in the shared header and governs expiry for every reader, so a new header
field that changes behavior must be added to this reuse check too — otherwise a process opening
with a different value silently inherits the creator's, producing config-dependent behavior.
- **No second shard guard while one is live (reentrancy, issue #30).** A memory-backend
- **Shm files are owner-private (issue #39).** The mmap files hold serialized (and possibly
pickled) return values, so they live in a per-user directory `warp_cache-<uid>` created `0o700`
(under `/dev/shm` on Linux, `$TMPDIR` otherwise) and are created `0o600`. On a shared parent
like `/dev/shm`, `ensure_secure_dir` refuses a pre-existing dir not owned by us or with
group/other access, so a co-located user can't read the cache or pre-create a crafted file that
the victim would `pickle.loads`. Any new shm file-creation site must set `.mode(0o600)`. A memory-backend
lookup runs arbitrary Python `__eq__` (via `PyObject_RichCompareBool`) *while a shard guard
is held*. That `__eq__` can re-enter the same `CachedFunction` (or, on GIL builds, hand off
the GIL to another thread that calls in) and take a second, conflicting guard — aliasing
Expand Down
71 changes: 59 additions & 12 deletions src/shm/region.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,69 @@
/// cache: header + lock + hash table + slab arena.
use std::fs;
use std::io;
use std::os::unix::fs::{DirBuilderExt, MetadataExt, OpenOptionsExt, PermissionsExt};
use std::path::{Path, PathBuf};

use memmap2::MmapMut;

use super::layout::{self, Bucket, Header, SlotHeader, BUCKET_EMPTY, MAGIC, SLOT_NONE, VERSION};
use super::lock::{ShmSeqLock, LOCK_SIZE};

/// Where to store the mmap files.
/// File mode for the cache files: owner read/write only. They hold serialized
/// (possibly pickled) return values, so no group/other access (#39).
const FILE_MODE: u32 = 0o600;

/// Current real user id.
fn current_uid() -> u32 {
// SAFETY: getuid() always succeeds and is thread-safe.
unsafe { libc::getuid() }
}

/// Per-user directory holding the mmap cache files.
///
/// The files contain serialized (and possibly pickled) return values, so they
/// must not be readable or pre-creatable by other local users (#39). On Linux the
/// parent is the world-writable, sticky `/dev/shm`, so the directory name is scoped
/// to the uid and created `0o700` (see `ensure_secure_dir`); on macOS/other `$TMPDIR`
/// is already per-user, but we keep the same layout for defense in depth.
fn shm_dir() -> PathBuf {
if cfg!(target_os = "linux") {
let base = if cfg!(target_os = "linux") {
PathBuf::from("/dev/shm")
} else {
// macOS and other Unix: use TMPDIR
std::env::temp_dir().join("warp_cache")
std::env::temp_dir()
};
base.join(format!("warp_cache-{}", current_uid()))
}

/// Create the per-user shm directory as `0o700`, or — if it already exists — verify
/// it is a directory we own with no group/other access. This refuses to use a
/// directory a hostile local user pre-created on a shared parent like `/dev/shm`,
/// which would otherwise let them read or tamper with the cache files (#39).
fn ensure_secure_dir(dir: &Path) -> io::Result<()> {
match fs::DirBuilder::new().mode(0o700).create(dir) {
Ok(()) => return Ok(()),
Err(e) if e.kind() == io::ErrorKind::AlreadyExists => {}
Err(e) => return Err(e),
}
// symlink_metadata (not metadata): a symlink must not redirect us elsewhere.
let meta = fs::symlink_metadata(dir)?;
if !meta.file_type().is_dir() {
return Err(io::Error::new(
io::ErrorKind::AlreadyExists,
"shm path exists but is not a directory",
));
}
if meta.uid() != current_uid() {
return Err(io::Error::new(
io::ErrorKind::PermissionDenied,
"shm directory is owned by another user",
));
}
// We own it — tighten perms if a previous run (or anyone) left it group/other-accessible.
if meta.permissions().mode() & 0o077 != 0 {
fs::set_permissions(dir, fs::Permissions::from_mode(0o700))?;
}
Ok(())
}

/// The full shared-memory region, owning the mmap handle and providing
Expand Down Expand Up @@ -50,9 +98,7 @@ impl ShmRegion {
}

let dir = shm_dir();
if !dir.exists() {
fs::create_dir_all(&dir)?;
}
ensure_secure_dir(&dir)?;

// Hash table must be power-of-2 for bitmask probing. `capacity * 2` and its
// next-power-of-two must both fit in u32, or the table is silently mis-sized
Expand All @@ -73,21 +119,23 @@ impl ShmRegion {
let data_path = dir.join(format!("{name}.data"));
let lock_path = dir.join(format!("{name}.lock"));

// Create or truncate the data file
// Create or truncate the data file (owner-only, #39)
let data_file = fs::OpenOptions::new()
.read(true)
.write(true)
.create(true)
.truncate(true)
.mode(FILE_MODE)
.open(&data_path)?;
data_file.set_len(total_size as u64)?;

// Create or truncate the lock file
// Create or truncate the lock file (owner-only, #39)
let lock_file = fs::OpenOptions::new()
.read(true)
.write(true)
.create(true)
.truncate(true)
.mode(FILE_MODE)
.open(&lock_path)?;
lock_file.set_len(LOCK_SIZE as u64)?;

Expand Down Expand Up @@ -189,9 +237,7 @@ impl ShmRegion {
ttl_nanos: u64,
) -> io::Result<Self> {
let dir = shm_dir();
if !dir.exists() {
fs::create_dir_all(&dir)?;
}
ensure_secure_dir(&dir)?;
let data_path = dir.join(format!("{name}.data"));
let lock_path = dir.join(format!("{name}.lock"));
let init_path = dir.join(format!("{name}.init"));
Expand All @@ -208,6 +254,7 @@ impl ShmRegion {
.write(true)
.create(true)
.truncate(false) // never truncate — it's purely an advisory lock file
.mode(FILE_MODE) // owner-only (#39)
.open(&init_path)?;
init_file.lock()?;

Expand Down
2 changes: 1 addition & 1 deletion tests/test_complex_objects.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class User:

def _cleanup_shm():
tmpdir = tempfile.gettempdir()
shm_dir = os.path.join(tmpdir, "warp_cache")
shm_dir = os.path.join(tmpdir, f"warp_cache-{os.getuid()}")
if os.path.isdir(shm_dir):
for f in glob.glob(os.path.join(shm_dir, "*")):
with contextlib.suppress(OSError):
Expand Down
8 changes: 4 additions & 4 deletions tests/test_introspection.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@


def _cleanup_shm():
shm_dir = os.path.join(tempfile.gettempdir(), "warp_cache")
if not hasattr(os, "getuid"): # Windows: no shared backend / shm files
return
shm_dir = os.path.join(tempfile.gettempdir(), f"warp_cache-{os.getuid()}")
if os.path.isdir(shm_dir):
for f in glob.glob(os.path.join(shm_dir, "*")):
with contextlib.suppress(OSError):
Expand All @@ -36,9 +38,7 @@ def _cleanup_shm():
"memory",
pytest.param(
"shared",
marks=pytest.mark.skipif(
sys.platform == "win32", reason="shared memory is Unix-only"
),
marks=pytest.mark.skipif(sys.platform == "win32", reason="shared memory is Unix-only"),
),
],
)
Expand Down
40 changes: 39 additions & 1 deletion tests/test_shared_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import contextlib
import glob
import os
import stat
import sys
import tempfile

import pytest
Expand All @@ -13,7 +15,7 @@
def _cleanup_shm():
"""Remove any leftover shared memory files."""
tmpdir = tempfile.gettempdir()
shm_dir = os.path.join(tmpdir, "warp_cache")
shm_dir = os.path.join(tmpdir, f"warp_cache-{os.getuid()}")
if os.path.isdir(shm_dir):
for f in glob.glob(os.path.join(shm_dir, "*")):
with contextlib.suppress(OSError):
Expand Down Expand Up @@ -425,3 +427,39 @@ def test_oversized_max_size_raises(self, max_size):
@cache(max_size=max_size, backend="shared")
def fn(x):
return x


class TestSharedFilePermissions:
"""Regression for #39: the mmap cache files hold serialized (and possibly
pickled) return values, so they must be owner-only (0o600) inside a per-user
0o700 directory — not world-readable in a shared dir like /dev/shm, where
another local user could read them or pre-create a crafted file."""

def setup_method(self):
_cleanup_shm()

def teardown_method(self):
_cleanup_shm()

@pytest.mark.skipif(sys.platform == "win32", reason="shared memory is Unix-only")
def test_shm_dir_and_files_are_owner_only(self):
# Mirror src/shm/region.rs::shm_dir.
base = "/dev/shm" if sys.platform.startswith("linux") else tempfile.gettempdir()
shm_dir = os.path.join(base, f"warp_cache-{os.getuid()}")

@cache(max_size=16, backend="shared")
def fn(x):
return x * 2

assert fn(1) == 2 # creates the region + files

# The per-user directory must be private (owner rwx only).
dmode = stat.S_IMODE(os.stat(shm_dir).st_mode)
assert dmode == 0o700, f"{shm_dir} is {oct(dmode)}, expected 0o700"

# Every cache file must be owner read/write only — no group/other bits.
files = glob.glob(os.path.join(shm_dir, "*"))
assert files, "no shm files were created"
for f in files:
fmode = stat.S_IMODE(os.stat(f).st_mode)
assert fmode & 0o077 == 0, f"{f} is group/other-accessible: {oct(fmode)}"
17 changes: 8 additions & 9 deletions tests/test_shared_multiprocess.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,21 @@
from warp_cache._warp_cache_rs import SharedCachedFunction


def _shm_dir():
# Must match src/shm/region.rs::shm_dir: per-user dir (#39) — warp_cache-<uid>
# under /dev/shm on Linux, $TMPDIR otherwise.
base = "/dev/shm" if sys.platform.startswith("linux") else tempfile.gettempdir()
return os.path.join(base, f"warp_cache-{os.getuid()}")


def _cleanup_shm():
tmpdir = tempfile.gettempdir()
shm_dir = os.path.join(tmpdir, "warp_cache")
shm_dir = _shm_dir()
if os.path.isdir(shm_dir):
for f in glob.glob(os.path.join(shm_dir, "*")):
with contextlib.suppress(OSError):
os.unlink(f)


def _shm_dir():
# Must match src/shm/region.rs::shm_dir: /dev/shm on Linux, $TMPDIR/warp_cache otherwise.
if sys.platform.startswith("linux"):
return "/dev/shm"
return os.path.join(tempfile.gettempdir(), "warp_cache")


def _unlink_shm(name):
"""Remove all files for a shm name so the next open is a cold create."""
for suffix in (".data", ".lock", ".init"):
Expand Down
2 changes: 1 addition & 1 deletion tests/test_shared_realistic.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

def _cleanup_shm():
tmpdir = tempfile.gettempdir()
shm_dir = os.path.join(tmpdir, "warp_cache")
shm_dir = os.path.join(tmpdir, f"warp_cache-{os.getuid()}")
if os.path.isdir(shm_dir):
for f in glob.glob(os.path.join(shm_dir, "*")):
with contextlib.suppress(OSError):
Expand Down
Loading