Skip to content

Fix infinte loop when reading from Stdin #139539

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Fix infinte loop when reading from Stdin #139539

wants to merge 2 commits into from

Conversation

Ayush1325
Copy link
Contributor

In the old implementation, it would infinitely retry on status NOT_READY. This is not desirable since NOT_READY can be used as the way to perform NULL reads. For more context how that is useful, checkout: #139517

cc @nicholasbishop

Ayush1325 added 2 commits April 8, 2025 21:51
@Ayush1325
std: sys: stdio: uefi: Implement is_ebadf
188cfec 
- NOT_READY status should be used here.

Signed-off-by: Ayush Singh <[email protected]>
@Ayush1325
std: sys: stdio: uefi: Do not retry on NOT_READY
0b47e50 
Since NOT_READY is now used to signal read 0. Remove the loop which
would always retry on NOT_READY status.

Instead, use the following algorithm:

1. Try reading any pending characters. Return if present.
2. If no pending character, NOT_READY is returned.
3. Wait for key.
4. Return the key or error.

Signed-off-by: Ayush Singh <[email protected]>
@rustbot
Copy link
Collaborator

rustbot commented Apr 8, 2025

r? @jhpratt

rustbot has assigned @jhpratt.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 8, 2025
@Ayush1325 Ayush1325 mentioned this pull request Apr 8, 2025
Open
phip1611
phip1611 approved these changes Apr 10, 2025
View reviewed changes
Copy link

@phip1611 phip1611 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Ayush1325
Copy link
Contributor Author

@rustbot label +O-UEFI

@rustbot rustbot added the O-UEFI UEFI label Apr 11, 2025
@joboet
Copy link
Member

joboet commented Apr 13, 2025

I don't think this makes sense – yes, if the event is signalled, ReadKeyStroke should return a key, but if it doesn't, that's no reason to fail. In combination with #139517, this PR just moves the infinite loop to the wait-for-event implementation (as the null-handle event will never be signalled).

@Ayush1325
Copy link
Contributor Author

Closing this in favor of #139517

@Ayush1325 Ayush1325 closed this Apr 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@phip1611 phip1611 phip1611 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jhpratt jhpratt

Labels

O-UEFI UEFI S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Ayush1325 @rustbot @joboet @phip1611 @jhpratt