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

Signal must be masked when it is delivered to a signal handler #9298

Closed
patacongo opened this issue May 16, 2023 · 7 comments · Fixed by #9299
Closed

Signal must be masked when it is delivered to a signal handler #9298

patacongo opened this issue May 16, 2023 · 7 comments · Fixed by #9299

Comments

@patacongo
Copy link
Contributor

Signal must be masked when it is delivered to a signal handler per:

https://pubs.opengroup.org/onlinepubs/007904875/functions/sigaction.html
When a signal is caught by a signal-catching function installed by sigaction(), a new signal mask is calculated and installed for the duration of the signal-catching function (or until a call to either sigprocmask() or sigsuspend() is made). This mask is formed by taking the union of the current signal mask and the value of the sa_mask for the signal being delivered [XSI] [Option Start] unless SA_NODEFER or SA_RESETHAND is set, [Option End] and then including the signal being delivered. If and when the user's signal handler returns normally, the original signal mask is restored.

Any action queued for that signal while the signal is masked should be deferred. It should go into the group pending signal list and should not be processed until until the signal is unmasked (which should occur when the signal handler returns).

@mu578
Copy link

mu578 commented May 16, 2023

I think the issue is a signal maybe delivered while its handler is running (the nested case being the shortest path to expose it) , so yes, it needs to be withheld or block (to let that handler a chance to exit). Should nested signals be banned? (absolutism) I don't know might break legit things out there.

@patacongo
Copy link
Contributor Author

Close with #9299

@patacongo
Copy link
Contributor Author

patacongo commented May 16, 2023

Should nested signals be banned? (absolutism)

I am not sure. Certainly be default, handlers for the same signal number are forced to be sequential (by the POSIX the requirement to mask the signal on entry into the signal handler). But:

  • You can always modify the sigprocmask within the signal handler, and
  • That would not apply to other signals.

Interrupting one signal handler to process another implies some priority handling of signals. Why would you interrupt one handler to run another versus letting the current handler run to completion first. I think that the dispatch logic currently will not permit nested signal handlers (but I would need to verify that -- It doesn't. See below).

@patacongo
Copy link
Contributor Author

patacongo commented May 16, 2023

Here is where one-at-a-time signal handling is enforced for ARMv7M: https://github.com/apache/nuttx/blob/master/arch/arm/src/armv7-m/arm_schedulesigaction.c#L88

/* Refuse to handle nested signal actions */

if (tcb->xcp.sigdeliver == NULL)
  {

So I suspect that fact that you are seeing nested signal handling in this case of PR #9296 is ONLY due to the fact that xcp.sigdeliver is NULL when it should be non-NULL (because we really are in a signal handler).

This results in the error:

  • The signal handler is incorrectly entered while a signal handler is being processed.
  • As a consequence the saved xcp.sigdeliver is clobber and an infinite loop results.

This change is correct because there should be no window where xcp.sigdeliver is NULL while we are in a signal handler. But I don't know if this is the root cause of the problem.

@patacongo
Copy link
Contributor Author

patacongo commented May 16, 2023

Why would you interrupt one handler to run another versus letting the current handler run to completion first.

POSIX does require that real-time signals do have a priority associated with them and their handlers should run in priority order. So it might not be a bad thing to permit a higher priority signal handler to interrupt a lower priority signal handler. Unfortunately signal prioritization is not currently supported: #8899

@patacongo
Copy link
Contributor Author

patacongo commented May 16, 2023

I think the issue is a signal maybe delivered while its handler is running (the nested case being the shortest path to expose it) , so yes, it needs to be withheld or block (to let that handler a chance to exit).

Just to clarify: Whn you say "this issue", you are referring to the bug fixed by PR #9296. This Issue addresses a POSIX compliance problem.

PR #9296 fixes the problem because it eliminates all nesting. There was a window in which xcp.sigdeliver was NULL when it should not have been. That permitted an unsupported nested signal action to occur because of the logic in https://github.com/apache/nuttx/blob/master/arch/arm/src/armv7-m/arm_schedulesigaction.c#L88

In the current design, any nested signal will clobber xcp.sigdelver and crash the system. PR #9296 does not change that.

@mu578
Copy link

mu578 commented May 16, 2023

You write faster that I can read (I have a slow morning today). Yes about priority handling, not particularly just for signal API, it is always nice to have.

Anyway, in a general context, any item which is enqueued should be considered read-only ; it could be handled with a private bitset ; because if something is marked deferred ; it should not be possible to externally change it (user), rationals not covered by posix specs.

Let it segfault.

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 a pull request may close this issue.

2 participants