Skip to content
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

lib/posix-poll: Fix poll() ignoring fd 0 #1255

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

andreittr
Copy link
Contributor

@andreittr andreittr commented Jan 10, 2024

Description of changes

This change fixes a bug where poll() would ignore fd 0 as if it were a negative fd, which are explicitly allowed as NOP entries.

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.uk on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): N/A
  • Platform(s): N/A
  • Application(s): N/A

Additional configuration

CONFIG_LIBPOSIX_POLL=y

Test snippet (assumes fd:0 is tty stdin):

struct pollfd fds = {
	.fd = 0,
	.events = POLLIN|POLLOUT
};
int r = poll(&fds, 1, 1000);
printf("%d %d %hd\n", r, errno, fds.revents);

On Linux poll immediately returns with POLLOUT set.
On staging, with or without this fix, Unikraft waits until the timeout, because vfscore stdio files never emit events.

Applying #1226 that implements tty files more consistently we can test the above snippet.
(GCC 13 or clang is required, otherwise you also need #1245)

Without fix, Unikraft waits until the timeout, with the fix it immediately returns POLLIN|POLLOUT.

This change fixes a bug where poll() would ignore fd 0 as if it were a
negative fd, which are explicitly allowed as NOP entries.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
@andreittr andreittr requested a review from a team as a code owner January 10, 2024 12:59
@github-actions github-actions bot added area/lib Internal Unikraft Microlibrary lang/c Issues or PRs to do with C/C++ labels Jan 10, 2024
@razvand razvand added this to the v0.17.0 (Calypso) milestone Jan 10, 2024
@razvand
Copy link
Contributor

razvand commented Jan 10, 2024

Hi, @andreittr . How does this bug manifest?

@andreittr
Copy link
Contributor Author

Hi, @andreittr . How does this bug manifest?

@razvand I've updated the description; turns out it needs quite a bit of setup to trigger.

@razvand razvand removed the request for review from a team January 18, 2024 08:47
@razvand razvand modified the milestones: v0.17.0 (Calypso), v0.16.2 Feb 1, 2024
Copy link
Contributor

@mariasfiraiala mariasfiraiala left a comment

Choose a reason for hiding this comment

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

Thanks @andreittr, I was able to reproduce the issue with #1226 and gcc 13.2 and the fix works just fine. All good on my side!

Reviewed-by: Maria Sfiraiala maria.sfiraiala@gmail.com

Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

@razvand razvand added the merge Label to trigger merge action label Feb 6, 2024
@unikraft-bot unikraft-bot changed the base branch from staging to staging-1255 February 6, 2024 17:26
@unikraft-bot unikraft-bot merged commit daba410 into unikraft:staging-1255 Feb 6, 2024
13 checks passed
unikraft-bot pushed a commit that referenced this pull request Feb 6, 2024
This change fixes a bug where poll() would ignore fd 0 as if it were a
negative fd, which are explicitly allowed as NOP entries.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
GitHub-Closes: #1255
@unikraft-bot unikraft-bot added ci/merged Merged by CI and removed merge Label to trigger merge action labels Feb 6, 2024
@andreittr andreittr deleted the ttr/fix-poll0 branch February 7, 2024 13:16
SerbanSo pushed a commit to SerbanSo/unikraft-ASLR that referenced this pull request Jun 16, 2024
This change fixes a bug where poll() would ignore fd 0 as if it were a
negative fd, which are explicitly allowed as NOP entries.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
GitHub-Closes: unikraft#1255
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lib Internal Unikraft Microlibrary ci/merged Merged by CI lang/c Issues or PRs to do with C/C++
Projects
Development

Successfully merging this pull request may close these issues.

4 participants