-
Notifications
You must be signed in to change notification settings - Fork 5k
repl: ignore non-regular history files and only read the tail of large ones #38217
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
Open
robobun
wants to merge
2
commits into
main
Choose a base branch
from
farm/5171eed1/repl-history-hostile-files
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+160
−19
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
🔴 On Windows,
sys::O::NONBLOCKpassed toFile::openatmakesopenat_windows_impldropFILE_SYNCHRONOUS_IO_NONALERTfromNtCreateFile, soopen_filenow returns an asynchronous HANDLE — butsave()'swrite_allcallsWriteFilewithlpOverlapped=NULLandload()'spread_allpasses a stack-localOVERLAPPEDwhose SAFETY comment requires a synchronous handle. Before this PR both paths opened synchronous handles, so this regresses Windows history persistence (and the three new non-skipIf(isWindows)tests round-trip through it). Gate theNONBLOCK | NOCTTYOR-in behind#[cfg(not(windows))]— seeDirectoryRoute.rs:319-326for the same pattern and rationale.Extended reasoning...
What the bug is
History::open_fileunconditionally ORssys::O::NONBLOCKinto the open flags for both the load and save paths:The doc comment says "the flag has no effect on the regular file this accepts", which is true on POSIX but false in bun_sys on Windows. On Windows,
sys::O::NONBLOCKis0o4000(src/sys/lib.rs:1180), andFile::openat→openat_a→openat(Windows arm, lib.rs:3969) →openat_windows_a→openat_windows_impl(lib.rs:7137). At line 7153 that function readslet nonblock = (flags & O::NONBLOCK) != 0, and at lines 7182-7186 it does:So passing
O::NONBLOCKdropsFILE_SYNCHRONOUS_IO_NONALERTfrom theNtCreateFileoptions and yields an asynchronous (overlapped) HANDLE.Why the downstream I/O breaks
Both callers assume a synchronous handle:
save()→file.write_all()→sys::write()(src/sys/lib.rs:3652-3680) callskernel32::WriteFilewithlpOverlapped = null_mut(). Microsoft documents that on a handle opened for asynchronous I/O the caller must supply anOVERLAPPED; passingNULLon such a handle is invalid and in practice fails (there is no I/O-manager-maintained file position). History is therefore silently never saved on Windows.load()→file.pread_all()→sys::pread()(src/sys/lib.rs:3682-3722) constructs a stack-localOVERLAPPEDwithhEvent = nulland passes it toReadFile. Its own// SAFETY:comment (line 3702) states "overlappedlives for the synchronous call (handle was not opened FILE_FLAG_OVERLAPPED)", and the error match (lines 3712-3719) handlesBROKEN_PIPE/HANDLE_EOF/OPERATION_ABORTEDbut notERROR_IO_PENDING. On an asynchronous handleReadFilemay returnFALSE+ERROR_IO_PENDING;preadthen returnsErrand unwinds while the kernel still holds a pointer to the now-dead stackOVERLAPPED— a documented-SAFETY-invariant violation and a stack use-after-free.Step-by-step proof (Windows)
bun replon Windows with a.bun_repl_historyfile present.History::loadcallsSelf::open_file(path, sys::O::RDONLY).open_filecomputesflags = O::RDONLY | O::NONBLOCK | 0 | O::CLOEXEC(O::NOCTTYis 0 on Windows, lib.rs:1228).File::openatreachesopenat_windows_impl;nonblockevaluatestrue;blocking_flagbecomes0;NtCreateFileis called withoutFILE_SYNCHRONOUS_IO_NONALERT→ the returned HANDLE is asynchronous.file.pread_all(&mut content, offset)callssys::pread, which passes the address of a stackOVERLAPPEDtoReadFileon that asynchronous handle. Either the read fails immediately (history not loaded) or it returnsERROR_IO_PENDING, in which casepreadreturnsErrand the kernel later writes into freed stack.History::savecallsSelf::open_file(path, O::WRONLY | O::CREAT | O::TRUNC)→ same asynchronous handle →WriteFile(..., NULL)fails → history not saved.Why this is a regression introduced by the PR
Before this PR,
save()usedsys::open_a(path, O::WRONLY | O::CREAT | O::TRUNC, 0o600)(noNONBLOCK) andload()usedsys::File::read_from— both produced synchronous handles on Windows. The PR description states onlycargo check -p bun_runtime --target x86_64-pc-windows-msvcwas run, not the tests. Three of the new tests — "keeps the newest entries…", "loads a file exactly at the size limit…", "loads only the tail of a file over the size limit" — are not gated byisWindows, setUSERPROFILEviahomeEnv(), and assert the exact contents of the saved history file, so a brokensave()fails Windows CI.The codebase already documents this hazard
src/runtime/server/DirectoryRoute.rs:319-326has the exact pattern with a comment explaining why:The unconditional-
NONBLOCKsites elsewhere in the tree (Blob.rs:1481,FileReader.rs:206) all go throughbun_sys::open(), which on Windows routes tosys_uv::open— a different codepath that does not hitNtCreateFile.Fix
Gate the POSIX-only flags on
cfg: