signal/sig_dispatch: Fix case where signal action is sent twice

[pussuw]

Summary

As far as I can interpret how signal delivery should work when the signal is blocked, it should still be sent to the pending queue even if the signal is masked. When the sigmask changes it will be delivered.

The original implementation did not add the pending signal action, if stcb->task_state == TSTATE_WAIT_SIG is true.

An attempt to patch this was made in #8563 but it is insufficient as it creates an issue when the task is not waiting for a signal, but is in syscall, in this case the signal is incorrectly queued twice.

Impact

Attempt #2 to fix signal action queuing

Testing

icicle:knsh

[pussuw]

Build error comes from libcxx, do I need to rebase or is it still an issue?

[xiaoxiang781216]

it is fixed by the last master, please rebase your change.

[xiaoxiang781216]

Let's ignore the ci broken which is fixed by #8611 and merge this simple change.

[GUIDINGLI]

@pussuw

I found a error in this PR.

With this PR, this LTP test case can't pass in configuration sim:nsh
https://github.com/nloopa/open_posix_testsuite/blob/master/conformance/interfaces/timer_settime/9-2.c

[GUIDINGLI]

When FLAT build.
Shouldn't call nxsig_add_pendingsignal() when stcb->task_state == TSTATE_WAIT_SIG.

[xiaoxiang781216]

@pussuw do you have more comment? If not, I will create a new patch to revert your change in the next Monday.

[pussuw]

I'm sorry to hear that you encountered a regression.

As far as I can understand how posix signals are supposed to work, this should have fixed the situation where the signal action is never performed if the signal is masked, or if system calls are in use.

My understanding of the posix signaling spec is that if a signal action is masked, it should be put into the pending queue no matter what. I am not sure about this though. Maybe someone who knows more about this subject can comment?

@xiaoxiang781216 the revert should take into account what this patch was supposed to fix; the signal action should not be pended twice.

[xiaoxiang781216]

@xiaoxiang781216 the revert should take into account what this patch was supposed to fix; the signal action should not be pended twice.

Actually, your change make the signal trigger twice:(.
@GUIDINGLI already point out the problem. If the signal wakes up the caller of sigwait, it shouldn't be added to the pending list again. Otherwise, user space will receive two events. You can try the test which is very simple:
https://github.com/linux-test-project/ltp/blob/master/testcases/open_posix_testsuite/conformance/interfaces/timer_settime/9-2.c
In most case, breaking PTS means our implementation doesn't conform the spec.

[pussuw]

Ok I understand, the signal is received twice. But the first branch does not queue a signal action which was the problem I tried to fix. Easy way to reproduce the issue is to start a task that just loops and calls sleep(). It will then be waiting for the timer alarm event, and the first TSTATE_WAIT_SIG branch is executed.

The problem I had is that using kill to terminate the task does not work, as the signal action for kill is not performed. I need to figure out a better way to fix this.

[pussuw]

The solution seems to be quite simple.

Like I said the signal delivery was not broken, but scheduling the signal action did not work, when executing a system call. The solution seems to be to just simply add the signal action to the signal action queue, not to the pending signal queue.

I will verify this by testing a bit more and if it works I'll provide a patch in a moment.