-
Notifications
You must be signed in to change notification settings - Fork 3.8k
fix string corruption in FS entry cache #2055
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
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 | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -1819,7 +1819,7 @@ pub const Resolver = struct { | |||||||
| } | ||||||||
| fn dirInfoForResolution( | ||||||||
| r: *ThisResolver, | ||||||||
| dir_path: []const u8, | ||||||||
| dir_path: string, | ||||||||
| package_id: Install.PackageID, | ||||||||
| ) !?*DirInfo { | ||||||||
| std.debug.assert(r.package_manager != null); | ||||||||
|
|
@@ -1833,7 +1833,7 @@ pub const Resolver = struct { | |||||||
| var cached_dir_entry_result = rfs.entries.getOrPut(dir_path) catch unreachable; | ||||||||
|
|
||||||||
| var dir_entries_option: *Fs.FileSystem.RealFS.EntriesOption = undefined; | ||||||||
| var needs_iter: bool = true; | ||||||||
| var needs_iter = true; | ||||||||
| var open_dir = std.fs.cwd().openIterableDir(dir_path, .{}) catch |err| { | ||||||||
| switch (err) { | ||||||||
| error.FileNotFound => unreachable, | ||||||||
|
|
@@ -1855,7 +1855,9 @@ pub const Resolver = struct { | |||||||
| if (needs_iter) { | ||||||||
| const allocator = r.fs.allocator; | ||||||||
| dir_entries_option = rfs.entries.put(&cached_dir_entry_result, .{ | ||||||||
| .entries = Fs.FileSystem.DirEntry.init(dir_path), | ||||||||
| .entries = Fs.FileSystem.DirEntry.init( | ||||||||
| Fs.FileSystem.DirnameStore.instance.append(string, dir_path) catch unreachable, | ||||||||
|
Collaborator
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. I think there are few cases where we need to clone this. The caller already cloned it except for the case you're running into, which has to do with automatic package installs
Contributor
Author
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. Okay let me look into that a bit more 👍
Collaborator
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. We could add a debug assertion which checks: Fs.FileSystem.DirnameStore.instance.isSliceInBuffer(dir_path) or bun.isHeapMemory(dir_path)Then we can find the callers of this function which are not cloning the dir name
Contributor
Author
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. There is only one user for this function: Line 1717 in 30e82c5
string of which is supplied by PackageManager.pathForResolution():Line 1662 in 30e82c5
which is using a threadlocal buffer:Line 260 in 30e82c5
... doesn't look very cloned to me 😅 |
||||||||
| ), | ||||||||
| }) catch unreachable; | ||||||||
|
|
||||||||
| if (FeatureFlags.store_file_descriptors) { | ||||||||
|
|
@@ -1870,7 +1872,7 @@ pub const Resolver = struct { | |||||||
|
|
||||||||
| // We must initialize it as empty so that the result index is correct. | ||||||||
| // This is important so that browser_scope has a valid index. | ||||||||
| var dir_info_ptr = r.dir_cache.put(&dir_cache_info_result, DirInfo{}) catch unreachable; | ||||||||
| var dir_info_ptr = r.dir_cache.put(&dir_cache_info_result, .{}) catch unreachable; | ||||||||
|
|
||||||||
| try r.dirInfoUncached( | ||||||||
| dir_info_ptr, | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,11 +53,11 @@ it("should install and run specified version", async () => { | |
| expect(await exited).toBe(0); | ||
| }); | ||
|
|
||
| it("should download dependencies to run local file", async () => { | ||
| it("should download dependency to run local file", async () => { | ||
| await writeFile( | ||
| join(x_dir, "test.js"), | ||
| ` | ||
| import { minify } from "uglify-js"; | ||
| const { minify } = require("uglify-js@3.17.4"); | ||
|
|
||
| console.log(minify("print(6 * 7)").code); | ||
| `, | ||
|
|
@@ -82,3 +82,42 @@ console.log(minify("print(6 * 7)").code); | |
| expect(await exited).toBe(0); | ||
| expect(await readdirSorted(x_dir)).toEqual([".cache", "test.js"]); | ||
| }); | ||
|
|
||
| it("should download dependencies to run local file", async () => { | ||
| await writeFile( | ||
| join(x_dir, "test.js"), | ||
| ` | ||
| import { file } from "bun"; | ||
| import decompress from "[email protected]"; | ||
|
|
||
| const buffer = await file("${join(import.meta.dir, "baz-0.0.3.tgz")}").arrayBuffer(); | ||
| for (const entry of await decompress(Buffer.from(buffer))) { | ||
| console.log(\`\${entry.type}: \${entry.path}\`); | ||
| } | ||
| `, | ||
| ); | ||
| const { stdout, stderr, exited } = spawn({ | ||
| cmd: [bunExe(), "test.js"], | ||
| cwd: x_dir, | ||
| stdout: null, | ||
| stdin: "pipe", | ||
| stderr: "pipe", | ||
| env: { | ||
| ...env, | ||
| BUN_INSTALL_CACHE_DIR: join(x_dir, ".cache"), | ||
| }, | ||
| }); | ||
| expect(stderr).toBeDefined(); | ||
| const err = await new Response(stderr).text(); | ||
| expect(err).toBe(""); | ||
| expect(stdout).toBeDefined(); | ||
| const out = await new Response(stdout).text(); | ||
| expect(out.split(/\r?\n/)).toEqual([ | ||
| "directory: package/", | ||
| "file: package/index.js", | ||
| "file: package/package.json", | ||
| "", | ||
| ]); | ||
| expect(await exited).toBe(0); | ||
| expect(await readdirSorted(x_dir)).toEqual([".cache", "test.js"]); | ||
| }); | ||
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.
you can tell this is some of the earliest code in bun because i did not know zig very well