Skip to content

Commit

Permalink
Fix potential crash when printing errors in bun install (#17027)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Jarred-Sumner
Jarred-Sumner authored Feb 6, 2025
1 parent 0c8658b commit d2cdb50
Showing 1 changed file with 35 additions and 30 deletions.
65 changes: 35 additions & 30 deletions src/install/install.zig
View file
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

0 comments on commit d2cdb50

Please sign in to comment.