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: 0 additions & 7 deletions src/event_loop/ManagedTask.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,8 @@
let callback = this.callback;
let ctx = this.ctx;
callback(ctx.unwrap().as_ptr())
}

Check warning on line 34 in src/event_loop/ManagedTask.rs

View check run for this annotation

Claude / Claude Code Review

Stale comment in event_loop.rs references removed ManagedTask::cancel()

Removing `ManagedTask::cancel()` leaves the doc comment on `release_queued_tasks_for_shutdown` at `src/jsc/event_loop.rs:735-742` stale — it still names `cancel()` (and the already-gone `SendQueue.close_next_tick`/`after_close_task`) as the load-bearing rationale for the `task.tag != ManagedTask` guard at line 756. Per REVIEW.md "grep for every sibling site sharing the pattern", that comment should be updated (and the guard's remaining justification, if any, restated) in this PR. Not a runtime b

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Removing ManagedTask::cancel() leaves the doc comment on release_queued_tasks_for_shutdown at src/jsc/event_loop.rs:735-742 stale — it still names cancel() (and the already-gone SendQueue.close_next_tick/after_close_task) as the load-bearing rationale for the task.tag != ManagedTask guard at line 756. Per REVIEW.md "grep for every sibling site sharing the pattern", that comment should be updated (and the guard's remaining justification, if any, restated) in this PR. Not a runtime bug — behavior is unchanged.

Extended reasoning...

What is stale

The PR deletes ManagedTask::cancel() from src/event_loop/ManagedTask.rs after verifying zero call sites. That's correct — nothing invokes .cancel() on a ManagedTask. But the doc comment on EventLoop::release_queued_tasks_for_shutdown at src/jsc/event_loop.rs:735-742 still documents cancel() as the reason ManagedTask entries are re-queued instead of freed at shutdown:

ManagedTask entries are deliberately re-queued rather than freed: owners (e.g. SendQueue.close_next_tick / after_close_task) keep raw back-pointers that they cancel() from Drop, and those Drops fire during destructOnExit (Subprocess::finalizeSendQueue::drop). Freeing the box here would leave those pointers dangling and make cancel() a heap-use-after-free.

With cancel() deleted, this comment now describes a UAF hazard around a method that structurally cannot be called. The task.tag != ManagedTask guard at line 756 is left with no live justification in prose.

Why the PR's verification missed it

The PR description says every symbol was checked with rg -w <symbol> src/ build/debug/codegen/ src/codegen/ for zero hits outside its own definition. A prose reference inside a /// doc comment doesn't show up as a caller to rg -w cancel (there are dozens of unrelated .cancel() hits on other types), and the referenced SendQueue.close_next_tick/after_close_task owners were already gone before this PR — rg 'close_next_tick|after_close_task' src/ returns only this comment. So the comment was already partially stale; removing cancel() is what makes its central claim reference a nonexistent method.

Step-by-step

  1. Before this PR: ManagedTask has a cancel() method that overwrites self.callback with a no-op. No code calls it.
  2. src/jsc/event_loop.rs:756 special-cases task.tag != ManagedTask so that __bun_release_task_at_shutdown never consumes a ManagedTask — they get re-queued and freed later in deinit() (line 783) via the cleanup field.
  3. The doc comment at :735-742 explains that guard by saying owners hold raw back-pointers and call cancel() from Drop during destructOnExit, so freeing the box early would make that cancel() a heap-UAF.
  4. This PR deletes cancel(). The named owners (SendQueue.close_next_tick, after_close_task) already exist nowhere in src/.
  5. After this PR: the guard at :756 and the free-in-deinit() at :783 are still correct (behavior unchanged — ManagedTask boxes are still freed via heap::take + cleanup after destructOnExit), but the comment now points at a method that doesn't exist to explain why.

Why this matters per REVIEW.md

REVIEW.md → Correctness → "Fix the whole class in the same PR … Grep for every sibling site sharing the pattern" and → One source of truth; update every consumer atomically both apply: a comment that names a symbol as its load-bearing rationale is a consumer of that symbol. REVIEW.md → Code style → "Delete dead code in the same PR that makes it dead" is what this PR is doing; the stale comment is a leftover of the same class.

REVIEW.md also says "Before deleting odd-looking code, git-blame why it was written — it is usually load-bearing." The next reader trying to understand why ManagedTask is special-cased at :756 will follow this comment to a method that no longer exists and owners that no longer exist, and can't tell whether the guard is still needed.

Impact

None at runtime. release_queued_tasks_for_shutdown and deinit() behave identically before and after. This is purely a doc-comment/rationale drift.

Fix

Update the doc comment at src/jsc/event_loop.rs:735-742 to either (a) delete the cancel()/SendQueue paragraph and restate the actual remaining reason ManagedTask is deferred to deinit() (if one still exists — e.g. cleanup may call into subsystems that aren't safe until after destructOnExit, or an owner still holds a raw back-pointer through destructOnExit), or (b) if no rationale remains, note that and consider dropping the special-case in a follow-up. Either way, the comment should stop naming cancel().


pub fn cancel(&mut self) {
fn noop(_: *mut c_void) -> JsResult<()> {
Ok(())
}
self.callback = noop;
}

// A per-(Type, Callback) trampoline is folded away by storing
// the type-erased fn pointer directly — `fn(*mut T)` and `fn(*mut c_void)` share ABI.
pub fn new<T>(ctx: *mut T, callback: fn(*mut T) -> JsResult<()>) -> Task {
Expand Down
4 changes: 1 addition & 3 deletions src/install/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,7 @@ pub mod external_slice {
};
}
pub mod versioned_url {
pub use bun_install_types::resolver_hooks::{
OldV2VersionedURL, VersionedURL, VersionedURLType,
};
pub use bun_install_types::resolver_hooks::{VersionedURL, VersionedURLType};
}

pub mod extract_tarball;
Expand Down
6 changes: 0 additions & 6 deletions src/install/lockfile/Package/Scripts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,6 @@ impl Scripts {
#[derive(Clone, Copy, PartialEq, Eq)]
pub enum PrintFormat {
Completed,
Info,
Untrusted,
}

Expand Down Expand Up @@ -459,11 +458,6 @@ impl List {
BStr::new(name),
BStr::new(script),
),
PrintFormat::Info => bun_core::pretty!(
" [{s}]<d>:<r> <cyan>{s}<r>\n",
BStr::new(name),
BStr::new(script),
),
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/install_types/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ pub use resolver_hooks::{
DependencySlice, DependencyVersion, DependencyVersionTag, DependencyVersionValue,
EnqueueResult, ExternalPackageNameHashList, ExternalSlice, ExternalStringList,
ExternalStringMap, Features, INVALID_DEPENDENCY_ID, INVALID_PACKAGE_ID, Libc, Negatable,
NegatableEnum, NegatableExt, NpmInfo, OldV2VersionedURL, OperatingSystem, PackageID,
PackageJsonView, PackageNameHash, PreinstallState, Repository, Resolution, ResolutionSlice,
ResolutionTag, ResolutionValue, TagInfo, TarballInfo, TaskCallbackContext,
TruncatedPackageNameHash, URI, VersionSlice, VersionedURL, VersionedURLType, WakeHandler,
NegatableEnum, NegatableExt, NpmInfo, OperatingSystem, PackageID, PackageJsonView,
PackageNameHash, PreinstallState, Repository, Resolution, ResolutionSlice, ResolutionTag,
ResolutionValue, TagInfo, TarballInfo, TaskCallbackContext, TruncatedPackageNameHash, URI,
VersionSlice, VersionedURL, VersionedURLType, WakeHandler,
};

// The canonical ExternalString / SlicedString / SemverString definitions live
Expand Down
1 change: 0 additions & 1 deletion src/install_types/resolver_hooks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1052,7 +1052,6 @@ impl Repository {
// can name the `npm` arm's payload without an upward edge.

pub type VersionedURL = VersionedURLType<u64>;
pub type OldV2VersionedURL = VersionedURLType<u32>;

#[repr(C)]
pub struct VersionedURLType<SemverInt: bun_semver::version::VersionInt> {
Expand Down
32 changes: 0 additions & 32 deletions src/resolver/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -278,38 +278,6 @@ impl Entry {
}
}

// `BSSList::append` requires `ValueType: Clone` (its overflow path
// retries with a copy). `Mutex`/`StringOrTinyString` aren't `Clone`, but for a
// freshly-constructed `Entry` (the only thing ever appended) a field-wise copy
// with a fresh `Mutex` is semantically equivalent to a by-value move.
impl Clone for Entry {
fn clone(&self) -> Self {
Self {
cache: core::cell::Cell::new(self.cache.get()),
dir: self.dir,
base_: strings::StringOrTinyString::init(self.base_.slice()),
base_lowercase_: strings::StringOrTinyString::init(self.base_lowercase_.slice()),
mutex: Mutex::default(),
need_stat: core::cell::Cell::new(self.need_stat.get()),
abs_path: self.abs_path,
}
}
}

impl Default for Entry {
fn default() -> Self {
Self {
cache: core::cell::Cell::new(EntryCache::default()),
dir: b"",
base_: strings::StringOrTinyString::init(b""),
base_lowercase_: strings::StringOrTinyString::init(b""),
mutex: Mutex::default(),
need_stat: core::cell::Cell::new(true),
abs_path: Interned::EMPTY,
}
}
}

// `entry` is a RAW `*mut Entry`. A safe
// `&self → &mut Entry` accessor would let two `get()` calls produce coexisting
// aliased `&mut Entry` (PORTING.md §Forbidden). Callers `unsafe { &mut *entry }`
Expand Down
11 changes: 0 additions & 11 deletions src/resolver/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2072,17 +2072,6 @@ pub mod cache {
pub(crate) stream: bool,
}

impl Default for Fs {
fn default() -> Self {
Self {
shared_buffer: MutableString::init(0).expect("unreachable"),
macro_shared_buffer: MutableString::init(0).expect("unreachable"),
use_alternate_source_cache: false,
stream: false,
}
}
}

/// Optional external destructor (`function(ctx)`) for foreign-owned
/// source bytes; `NONE` when there is nothing external to free.
#[repr(C)]
Expand Down
15 changes: 0 additions & 15 deletions src/resolver/result.rs
Original file line number Diff line number Diff line change
Expand Up @@ -292,21 +292,6 @@ pub struct DirEntryResolveQueueItem {
pub(crate) fd: FD,
}

impl Default for DirEntryResolveQueueItem {
fn default() -> Self {
Self {
result: allocators::Result {
hash: 0,
index: allocators::NOT_FOUND,
status: allocators::ItemStatus::Unknown,
},
unsafe_path: bun_ptr::RawSlice::EMPTY,
safe_path: bun_ptr::RawSlice::EMPTY,
fd: FD::INVALID,
}
}
}

// `bun_alloc::Result` doesn't derive Clone (yet); all its fields are Copy, so
// hand-roll Clone here for the queue-item move at `dir_info_cached`.
impl Clone for DirEntryResolveQueueItem {
Expand Down
6 changes: 0 additions & 6 deletions src/runtime/node/node_fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4550,12 +4550,6 @@ pub enum StatOrNotFound {
NotFound,
}
impl StatOrNotFound {
pub fn to_js(&mut self, global_object: &JSGlobalObject) -> JsResult<JSValue> {
match self {
StatOrNotFound::Stats(s) => s.to_js_newly_created(global_object),
StatOrNotFound::NotFound => Ok(JSValue::UNDEFINED),
}
}
pub(crate) fn to_js_newly_created(&self, global_object: &JSGlobalObject) -> JsResult<JSValue> {
match self {
StatOrNotFound::Stats(s) => s.to_js_newly_created(global_object),
Expand Down
2 changes: 1 addition & 1 deletion src/runtime/webcore/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ pub mod fetch_tasklet;

#[path = "fetch/FetchRequestBodySink.rs"]
pub mod fetch_request_body_sink;
pub use self::fetch_request_body_sink::{FetchRequestBodySink, FetchRequestBodySinkJSSink};
pub use self::fetch_request_body_sink::FetchRequestBodySink;

#[path = "fetch/compress_body.rs"]
pub mod compress_body;
Expand Down
3 changes: 0 additions & 3 deletions src/runtime/webcore/fetch/FetchRequestBodySink.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use bun_sys::Error as SysError;
use crate::webcore::blob::SizeType as BlobSizeType;
use crate::webcore::fetch::fetch_tasklet::FetchTasklet;
use crate::webcore::jsc::{JSGlobalObject, JSPromise, JSValue};
use crate::webcore::sink::JSSink;
use crate::webcore::streams::{
SourceHandle, Start, StartTag, StreamError, StreamResult, Writable, WritablePending,
};
Expand Down Expand Up @@ -315,5 +314,3 @@ impl crate::webcore::sink::JsSinkType for FetchRequestBodySink {
self.done
}
}

pub type FetchRequestBodySinkJSSink = JSSink<FetchRequestBodySink>;