Skip to content

fix: correct fcntl signal owner handling#1876

Open
flynojy wants to merge 1 commit intoDragonOS-Community:masterfrom
flynojy:claude/fix-issue-1841-S9w1m
Open

fix: correct fcntl signal owner handling#1876
flynojy wants to merge 1 commit intoDragonOS-Community:masterfrom
flynojy:claude/fix-issue-1841-S9w1m

Conversation

@flynojy
Copy link
Copy Markdown

@flynojy flynojy commented Apr 24, 2026

Summary

Fix fcntl signal owner handling and add dunitest coverage for the related behavior.

Changes

  • Correct fcntl signal owner handling logic in VFS/fcntl code.
  • Update related fcntl syscall handling.
  • Add a new fcntl_signal dunitest case.
  • Add the new test case to the dunitest whitelist.

Testing

  • Added user/apps/tests/dunitest/suites/normal/fcntl_signal.cc.
  • Not fully verified locally because the PR CI workflow is currently awaiting maintainer approval.

@github-actions github-actions Bot added the Bug fix A bug is fixed in this pull request label Apr 24, 2026
@jiuyue486
Copy link
Copy Markdown

this is also my account. I accidentally used the wrong one to open this merge request

int main(int argc, char** argv) {
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
} No newline at end of file
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No new lines at the file end?

close(fd);
}

// F_SETSIG 后 F_GETSIG 应返回同一值。
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please write in English

@fslongjin
Copy link
Copy Markdown
Member

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7278ed9114

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

.read()
.get_file_by_fd(fd)
.ok_or(SystemError::EBADF)?;
file.set_fasync_signum(arg as i32);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Use configured F_SETSIG value for async I/O delivery

This branch records the requested signal with file.set_fasync_signum(...), but the notification path still always emits SIGIO (FAsyncItems::send_sigio_to_process hard-codes Signal::SIGIO_OR_POLL), so fcntl(fd, F_SETSIG, <nonzero>) never changes the delivered signal. In workloads that rely on O_ASYNC plus a real-time/alternate signal, the new API returns success but behaves incorrectly versus Linux semantics.

Useful? React with 👍 / 👎.

Comment on lines +295 to +296
if arg > Signal::SIGRTMAX as usize {
return Err(SystemError::EINVAL);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Validate F_SETSIG after narrowing arg to int

The new check compares the full usize syscall argument against SIGRTMAX, but Linux F_SETSIG semantics treat the third argument as an int (high bits are truncated for raw syscall callers). As written, values whose low 32 bits are valid signals (e.g. 0x1_0000_0001) are rejected with EINVAL here, creating Linux 6.6 ABI incompatibility for non-libc/compat callers.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug fix A bug is fixed in this pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants