Repository navigation
prompt(), confirm(): skip a byte order mark at the first dialog read of stdin #46280
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
4
commits into
main
Choose a base branch
from
robobun/d5a5f3b6/prompt-confirm-stdin-bom
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.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
6cb8ab3
prompt(), confirm(): skip a byte order mark at the first dialog read …
robobun 8ce36e9
test: temporary record of what a PowerShell pipe writes to a Bun child
robobun ef4cb6d
test: remove the temporary record of what a PowerShell pipe writes
robobun d4f10da
Merge branch 'main' into robobun/d5a5f3b6/prompt-confirm-stdin-bom
robobun 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
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
Oops, something went wrong.
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.
🟡 (optional) After an alert() that read nothing, a later confirm() on a line beginning with a byte order mark answers false, while the same sequence through confirm() first answers true. alert() at prompt.rs:61 consumes the flag unconditionally, even when its loop at prompt.rs:55 broke on an immediate Err (EOF, EAGAIN on a non-blocking fd, or a transient read failure), whereas confirm() (prompt.rs:131) and prompt() (prompt.rs:338) return before taking it. Fix: take the flag only once a dialog has actually consumed its first byte, in all three functions, so the mark skip applies to the first bytes a dialog reads.
Why this was flagged
Trigger: stdin is a terminal or a non-blocking fd (a parent or
process.stdincan set O_NONBLOCK on fd 0, andtake_byteat src/bun_core/output.rs:2747 returns Err on EAGAIN) and the script calls alert() then confirm(). alert()'s loop at src/runtime/webcore/prompt.rs:55 breaks on the Err without reading any byte, then prompt.rs:61 swaps the flag to false. confirm() then reads the actual first bytes of stdin,EF BB BF 79 0A, but prompt.rs:75 sees the flag already false and returns 0xEF, so the_arm at prompt.rs:174 drains the line and answers false. With confirm() first, the same bytes answer true. The base branch answered false in both orders, so the new code is now order-dependent for the same input. The dismissal (argued, six finders) only considered a TTY Ctrl-D and ignored that the three dialogs take the flag at different points: confirm() and prompt() after a successful first byte, alert() regardless of success. Remedy: in alert(), take the flag only after at least one byte was read, matching the two siblings.Verification: Triggered when fd 0 is non-blocking and the writer's bytes are not yet in the pipe when alert() runs; the following confirm() then answers false exactly as the base does. alert()'s loop at src/runtime/webcore/prompt.rs:54-59 breaks on the first Err without consuming anything, and line 61 then runs
Output::take_first_dialog_read();unconditionally. The new tests (test/js/web/web-globals.test.js:392, 404) only cover alert() consuming a real line.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.
Correct.
alert()callstake_first_dialog_read()after its read loop, also when the loop read no byte (src/runtime/webcore/prompt.rs:57).confirm()andprompt()return before the call when their first read fails, so there the flag stays for the next dialog.The sequence is
falseon main too. I ran it on Bun 1.4.3. Stdin was a regular file that is empty at the first dialog and gets the mark andyafter it.alert()and thenconfirm()give[null,false]. So this change makes nothing worse there. The sequence stays unrepaired whenalert()is the dialog that reads nothing.d4f10da only merges main, so
alert()is unchanged. The description now states thatalert()takes the flag also when it read no byte. The repair is to take the flag inalert()only after it read a byte. It rides the next push to this branch, if the maintainer's answer keeps the flag.