Skip to content

upgrade: pass a stack Progress instead of a leaked Box and raw ptrs #37674

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
Open

upgrade: pass a stack Progress instead of a leaked Box and raw ptrs #37674

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
175 changes: 60 additions & 115 deletions src/runtime/cli/upgrade_command.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,12 +177,11 @@ pub(crate) struct UpgradeCommand;
impl UpgradeCommand {
const DEFAULT_GITHUB_HEADERS: &'static [u8] = b"Acceptapplication/vnd.github.v3+json";

pub(crate) fn get_latest_version<const SILENT: bool>(
pub(crate) fn get_latest_version(
env_loader: &mut DotEnv::Loader,
refresher: Option<&mut Progress::Progress>,
mut progress: Option<&mut Progress::Node>,
progress: &mut Progress::Progress,
use_profile: bool,
) -> crate::Result<Option<Version>> {
) -> crate::Result<Version> {
let mut headers_buf: Vec<u8> = Self::DEFAULT_GITHUB_HEADERS.to_vec();

let mut header_entries: headers::EntryList = headers::EntryList::default();
Expand All @@ -197,7 +196,6 @@ impl UpgradeCommand {
},
};
header_entries.append(accept).expect("oom");
// defer if SILENT header_entries.deinit() — Drop handles this

// Incase they're using a GitHub proxy in e.g. China
let mut github_api_domain: &[u8] = b"api.github.com";
Expand Down Expand Up @@ -271,12 +269,8 @@ impl UpgradeCommand {
));
async_http.client.flags.reject_unauthorized = env_loader.get_tls_reject_unauthorized();

if !SILENT {
// `progress_node` stores an untracked NonNull borrow of the caller's
// `progress`; sound because `send_sync` below completes before this
// frame returns, so the pointee outlives every use.
async_http.client.progress_node = Some(NonNull::from(progress.as_deref_mut().unwrap()));
}
// Untracked borrow; the HTTP thread only touches the node while `send_sync` runs.
async_http.client.progress_node = Some(NonNull::from(&mut progress.root));
let response = async_http.send_sync(metadata_body)?;

match response.status_code() {
Expand All @@ -298,36 +292,28 @@ impl UpgradeCommand {
let expr = match JSON::parse_utf8(&source, &mut log, bump) {
Ok(e) => e,
Err(err) => {
if !SILENT {
progress.expect("infallible: progress active").end();
refresher.expect("infallible: progress active").refresh();
progress.root.end();
progress.refresh();

if log.errors > 0 {
let _ = log.print(std::ptr::from_mut(Output::error_writer()));
Global::exit(1);
} else {
bun_core::pretty_errorln!(
"Error parsing releases from GitHub: <r><red>{}<r>",
err.name()
);
Global::exit(1);
}
if log.errors > 0 {
let _ = log.print(std::ptr::from_mut(Output::error_writer()));
Global::exit(1);
} else {
bun_core::pretty_errorln!(
"Error parsing releases from GitHub: <r><red>{}<r>",
err.name()
);
Global::exit(1);
}

return Ok(None);
}
};

if log.errors > 0 {
if !SILENT {
progress.expect("infallible: progress active").end();
refresher.expect("infallible: progress active").refresh();

let _ = log.print(std::ptr::from_mut(Output::error_writer()));
Global::exit(1);
}
progress.root.end();
progress.refresh();

return Ok(None);
let _ = log.print(std::ptr::from_mut(Output::error_writer()));
Global::exit(1);
}

let mut version = Version {
Expand All @@ -338,18 +324,14 @@ impl UpgradeCommand {
};

if !expr.is_object() {
if !SILENT {
progress.expect("infallible: progress active").end();
refresher.expect("infallible: progress active").refresh();
progress.root.end();
progress.refresh();

bun_core::pretty_errorln!(
"JSON error - expected an object but received {:?}",
core::mem::discriminant(&expr.data)
);
Global::exit(1);
}

return Ok(None);
bun_core::pretty_errorln!(
"JSON error - expected an object but received {:?}",
core::mem::discriminant(&expr.data)
);
Global::exit(1);
}

if let Some(tag_name_) = expr.as_property(b"tag_name") {
Expand All @@ -359,20 +341,16 @@ impl UpgradeCommand {
}

if version.tag.is_empty() {
if !SILENT {
progress.expect("infallible: progress active").end();
refresher.expect("infallible: progress active").refresh();

bun_core::pretty_errorln!(
"JSON Error parsing releases from GitHub: <r><red>tag_name<r> is missing?\n{}",
// `version.buf` is still empty at this point;
// print the raw payload instead.
bstr::BStr::new(metadata_body.list.as_slice())
);
Global::exit(1);
}
progress.root.end();
progress.refresh();

return Ok(None);
bun_core::pretty_errorln!(
"JSON Error parsing releases from GitHub: <r><red>tag_name<r> is missing?\n{}",
// `version.buf` is still empty at this point;
// print the raw payload instead.
bstr::BStr::new(metadata_body.list.as_slice())
);
Global::exit(1);
}

'get_asset: {
Expand Down Expand Up @@ -447,27 +425,23 @@ impl UpgradeCommand {
u32::try_from(((n.value().ceil()) as i32).max(0)).unwrap();
}
}
return Ok(Some(version));
return Ok(version);
}
}
}
}

if !SILENT {
progress.expect("infallible: progress active").end();
refresher.expect("infallible: progress active").refresh();
if let Some(name) = version.name() {
bun_core::pretty_errorln!(
"Bun v{} is out, but not for this platform ({}) yet.",
bstr::BStr::new(&name),
Version::TRIPLET
);
}

Global::exit(0);
progress.root.end();
progress.refresh();
if let Some(name) = version.name() {
bun_core::pretty_errorln!(
"Bun v{} is out, but not for this platform ({}) yet.",
bstr::BStr::new(&name),
Version::TRIPLET
);
}

Ok(None)
Global::exit(0);
}

const EXE_SUFFIX: &'static str = if cfg!(windows) { ".exe" } else { "" };
Expand Down Expand Up @@ -558,33 +532,13 @@ impl UpgradeCommand {
let use_profile = argv_contains(b"--profile");

let mut version: Version = if !use_canary {
// `Progress::start` returns `&mut Node` borrowing `refresher`;
// leak the Progress and use raw pointers so we can pass both
// `&mut refresher` and `&mut progress` to `get_latest_version`.
let refresher: *mut Progress::Progress =
bun_core::heap::into_raw(Box::new(Progress::Progress::default()));
// SAFETY: refresher is a fresh leaked allocation.
let progress: *mut Progress::Node =
unsafe { (*refresher).start(b"Fetching version tags", 0) };

let Some(version) = Self::get_latest_version::<false>(
&mut env_loader,
// SAFETY: refresher/progress point into the same leaked allocation;
// `get_latest_version` only touches them on the !SILENT error
// path (no overlapping live borrows).
Some(unsafe { &mut *refresher }),
// SAFETY: progress points into the same leaked allocation (see above).
Some(unsafe { &mut *progress }),
use_profile,
)?
else {
return Ok(());
};
let mut progress = Progress::Progress::default();
progress.start(b"Fetching version tags", 0);

let version = Self::get_latest_version(&mut env_loader, &mut progress, use_profile)?;

// SAFETY: see above.
unsafe { (*progress).end() };
// SAFETY: refresher is a leaked Box (process-lifetime); no other &mut is live.
unsafe { (*refresher).refresh() };
progress.root.end();
progress.refresh();

if !Environment::IS_CANARY {
if version.name().is_some() && version.is_current() {
Expand Down Expand Up @@ -638,15 +592,10 @@ impl UpgradeCommand {
let http_proxy = env_loader.get_http_proxy_for(&zip_url);

{
let refresher: *mut Progress::Progress =
bun_core::heap::into_raw(Box::new(Progress::Progress::default()));
// SAFETY: refresher is a fresh leaked allocation.
let progress: *mut Progress::Node =
unsafe { (*refresher).start(b"Downloading", version.size as usize) };
// SAFETY: see above.
unsafe { (*progress).unit = Progress::Unit::Bytes };
// SAFETY: refresher is a leaked Box (process-lifetime); no other &mut is live.
unsafe { (*refresher).refresh() };
let mut progress = Progress::Progress::default();
progress.start(b"Downloading", version.size as usize);
progress.root.unit = Progress::Unit::Bytes;
progress.refresh();
// Store in the process-lifetime CLI arena.
let zip_file_buffer: &'static mut MutableString = crate::cli::cli_arena()
.alloc(MutableString::init(version.size.max(1024) as usize)?);
Expand All @@ -661,10 +610,8 @@ impl UpgradeCommand {
None,
HTTP::FetchRedirect::Follow,
));
// `progress` is intentionally leaked (process-lifetime), so the
// untracked NonNull stored in `progress_node` can never dangle.
async_http.client.progress_node =
Some(NonNull::new(progress).expect("leaked Box is non-null"));
// Untracked borrow; the HTTP thread only touches the node while `send_sync` runs.
async_http.client.progress_node = Some(NonNull::from(&mut progress.root));
async_http.client.flags.reject_unauthorized = env_loader.get_tls_reject_unauthorized();

let response = async_http.send_sync(zip_file_buffer)?;
Expand Down Expand Up @@ -693,10 +640,8 @@ impl UpgradeCommand {

let bytes = zip_file_buffer.slice();

// SAFETY: refresher/progress are leaked allocations.
unsafe { (*progress).end() };
// SAFETY: refresher is a leaked Box (process-lifetime); no other &mut is live.
unsafe { (*refresher).refresh() };
progress.root.end();
progress.refresh();

if bytes.is_empty() {
bun_core::pretty_errorln!(
Expand Down
4 changes: 2 additions & 2 deletions test/cli/install/bun-upgrade.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,8 @@ function startReleaseServer(opts: {
NODE_TLS_REJECT_UNAUTHORIZED: "0",
GITHUB_API_DOMAIN: `${server.hostname}:${server.port}`,
// The upgrade-failure path exits via Global::exit(1) while the HTTP
// thread and the intentionally-leaked progress/download buffers are
// still live; LeakSanitizer reports those at exit and abort_on_error
// thread and the intentionally-leaked download buffers are still
// live; LeakSanitizer reports those at exit and abort_on_error
// turns the clean exit(1) into SIGABRT on the ASAN lane. Leak
// detection is not what these tests assert.
ASAN_OPTIONS: [env.ASAN_OPTIONS, "detect_leaks=0"].filter(Boolean).join(":"),
Expand Down