Skip to content
Open
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
62 changes: 21 additions & 41 deletions src/install/PackageManager/processDependencyList.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
use crate::package_manager_real::{
PackageManager, TaskCallbackList, enqueue, resolution as pm_resolution,
};
use crate::repository_real::{Repository, RepositoryExt as _};
use crate::repository_real::RepositoryExt as _;
use crate::resolution::{ResolutionType, Tag as ResolutionTag, TaggedValue};
use crate::{
DependencyID, ExtractData, Features, INVALID_PACKAGE_ID, PackageID, Resolution,
TaskCallbackContext, initialize_store,
ExtractData, Features, INVALID_PACKAGE_ID, PackageID, Resolution, TaskCallbackContext,
dependency, initialize_store,
};

// ──────────────────────────────────────────────────────────────────────────
Expand All @@ -29,16 +29,12 @@
pub struct GitResolver<'a> {
pub(crate) resolved: &'a [u8],
pub(crate) resolution: &'a Resolution,
pub(crate) dep_id: DependencyID,
/// Owned scratch buffer that
/// `Package::parse_with_json` may assign when the package.json `name`
/// field is missing (see `ResolverContext::set_new_name`).
pub(crate) new_name: Vec<u8>,
/// `Repository::fallback_package_name`, copied out before parsing starts
/// because it is sliced from the lockfile string buffer parsing appends to.
pub(crate) fallback_name: &'a [u8],
}

impl<'a> ResolverContext for GitResolver<'a> {
const IS_GIT_RESOLVER: bool = true;

fn check_bundled_dependencies() -> bool {
true
}
Expand Down Expand Up @@ -67,20 +63,8 @@
}))
}

fn resolution(&self) -> &Resolution {
self.resolution
}
fn dep_id(&self) -> DependencyID {
self.dep_id
}
fn new_name(&self) -> &[u8] {
&self.new_name
}
fn set_new_name(&mut self, name: Vec<u8>) {
self.new_name = name;
}
fn take_new_name(&mut self) -> Vec<u8> {
core::mem::take(&mut self.new_name)
fn fallback_name(&self) -> Option<Vec<u8>> {
Some(self.fallback_name.to_vec())
}
}

Expand Down Expand Up @@ -117,6 +101,10 @@
_ => unreachable!(),
}))
}

fn fallback_name(&self) -> Option<Vec<u8>> {
Some(dependency::fallback_package_name(self.url).to_vec())
}
}

// ──────────────────────────────────────────────────────────────────────────
Expand All @@ -128,19 +116,21 @@
pub(crate) fn process_extracted_tarball_package(
&mut self,
package_id: &mut PackageID,
dep_id: DependencyID,
resolution: &Resolution,
data: &ExtractData,
log_level: LogLevel,
) -> Option<Package> {
match resolution.tag {
ResolutionTag::Git | ResolutionTag::Github => {
let mut package = 'package: {
let fallback_name: Vec<u8> = resolution
.repository()
.fallback_package_name(self.lockfile.buffers.string_bytes.as_slice())
.to_vec();
let mut resolver = GitResolver {
resolved: &data.resolved,
resolution,
dep_id,
new_name: Vec::new(),
fallback_name: &fallback_name,
};

let mut pkg = Package::default();
Expand All @@ -164,10 +154,10 @@
Output::err(
err,
"failed to parse package.json for <b>{}<r>",
format_args!("{}", resolution.fmt_url(string_buf)),
);
}
Global::crash();
self.crash();

Check warning on line 160 in src/install/PackageManager/processDependencyList.rs

View check run for this annotation

Claude / Claude Code Review

Incomplete Global::crash() → self.crash() migration in process_extracted_tarball_package

The `Global::crash()` → `self.crash()` migration was applied to the Git/Github and LocalTarball/RemoteTarball arms but not the fallback `_` arm's `parse_package_json` error branch (line ~300), which also writes to `log`. Per REVIEW.md "Fix the whole class in the same PR … parallel switch arms", that third site should get the same treatment (and `Global` in the `use bun_core::{Global, Output};` import then becomes unused).
Comment thread
robobun marked this conversation as resolved.
}

let has_scripts = pkg.scripts.has_any() || {
Expand All @@ -184,25 +174,15 @@
}

// package.json doesn't exist, no dependencies to worry about but we need to decide on a name for the dependency
// tag is `.git` or `.github`; both store `Repository`.
let repo = *resolution.repository();

let new_name = Repository::create_dependency_name_from_version_literal(
&repo,
self.lockfile.buffers.string_bytes.as_slice(),
&self.lockfile.buffers.dependencies[dep_id as usize],
);
// `defer manager.allocator.free(new_name)` — `new_name: Vec<u8>` drops at scope end.

{
let mut builder = self.lockfile.string_builder();

builder.count(&new_name);
builder.count(&fallback_name);
resolver.count(&mut builder, &Expr::default());

bun_core::handle_oom(builder.allocate());

let name = builder.append::<ExternalString>(&new_name);
let name = builder.append::<ExternalString>(&fallback_name);
pkg.name = name.value;
pkg.name_hash = name.hash;

Expand Down Expand Up @@ -264,7 +244,7 @@
err.name(),
);
}
Global::crash();
self.crash();
}

