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

Handle EINTR correctly in forkserver #3

Merged
merged 1 commit into from
Jun 28, 2024
Merged

Conversation

arata-nvm
Copy link
Collaborator

@arata-nvm arata-nvm commented Jun 24, 2024

Type of PR

  • Changes related to the roadmap (e.g., TODO.md) (type: A) -> Create an issue corresponding to the PR in advance, and refer to this PR in the issue.
  • Changes that are not related to the roadmap
    • Change with multiple possible solutions to the issue (type: B-1) -> Create an issue corresponding to the PR in advance and refer to this PR in the issue.
    • Change with a single solution (type: B-2) -> There is no need to create an issue corresponding to the PR in advance. Please discuss it in this PR.

Related Issue

Importance of PR

  • Importance of the issue
    • Large (based on several days to weeks of discussion and verification, e.g., this issue is a blocking issue for other issues on the roadmap, etc.)
    • Medium (based on a few hours to a day of discussion and verification, e.g., this issue is a blocking issue for another minor issue)
    • Small (apparent changes such as build error)
  • Complexity of the solution (code, tests, etc.)
    • Large (requires several days to several weeks of review)
    • Medium (requires several hours to a day of review)
    • Small (trivial changes, such as build error)

PR Overview

fuzzuf-cc has an issue with forkserver terminating with the following error:

[!] [FdChannel] Failed to send: ExecutePUT (Requested 4 bytes, Sent -1 bytes): Broken pipe

This is because read function can return EINTR, which is not being handled correctly.
This PR fixes this issue by modifying Recv function in forkserver to handle EINTR.

Concerns (Optional)

  • Performance
  • Source Code Quality

The PR author should fill in the following checklist when submitting this PR.

Optional Entries

  • If this PR is a PR type A/B-1, there is a cross-link between this PR and the related issue.

Mandatory Entries

  • The PR title is a summary of the changes.
  • Completed each required field of the PR.

The PR author should fill out the following checklist in the comments to confirm that this PR is ready to be merged

  • CI is green or confirmed test run results.
  • All change suggestions from reviewers have been resolved (fixed or foregone).

The maintainer of this repository will set up a reviewer for each PR.
PR reviewers should review this PR in terms of the checklist below before moving on to a detailed code review. Please comment on their initial response by filling in the checklist below.

Optional Entries

  • The reviewer assigned more reviewers if needed.
  • The reviewer noted that it is necessary to break out some of the changes in this PR into other PRs if needed.
  • The reviewer noted that the initial response is insufficient if needed.

Mandatory Entries

  • The title of this PR summarizes the changes made by this PR properly.
  • The target branch of this PR is as intended.
  • The reviewer understands the issues in this PR.
  • The reviewer plans to review with an appropriate workload based on the importance of this PR.

When the PR reviewer concludes that this PR is ready to be merged, please fill in the checklist below by posting it in the comment. If there is more than one reviewer, please do this on your own.

Optional Entries

  • The reviewer noted that if you believe that new tests are needed to evaluate this PR, they have been noted.
  • If minor refactorings are not mentioned in the PR, I understand the intent.
  • If this PR is a PR type A/B-1, we have agreed on this PR's design, direction, and granularity in the related issue.

Mandatory Entries

  • The reviewer understands how this PR addresses the issue and the specific changes.
  • This PR uses the best possible issue resolution that the reviewer can think of.
  • This PR is ready to be merged.

@arata-nvm arata-nvm self-assigned this Jun 24, 2024
@arata-nvm arata-nvm marked this pull request as ready for review June 25, 2024 02:53
@arata-nvm arata-nvm requested a review from retrage June 25, 2024 03:13
Copy link
Contributor

@retrage retrage left a comment

Choose a reason for hiding this comment

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

It looks good to me

@arata-nvm arata-nvm merged commit 01b2c3e into main Jun 28, 2024
2 checks passed
@arata-nvm arata-nvm deleted the fix/forkserver-recv branch June 28, 2024 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants