Repository navigation
install: keep the project's .env out of the install's git children and HTTP client #42597
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.
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
Changes from all commits
a91b0fc
8248f46
d1df4a6
0bb17e5
6c40f30
6da9f91
1ec219e
f09a5f5
9f58eee
9c82bd3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ use bun_sys::windows::libuv as uv; | |
| use bun_threading::thread_pool as ThreadPool; | ||
|
|
||
| use crate::install::{ExtractData, ExtractDataJson}; | ||
| use crate::package_manager_real::ProcessOnlyEnv; | ||
| use crate::package_manager_task::{self as Task, Tag}; | ||
| use crate::repository::{GitEnv, Repository, RepositoryExt as _, is_safe_resolved_tag}; | ||
| use crate::{Error, PackageManager}; | ||
|
|
@@ -593,7 +594,7 @@ impl GitSubprocess { | |
| /// Spawns `git <args>` with stdout and stderr captured. On `Ok` the child | ||
| /// may already have exited and freed `this`. | ||
| fn spawn(this: ThisPtr<Self>, args: &[&[u8]]) -> Result<(), Error> { | ||
| let env = GitEnv::get(this.manager().env_mut()); | ||
| let env = GitEnv::get(this.manager().tooling_env()); | ||
| let Some(git) = &env.git else { | ||
| this.log_error(format_args!( | ||
| "\"git\" is not installed (needed for \"{}\")", | ||
|
|
@@ -776,8 +777,11 @@ impl GitSubprocess { | |
| && strings::contains(stderr, b"not") | ||
| && strings::contains(stderr, b"found")) | ||
| || strings::contains(stderr, b"does not exist")); | ||
| if !ok && !not_found { | ||
| this.report_failure(status, stderr); | ||
| if !ok { | ||
| if !not_found { | ||
| this.report_failure(status, stderr); | ||
| } | ||
| this.manager().note_dotenv_only_vars(ProcessOnlyEnv::Git); | ||
|
Comment on lines
+780
to
+784
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 (optional) Users whose install succeeds can still get the new Why this was flaggedA project Verification: nit. A project |
||
| } | ||
| let name = BStr::new(&this.name); | ||
| match step { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟣 pre-existing, not blocking: Pre-existing (memory safety): a script that writes a proxy variable to process.env while an auto-install request is pending can make the HTTP thread dial a freed buffer. http_proxy() at src/install/PackageManager.rs:902 detaches the loader reference to 'static, but on the runtime path tooling_env() is the VM loader, whose map value Bun__setEnvValue replaces and frees (src/runtime/api/BunObject.rs:2051, :2071). Fix: make the install's requests own their proxy bytes, as fetch() does with url_proxy_buffer, so the URL stored in NetworkTask.rs:666 and :887 never borrows the env map, and state that invariant in the SAFETY comment instead of "process_env is never written after init()", which does not cover the env() fallback.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A
bun app.tsrun without node_modules imports a package that auto-install must fetch; the resolver enters init_with_runtime_once, which sets process_env to None (src/install/PackageManager.rs:2584), so tooling_env() (:893-895) returns env(), the VM's transpiler loader. http_proxy() (:897-903) laundered that reference to 'static and returns a URL<'static> whose hostname slice points into the HashTableValue Box of the HTTPS_PROXY entry (src/dotenv/env_loader.rs:325-331). NetworkTask::for_manifest and for_tarball store it in the AsyncHTTP (src/install/NetworkTask.rs:666, :887); the HTTP thread dials url.hostname from it (src/http/HTTPThread.rs:506). Auto-install is asynchronous in the runtime, so JS can runprocess.env.HTTPS_PROXY = othermeanwhile; Bun__setEnvValue puts a new Box or removes the entry (src/runtime/api/BunObject.rs:2051, :2071), freeing the bytes the pending request still points at. The base branch had the same borrow through env_mut(); the diff rewrites the function and adds a SAFETY comment whose stated invariant covers only process_env, not this fallback.Verification: pre-existing (memory safety; the base reaches the same freed bytes by the same route). JS writes a proxy variable to
process.envwhile an auto-install request is in flight.Bun__setEnvValue(src/runtime/api/BunObject.rs:2047-2071) writesvm.transpiler.env_mut().map, freeing theBox<[u8]>the HTTP thread'sclient.http_proxyborrows. The SAFETY comment does not cover the runtime path.