Repository navigation
Make --linker=isolated the default for bun install
#23311
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 8 commits
9b8a252
88a2f36
257ba67
46244ae
7b31bbe
3c031da
6e39987
e3bf024
9722655
0c2951c
d116f9c
84c04c8
0b411c4
6778cee
d4eeaed
1b901d2
e52b3f0
3e253e0
e9d6b0e
51d83a3
93bef3b
6df179b
d0680cf
cec1af1
70ac9a7
efdc068
690c48e
ba454ca
c44b552
eb1fc82
b43e29e
31027da
28038b3
d2e383f
848b20a
b3f8978
92c4561
f52b4e9
2404d30
d1e145c
9288f8e
3ff799b
41a073c
f30ff00
794559a
0bffe4c
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 | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -841,6 +841,15 @@ pub fn installIsolatedPackages( | |||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!missing_from_cache) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (patch_info == .patch) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| installer.applyPackagePatch(entry_id, patch_info.patch) catch |err| { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // monotonic is okay because we haven't started the task yet (it isn't running | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // on another thread) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| entry_steps[entry_id.get()].store(.done, .monotonic); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| installer.onTaskFail(entry_id, .{ .patching = err }); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+934
to
+944
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. patch_log not freed on success (memory leak). Deinit the log when no errors to avoid leaking allocations. - var patch_log: bun.logger.Log = .init(manager.allocator);
- installer.applyPackagePatch(entry_id, patch_info.patch, &patch_log);
- if (patch_log.hasErrors()) {
+ var patch_log: bun.logger.Log = .init(manager.allocator);
+ installer.applyPackagePatch(entry_id, patch_info.patch, &patch_log);
+ if (patch_log.hasErrors()) {
// monotonic is okay because we haven't started the task yet (it isn't running
// on another thread)
entry_steps[entry_id.get()].store(.done, .monotonic);
installer.onTaskFail(entry_id, .{ .patching = patch_log });
continue;
- }
+ } else {
+ patch_log.deinit();
+ }Additionally, confirm ownership: if onTaskFail assumes ownership of patch_log, ensure no deinit happens on the error path elsewhere. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||
| installer.startTask(entry_id); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
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.
Resource leak if FileCopier initialization fails.
The
cached_package_folderdirectory opened on line 766 is not closed ifFileCopier.initfails on line 774. Thedefer copier.deinit()on line 780 only executes if init succeeds, leaving the directory handle leaked on the error path.Apply this diff to ensure cleanup:
const cached_package_folder = try cache_dir.openDir(cache_dir_subpath, .{ .iterate = true }); +defer cached_package_folder.close(); const ignore_directories: []const bun.OSPathSlice = &.{ comptime bun.OSPathLiteral("node_modules"), comptime bun.OSPathLiteral(".git"), comptime bun.OSPathLiteral("CMakeFiles"), }; var copier: bun.install.FileCopier = try .init( .fromStdDir(cached_package_folder), src_path, dest_subpath, ignore_directories, ); defer copier.deinit(); try copier.copy().unwrap();Note: Verify that
FileCopier.initdoes not take ownership of the directory handle. If it does transfer ownership, the explicitclose()may cause a double-free. In that case, you'll need to conditionally close it only on init failure.📝 Committable suggestion