Skip to content
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

Fix potential crash when printing errors in bun install #17027

Merged
merged 1 commit into from
Feb 6, 2025
Merged

Fix potential crash when printing errors in bun install #17027

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
65 changes: 35 additions & 30 deletions src/install/install.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,8 @@ pub const Aligner = struct {
};

const NetworkTask = struct {
http: AsyncHTTP = undefined,
unsafe_http_client: AsyncHTTP = undefined,
response: bun.http.HTTPClientResult = .{},
task_id: u64,
url_buf: []const u8 = &[_]u8{},
retried: u16 = 0,
Expand All @@ -282,10 +283,11 @@ const NetworkTask = struct {
};
pub const DedupeMap = std.HashMap(u64, DedupeMapEntry, IdentityContext(u64), 80);

pub fn notify(this: *NetworkTask, async_http: *AsyncHTTP, _: anytype) void {
pub fn notify(this: *NetworkTask, async_http: *AsyncHTTP, result: bun.http.HTTPClientResult) void {
defer this.package_manager.wake();
async_http.real.?.* = async_http.*;
async_http.real.?.response_buffer = async_http.response_buffer;
this.response = result;
this.package_manager.async_network_task_queue.push(this);
}

Expand Down Expand Up @@ -454,13 +456,13 @@ const NetworkTask = struct {
this.allocator = allocator;

const url = URL.parse(this.url_buf);
this.http = AsyncHTTP.init(allocator, .GET, url, header_builder.entries, header_builder.content.ptr.?[0..header_builder.content.len], &this.response_buffer, "", this.getCompletionCallback(), HTTP.FetchRedirect.follow, .{
this.unsafe_http_client = AsyncHTTP.init(allocator, .GET, url, header_builder.entries, header_builder.content.ptr.?[0..header_builder.content.len], &this.response_buffer, "", this.getCompletionCallback(), HTTP.FetchRedirect.follow, .{
.http_proxy = this.package_manager.httpProxy(url),
});
this.http.client.flags.reject_unauthorized = this.package_manager.tlsRejectUnauthorized();
this.unsafe_http_client.client.flags.reject_unauthorized = this.package_manager.tlsRejectUnauthorized();

if (PackageManager.verbose_install) {
this.http.client.verbose = .headers;
this.unsafe_http_client.client.verbose = .headers;
}

this.callback = .{
Expand All @@ -471,14 +473,14 @@ const NetworkTask = struct {
};

if (PackageManager.verbose_install) {
this.http.verbose = .headers;
this.http.client.verbose = .headers;
this.unsafe_http_client.verbose = .headers;
this.unsafe_http_client.client.verbose = .headers;
}

// Incase the ETag causes invalidation, we fallback to the last modified date.
if (last_modified.len != 0 and bun.getRuntimeFeatureFlag("BUN_FEATURE_FLAG_LAST_MODIFIED_PRETEND_304")) {
this.http.client.flags.force_last_modified = true;
this.http.client.if_modified_since = last_modified;
this.unsafe_http_client.client.flags.force_last_modified = true;
this.unsafe_http_client.client.if_modified_since = last_modified;
}
}

Expand All @@ -487,7 +489,7 @@ const NetworkTask = struct {
}

pub fn schedule(this: *NetworkTask, batch: *ThreadPool.Batch) void {
this.http.schedule(this.allocator, batch);
this.unsafe_http_client.schedule(this.allocator, batch);
}

pub const ForTarballError = OOM || error{
Expand Down Expand Up @@ -547,12 +549,12 @@ const NetworkTask = struct {

const url = URL.parse(this.url_buf);

this.http = AsyncHTTP.init(allocator, .GET, url, header_builder.entries, header_buf, &this.response_buffer, "", this.getCompletionCallback(), HTTP.FetchRedirect.follow, .{
this.unsafe_http_client = AsyncHTTP.init(allocator, .GET, url, header_builder.entries, header_buf, &this.response_buffer, "", this.getCompletionCallback(), HTTP.FetchRedirect.follow, .{
.http_proxy = this.package_manager.httpProxy(url),
});
this.http.client.flags.reject_unauthorized = this.package_manager.tlsRejectUnauthorized();
this.unsafe_http_client.client.flags.reject_unauthorized = this.package_manager.tlsRejectUnauthorized();
if (PackageManager.verbose_install) {
this.http.client.verbose = .headers;
this.unsafe_http_client.client.verbose = .headers;
}
}
};
Expand Down Expand Up @@ -736,7 +738,7 @@ pub const Task = struct {
const package_manifest = Npm.Registry.getPackageMetadata(
allocator,
manager.scopeForPackageName(manifest.name.slice()),
manifest.network.http.response.?,
(manifest.network.response.metadata orelse @panic("Assertion failure: Expected metadata to be set")).response,
body,
&this.log,
manifest.name.slice(),
Expand Down Expand Up @@ -6402,7 +6404,7 @@ pub const PackageManager = struct {
}
}

if (!has_network_error and task.http.response == null) {
if (!has_network_error and task.response.metadata == null) {
has_network_error = true;
const min = manager.options.min_simultaneous_requests;
const max = AsyncHTTP.max_simultaneous_requests.load(.monotonic);
Expand All @@ -6412,8 +6414,8 @@ pub const PackageManager = struct {
}

// Handle retry-able errors.
if (task.http.response == null or task.http.response.?.status_code > 499) {
const err = task.http.err orelse error.HTTPError;
if (task.response.metadata == null or task.response.metadata.?.response.status_code > 499) {
const err = task.response.fail orelse error.HTTPError;

if (task.retried < manager.options.max_retry_count) {
task.retried += 1;
Expand All @@ -6433,9 +6435,9 @@ pub const PackageManager = struct {
}
}

const response = task.http.response orelse {
const metadata = task.response.metadata orelse {
// Handle non-retry-able errors.
const err = task.http.err orelse error.HTTPError;
const err = task.response.fail orelse error.HTTPError;

if (@TypeOf(callbacks.onPackageManifestError) != void) {
callbacks.onPackageManifestError(
Expand Down Expand Up @@ -6478,6 +6480,7 @@ pub const PackageManager = struct {

continue;
};
const response = metadata.response;

if (response.status_code > 399) {
if (@TypeOf(callbacks.onPackageManifestError) != void) {
Expand Down Expand Up @@ -6507,15 +6510,15 @@ pub const PackageManager = struct {
logger.Loc.Empty,
manager.allocator,
"<r><red><b>GET<r><red> {s}<d> - {d}<r>",
.{ task.http.client.url.href, response.status_code },
.{ metadata.url, response.status_code },
) catch bun.outOfMemory();
} else {
manager.log.addWarningFmt(
null,
logger.Loc.Empty,
manager.allocator,
"<r><yellow><b>GET<r><yellow> {s}<d> - {d}<r>",
.{ task.http.client.url.href, response.status_code },
.{ metadata.url, response.status_code },
) catch bun.outOfMemory();
}
if (manager.subcommand != .remove) {
Expand All @@ -6534,7 +6537,7 @@ pub const PackageManager = struct {

if (comptime log_level.isVerbose()) {
Output.prettyError(" ", .{});
Output.printElapsed(@as(f64, @floatFromInt(task.http.elapsed)) / std.time.ns_per_ms);
Output.printElapsed(@as(f64, @floatFromInt(task.unsafe_http_client.elapsed)) / std.time.ns_per_ms);
Output.prettyError("\n<d>Downloaded <r><green>{s}<r> versions\n", .{name.slice()});
Output.flush();
}
Expand Down Expand Up @@ -6582,7 +6585,7 @@ pub const PackageManager = struct {
manager.task_batch.push(ThreadPool.Batch.from(manager.enqueueParseNPMPackage(task.task_id, name, task)));
},
.extract => |*extract| {
if (!has_network_error and task.http.response == null) {
if (!has_network_error and task.response.metadata == null) {
has_network_error = true;
const min = manager.options.min_simultaneous_requests;
const max = AsyncHTTP.max_simultaneous_requests.load(.monotonic);
Expand All @@ -6591,8 +6594,8 @@ pub const PackageManager = struct {
}
}

if (task.http.response == null or task.http.response.?.status_code > 499) {
const err = task.http.err orelse error.TarballFailedToDownload;
if (task.response.metadata == null or task.response.metadata.?.response.status_code > 499) {
const err = task.response.fail orelse error.TarballFailedToDownload;

if (task.retried < manager.options.max_retry_count) {
task.retried += 1;
Expand All @@ -6618,8 +6621,8 @@ pub const PackageManager = struct {
}
}

const response = task.http.response orelse {
const err = task.http.err orelse error.TarballFailedToDownload;
const metadata = task.response.metadata orelse {
const err = task.response.fail orelse error.TarballFailedToDownload;

if (@TypeOf(callbacks.onPackageDownloadError) != void) {
const package_id = manager.lockfile.buffers.resolutions.items[extract.dependency_id];
Expand Down Expand Up @@ -6674,6 +6677,8 @@ pub const PackageManager = struct {
continue;
};

const response = metadata.response;

if (response.status_code > 399) {
if (@TypeOf(callbacks.onPackageDownloadError) != void) {
const err = switch (response.status_code) {
Expand Down Expand Up @@ -6705,7 +6710,7 @@ pub const PackageManager = struct {
manager.allocator,
"<r><red><b>GET<r><red> {s}<d> - {d}<r>",
.{
task.http.client.url.href,
metadata.url,
response.status_code,
},
) catch bun.outOfMemory();
Expand All @@ -6716,7 +6721,7 @@ pub const PackageManager = struct {
manager.allocator,
"<r><yellow><b>GET<r><yellow> {s}<d> - {d}<r>",
.{
task.http.client.url.href,
metadata.url,
response.status_code,
},
) catch bun.outOfMemory();
Expand All @@ -6737,7 +6742,7 @@ pub const PackageManager = struct {

if (comptime log_level.isVerbose()) {
Output.prettyError(" ", .{});
Output.printElapsed(@as(f64, @floatCast(@as(f64, @floatFromInt(task.http.elapsed)) / std.time.ns_per_ms)));
Output.printElapsed(@as(f64, @floatCast(@as(f64, @floatFromInt(task.unsafe_http_client.elapsed)) / std.time.ns_per_ms)));
Output.prettyError("<d> Downloaded <r><green>{s}<r> tarball\n", .{extract.name.slice()});
Output.flush();
}
Expand Down