Skip to content
Closed
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
58 changes: 44 additions & 14 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 13 additions & 17 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -164,22 +164,18 @@ inherits = "release"
lto = "fat"
codegen-units = 1

# Important decision (security): pin `jiter` to upstream main so the whole
# workspace can run pyo3 0.29, which fixes GHSA-36hh-v3qg-5jq4 (high, OOB read
# in PyList/PyTuple iterators) and GHSA-chgr-c6px-7xpp (missing `Sync` bound)
# in the bashkit-python extension.
# Important decision (security): the workspace runs pyo3 0.29, which fixes
# GHSA-36hh-v3qg-5jq4 (high, OOB read in PyList/PyTuple iterators) and
# GHSA-chgr-c6px-7xpp (missing `Sync` bound) in the bashkit-python extension.
#
# Blocker: `monty 0.0.19` -> `jiter ^0.15.0` and jiter 0.15.0 declares an
# optional `pyo3 = "^0.28.2"` (jiter 0.16.0 moved to pyo3 0.29 but is outside
# monty's range).
# Although jiter's `python` feature is never activated here (monty only enables
# `num-bigint`), the weak `pyo3?/num-bigint` reference plus pyo3-ffi's
# `links = "python"` global uniqueness force the resolver to honour jiter's
# 0.28 constraint, pinning the whole graph below pyo3 0.29.
# This used to require a `[patch.crates-io]` pin of `jiter` to upstream main:
# `monty 0.0.19` -> `jiter ^0.15.0`, and jiter 0.15.0 declared an optional
# `pyo3 = "^0.28.2"` that dragged the whole graph below pyo3 0.29 (the weak
# `pyo3?/num-bigint` reference plus pyo3-ffi's `links = "python"` global
# uniqueness force the resolver to honour it even though jiter's `python`
# feature is never activated here).
#
# jiter `main` is still version 0.15.0 (semver-compatible with monty's
# `^0.15.0`) but already bumped its pyo3 dependency to 0.29, so this patch
# unblocks the upgrade without a fork. Drop it once monty ships a release that
# tracks a published jiter with pyo3 0.29.
[patch.crates-io]
jiter = { git = "https://github.com/pydantic/jiter", rev = "6d57715e01ec78859c62fc5447073c0b5902de39" }
# `monty 0.0.21` tracks the published `jiter 0.16.0`, which is already on
# pyo3 0.29, so the patch is no longer needed and has been dropped. Keep
# monty and monty-types on the same version — a split leaves two
# `monty-types` in the graph and its result types stop unifying.
2 changes: 1 addition & 1 deletion crates/bashkit-python/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ serde_json = { workspace = true }

# Big-integer support for py_to_monty BigInt extraction.
# Pinned to 0.4.x: py_to_monty parses Python ints into `num_bigint::BigInt` and
# hands them to `MontyObject::BigInt`, and monty (0.0.19) depends on
# hands them to `MontyObject::BigInt`, and monty (0.0.21) depends on
# num-bigint 0.4. Bumping to 0.5 makes the two BigInt types mismatch and fails
# to compile, so this must track whatever major version bashkit core / monty use.
num-bigint = "^0.4.6"
Expand Down
4 changes: 2 additions & 2 deletions crates/bashkit/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,8 @@ os_display = "0.1.3"
# Registry dep since monty 0.0.19 — the first version published to crates.io.
# Keeps `python` usable for downstream crates and lets the publish workflow
# ship the feature instead of stripping it.
monty = { version = "0.0.19", optional = true }
monty-types = { version = "0.0.19", optional = true }
monty = { version = "0.0.21", optional = true }
monty-types = { version = "0.0.21", optional = true }

# Embedded TypeScript interpreter (optional)
zapcode-core = { version = "1.5.1", optional = true }
Expand Down
121 changes: 31 additions & 90 deletions crates/bashkit/src/builtins/python.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,10 @@ use async_trait::async_trait;
use chrono::{Datelike, Timelike};
use monty::{MontyRun, RunProgress};
use monty_types::{
CompileOptions, ExcType, ExtFunctionResult, FileMode, LimitedTracker, MontyDate, MontyDateTime,
MontyException, MontyFileHandle, MontyObject, NameLookupResult, OsFunctionCall, PrintWriter,
ResourceError, ResourceLimits, ResourceTracker, dir_stat, file_stat, symlink_stat,
CompileOptions, ExcType, ExtFunctionResult, FileMode, MontyDate, MontyDateTime, MontyException,
MontyFileHandle, MontyObject, NameLookupResult, OsFunctionCall, PrintWriter, ResourceLimits,
ResourceTracker, dir_stat, file_stat, symlink_stat,
};
use std::cell::Cell;
use std::collections::HashMap;
use std::future::Future;
use std::path::{Path, PathBuf};
Expand All @@ -50,85 +49,28 @@ const DISABLED_STDLIB_MODULES: &[&str] = &["re"];
// sandboxed code cannot fingerprint host clock/timezone state.
const VIRTUAL_NOW_UNIX_SECS: i64 = 1_704_067_200; // 2024-01-01T00:00:00Z

/// Bridges Monty's statement/allocation checkpoints into the shared request
/// budget while retaining Monty's own memory/time/recursion ceilings.
#[derive(Debug)]
struct BudgetTracker {
runtime: LimitedTracker,
execution: Option<crate::limits::ExecutionBudget>,
vm_checkpoints: Cell<u64>,
}

impl BudgetTracker {
fn budget_error(err: crate::limits::LimitExceeded) -> ResourceError {
// Monty's tracker error type has no host-defined variant. The shared
// budget retains the precise poisoned reason; use an uncatchable
// memory error only to stop the VM at this checkpoint.
let _ = err;
ResourceError::Memory { limit: 0, used: 1 }
}

fn consume_work(&self) -> std::result::Result<(), ResourceError> {
if let Some(budget) = &self.execution {
budget.consume_work(1).map_err(Self::budget_error)?;
}
Ok(())
}

fn check_vm(&self) -> std::result::Result<(), ResourceError> {
if let Some(budget) = &self.execution {
budget.check().map_err(Self::budget_error)?;
let checkpoints = self.vm_checkpoints.get().wrapping_add(1);
self.vm_checkpoints.set(checkpoints);
if checkpoints.is_multiple_of(64) {
budget.consume_work(1).map_err(Self::budget_error)?;
}
}
Ok(())
}
}

impl ResourceTracker for BudgetTracker {
fn on_free(&self, get_size: impl FnOnce() -> usize) {
self.runtime.on_free(get_size);
}

fn check_time(&self) -> std::result::Result<(), ResourceError> {
self.check_vm()?;
self.runtime.check_time()
}

fn check_recursion_depth(
&self,
current_depth: usize,
) -> std::result::Result<(), ResourceError> {
self.runtime.check_recursion_depth(current_depth)
}

fn check_large_result(&self, estimated_bytes: usize) -> std::result::Result<(), ResourceError> {
self.runtime.check_large_result(estimated_bytes)
}

fn on_grow(
&self,
additional_bytes: impl FnOnce() -> usize,
) -> std::result::Result<(), ResourceError> {
self.consume_work()?;
self.runtime.on_grow(additional_bytes)
}

fn gc_interval(&self) -> Option<usize> {
self.runtime.gc_interval()
}

fn on_execution_start(&self) {
self.runtime.on_execution_start();
}

fn on_execution_stop(&self) {
self.runtime.on_execution_stop();
}
}
// Important decision (security): Monty 0.0.21 turned `ResourceTracker` from a
// host-implementable trait into a concrete struct, deleting `LimitedTracker`.
// Bashkit used to wrap it in a `BudgetTracker` that charged the shared
// `ExecutionBudget` from inside the VM's own allocation/statement checkpoints.
// There is no replacement hook in 0.0.21, so that per-checkpoint charging is
// gone and the shared budget is now driven the same way the TypeScript builtin
// (which never had an in-VM hook) drives it:
//
// * up front, before the VM starts — input bytes, code size, and a reserve
// proportional to the VM's independent memory ceiling, so repeated
// invocations cannot each claim a fresh full allowance;
// * per host round-trip in the start/resume loop — one unit per OS call,
// 100 per external function call.
//
// The containment properties that matter are unchanged, because they were
// never the tracker's job: `max_duration` is clamped to the caller's remaining
// execution deadline before the VM starts (see `Builtin::execute` below), and
// Monty's own `ResourceTracker` still enforces that duration plus `max_memory`
// and the recursion ceiling synchronously. A CPU-bound script that never
// re-enters the host loop is therefore still stopped by the clamped deadline;
// what is lost is only the finer-grained *work-unit* accounting for such a
// script, which the up-front reserve approximates.
const VIRTUAL_NOW_NANOS: u32 = 123_456_000; // 123456 µs for deterministic microseconds

const PYTHON_INPROCESS_OPT_IN_ENV: &str = "BASHKIT_ALLOW_INPROCESS_PYTHON";
Expand Down Expand Up @@ -625,16 +567,15 @@ async fn run_python(
Err(e) => return Ok(format_exception(e)),
};

let limits = ResourceLimits::new()
let limits = ResourceLimits::default()
.max_duration(py_limits.common.max_duration)
.max_memory(py_limits.common.max_memory)
.max_recursion_depth(Some(py_limits.common.max_call_depth));
.max_recursion_depth(py_limits.common.max_call_depth);

let tracker = BudgetTracker {
runtime: LimitedTracker::new(limits),
execution: execution_budget.clone(),
vm_checkpoints: Cell::new(0),
};
// See the decision note at the top of this file: Monty 0.0.21 dropped the
// host-implementable tracker trait, so the shared budget is charged up
// front and per host round-trip instead of per VM checkpoint.
let tracker = ResourceTracker::new(limits);
// Important security decision: cap collected print output at the same
// memory budget as the VM heap. Monty 0.0.19 added a byte cap on
// `PrintWriter::CollectString` because a `while True: print(...)` loop
Expand Down
27 changes: 27 additions & 0 deletions crates/bashkit/tests/integration/execution_budget_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,33 @@ async fn repeated_python_entries_share_runtime_admission_budget() {
);
}

#[cfg(feature = "python")]
#[tokio::test]
/// TM-DOS-096: a VFS-heavy Python loop must still exhaust shared work units.
///
/// Monty 0.0.21 removed the host-implementable `ResourceTracker` trait, so
/// Bashkit no longer charges the budget from inside the VM's own allocation
/// checkpoints; the start/resume loop's per-round-trip charging is what
/// remains. Each `open()` suspends the VM into an OsCall, so a loop of them
/// must still drain the budget — this fails if that charging is ever dropped.
async fn python_vfs_round_trips_consume_shared_work_budget() {
let limits = ExecutionLimits::new()
.max_work_units(2_000_000)
.max_aggregate_input_bytes(100_000);
let mut bash = Bash::builder()
.limits(limits)
.python()
.env("BASHKIT_ALLOW_INPROCESS_PYTHON", "1")
.build();

assert_budget_exhausted(
bash.exec(
"python -c '\nfor i in range(100000):\n f = open(\"/tmp/x\", \"w\")\n f.write(\"a\")\n f.close()\n'",
)
.await,
);
}

#[cfg(feature = "typescript")]
#[tokio::test]
/// TM-DOS-096: separate TypeScript entries cannot refresh allocation fuel.
Expand Down
Loading
Loading