let has_scripts = package.scripts.has_any() || {
Expand Down
2 changes: 0 additions & 2 deletions src/install/PackageManager/runTasks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1152,7 +1152,6 @@ pub fn run_tasks<C: RunTasksCallbacks>(
}
} else if let Some(pkg) = manager.process_extracted_tarball_package(
&mut package_id,
dependency_id,
resolution,
// Tag-checked accessor (debug_asserts Extract|LocalTarball);
// shared `&task` here coexists with the field-disjoint
Expand Down Expand Up @@ -1481,7 +1480,6 @@ pub fn run_tasks<C: RunTasksCallbacks>(
}
} else if let Some(pkg) = manager.process_extracted_tarball_package(
&mut package_id,
git_checkout.dependency_id,
resolution,
// Tag-checked accessor (debug_asserts GitCheckout); shared
// `&task` here coexists with the field-disjoint
Expand Down
30 changes: 30 additions & 0 deletions src/install/dependency.rs
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,36 @@ pub(crate) fn is_safe_install_folder_name(name: &[u8]) -> bool {
true
}

/// bun.lock stores a package as `"<name>@<resolution>"` and takes it apart again with
/// `split_name_and_version`, then rejects names that fail `is_safe_install_folder_name`.
/// A name containing any `@` other than a scope marker (`a@b`, `@s/a@b`) or failing that
/// check would therefore be written out but could never be loaded back.
pub(crate) fn is_safe_lockfile_package_name(name: &[u8]) -> bool {
is_safe_install_folder_name(name) && !strings::contains_char(&name[1..], b'@')
}

const FALLBACK_PACKAGE_NAME: &[u8] = b"unnamed-package";

/// Name for a package whose own package.json has no `name`: the last component of where it
/// came from, so `../pkgs/foo`, `https://host/foo.tgz?token=x` and `https://host/user/foo`
/// all become `foo`. Always `is_safe_lockfile_package_name`, since it ends up in bun.lock.
pub(crate) fn fallback_package_name(location: &[u8]) -> &[u8] {
let without_query = match strings::index_of_char(location, b'?') {
Some(query_start) => &location[..query_start as usize],
None => location,
};
let basename = bun_paths::basename(without_query);
let name = strings::without_suffix_comptime(
strings::without_suffix_comptime(basename, b".tgz"),
b".tar.gz",
);
if is_safe_lockfile_package_name(name) {
name
} else {
FALLBACK_PACKAGE_NAME
}
}

/// assumes version is valid
pub fn without_build_tag(version: &[u8]) -> &[u8] {
if let Some(plus) = strings::index_of_char(version, b'+') {
Expand Down
Loading
Loading