perlipc.pod: fix the "exec from signal handler" example

[stepnem]

I was scratching my head wondering why my signal handlers calling exec[1] didn't work, and neither did the perlipc example.

Fortunately a search turned up https://www.perlmonks.org/?node_id=440900 which still cites the old (and apparently still correct) documentation.

I don't know if something changed since 2011 or if the pertinent changes in commit de7ba5179657 were wrong to begin with.

For convenience I've also put the test script demonstrating the issue (included in the commit message, with further details) into a gist:

https://gist.github.com/stepnem/738d7d2e21bc4ab3d1d0a715c4a8bf86

[1] Note that the exec seems important; a simpler handler (e.g., just printing or setting a variable) appears to work fine without POSIX.

[Leont]

I don't know if something changed since 2011 or if the pertinent changes in commit https://github.com/Perl/perl5/commit/de7ba5179657b89436ba73457f8f93957b43a3f0 were wrong to begin with.

Yeah I may have removed too much in that example.

[stepnem]

Yeah I may have removed too much in that example.

Thank you for having a look.

Having reread the man page (and relevant parts of POSIX and Linux docs), I realize the paragraph I restored really talks about an unrelated issue, which (as mentioned in de7ba5179657 commit message) is unlikely to occur nowadays, so let's keep that removed.

The POSIX::sigaction (and SA_NODEFER) is needed here only because the signal handler calls exec: by default (with a plain %SIG handler) the handled signal is blocked (added to process signal mask) while the handler function runs, but here it never returns (calling exec) and the signal mask is preserved across execve, preventing further instances of the signal from being delivered to the new process.

Perhaps let's also improve the code comment a bit to explain the SA_NODEFER usage.

[Leont]

Perhaps let's also improve the code comment a bit to explain the SA_NODEFER usage.

Maybe, or maybe it should instead use sigprocmask in the sighandler to reset the mask right before the exec. That sounds easier to me.

[stepnem]

Maybe, or maybe it should instead use sigprocmask in the sighandler to reset the mask right before the exec

Yeah, although if the concern is avoiding the window for another SIGHUP in the handler function before exec, wouldn't it be better to unblock SIGHUP unconditionally in the main program (outside the handler)?

(Possibly for other reasons as well: after all, the signal might also be added to procmask by something else before executing the program; also, the perl POSIX module has the following caveat for sigprocmask: "Note that you can't reliably block or unblock a signal from its own signal handler if you're using safe signals.")

[jkeenan]

@stepnem, we have certain metadata checks that are preventing your pull request from being tested by our continuous integration rig. Could you please do the following:

1. Run perl Porting/updateAUTHORS.pl. This will add your name and email address to the correct location in our AUTHORS file.
2. Apply the following patch to pod/perlipc.pod. This will have your p.r. conform to our line-length standards.

diff --git a/pod/perlipc.pod b/pod/perlipc.pod
index b14e8344b7..5169aed767 100644
--- a/pod/perlipc.pod
+++ b/pod/perlipc.pod
@@ -211,7 +211,8 @@ info to show that it works; it should be replaced with the real code.
 
   # Make sure SIGHUP isn't blocked (as it normally would be, due to the
   # handler function calling exec rather than returning)
-  POSIX::sigprocmask(&POSIX::SIG_UNBLOCK, POSIX::SigSet->new(&POSIX::SIGHUP));
+  POSIX::sigprocmask(&POSIX::SIG_UNBLOCK,
+    POSIX::SigSet->new(&POSIX::SIGHUP));
 
   code();

Please run make test_porting to make sure all metadata tests pass, then re-push the p.r. Thanks.

[Leont]

Yeah, although if the concern is avoiding the window for another SIGHUP in the handler function before exec, wouldn't it be better to unblock SIGHUP unconditionally in the main program (outside the handler)?

I don't think that is a concern.

[stepnem]

@jkeenan thank you for the heads up; perhaps an update to perlhack.pod could be helpful? The current text reads as if the potential AUTHORS update were not supposed to be done by the patch submitter themselves (nor is updateAUTHORS.pl mentioned anywhere). I've also fixed up the line length and make test_porting passed.

@Leont seemed unhappy about the alternative proposed in the last patch, (although he didn't really explain why, and for some reason I only see that comment in my mail, not here on GitHub), so I moved it to the end after the AUTHORS addition so that it can be just dropped without further rebasing, if the version introduced by the preceding patches is OK.

[Leont]

@Leont seemed unhappy about the alternative proposed in the last patch, (although he didn't really explain why, and for some reason I only see that comment in my mail, not here on GitHub)

I think never blocking is conceptually cleaner than having to unblock explicitly. That said, in Perl the former is a bit more complicated than the latter (even when it was failing to mark the handler as delayed/safe).

Both versions are correct so it doesn't really matter much, I'm fine with merging it this way.

[stepnem]

I think never blocking is conceptually cleaner than having to unblock explicitly.

Yeah, probably. (FWIW, IIRC none of the few Unix daemons I checked back when preparing the patches seemed to care about explicitly (during startup) unblocking signals they were supposed to act on, either.)

That said, in Perl the former is a bit more complicated than the latter (even when it was failing to mark the handler as delayed/safe).

Thanks, this made me reread relevant parts of the POSIX module and perlipc man pages and realize that handlers created with POSIX::SigAction have the "safe" flag off, making them behave differently from the default Perl>=5.8 "deferred" signal handling.

So if I understand your "failing to mark" comment correctly, you mean that the version with SA_NODEFER is missing something like

Use the "deferred" signal handling scheme (default for

regular signal handlers since Perl 5.8.0, but

POSIX::SigAction objects are created with "safe" off).

$action->safe(1);

before the 'POSIX::sigaction(&POSIX::SIGHUP, $action);' line?

I can add that and drop the last patch, if you think that's better.

On that note, isn't the "try using the POSIX sigaction() function, which bypasses Perl safe signals" advice from perlipc not quite correct? If that were the case, there would be no point in doing $action->safe(1). It's really the SigAction class (producing objects with "safe" unset), not the sigaction function, that causes the bypass, right?

[Leont]

Thanks, this made me reread relevant parts of the POSIX module and perlipc man pages and realize that handlers created with POSIX::SigAction have the "safe" flag off, making them behave differently from the default Perl>=5.8 "deferred" signal handling.

So if I understand your "failing to mark" comment correctly, you mean that the version with SA_NODEFER is missing something like

# Use the "deferred" signal handling scheme (default for
# regular signal handlers since Perl 5.8.0, but
# POSIX::SigAction objects are created with "safe" off).
$action->safe(1);

before the 'POSIX::sigaction(&POSIX::SIGHUP, $action);' line?

Correct.

I can add that and drop the last patch, if you think that's better.

That would be great.

On that note, isn't the "try using the POSIX sigaction() function, which bypasses Perl safe signals" advice from perlipc not quite correct? If that were the case, there would be no point in doing $action->safe(1). It's really the SigAction class (producing objects with "safe" unset), not the sigaction function, that causes the bypass, right?

They don't really exist without each other, so the distinction is moot.

[stepnem]

So if I understand your "failing to mark" comment correctly, you mean that the version with SA_NODEFER is missing something like

# Use the "deferred" signal handling scheme (default for
# regular signal handlers since Perl 5.8.0, but
# POSIX::SigAction objects are created with "safe" off).
$action->safe(1);

before the 'POSIX::sigaction(&POSIX::SIGHUP, $action);' line?

Correct.

So I did that, and it doesn't work! Apparently safe signals are incompatible with SA_NODEFER, i.e., after modifying my test script (from the gist and the first commit message) to mark the action "safe" as above, again only one SIGHUP is caught, same as with the current broken version of the example that this PR set out to fix.

On that note, isn't the "try using the POSIX sigaction() function, which bypasses Perl safe signals" advice from perlipc not quite correct? If that were the case, there would be no point in doing $action->safe(1). It's really the SigAction class (producing objects with "safe" unset), not the sigaction function, that causes the bypass, right?

They don't really exist without each other, so the distinction is moot.

I really don't think the distinction is moot (you can use sigaction() to install both safe and unsafe handlers, so saying 'sigaction() bypasses Perl safe signals' is incorrect: whether it does or not depends on the "safe" attribute of the SigAction object passed to it), and given how subtle both the behavior and its documentation are (as witnessed by our repeated confusion here), I do think we should try not to muddle the waters further; but that's for a separate PR I think.

As for this one, unless you have other suggestions, it seems we're back to "Both versions are correct so it doesn't really matter much, I'm fine with merging it this way."? In which case I'd leave it to your discretion whether to just drop the last patch or not.

Thanks.

[Leont]

Apparently safe signals are incompatible with SA_NODEFER

Just great! /s

I really don't think the distinction is moot (you can use sigaction() to install both safe and unsafe handlers, so saying 'sigaction() bypasses Perl safe signals' is incorrect: whether it does or not depends on the "safe" attribute of the SigAction object passed to it), and given how subtle both the behavior and its documentation are (as witnessed by our repeated confusion here), I do think we should try not to muddle the waters further; but that's for a separate PR I think.

Yeah, I think you're right.

As for this one, unless you have other suggestions, it seems we're back to "Both versions are correct so it doesn't really matter much, I'm fine with merging it this way."? In which case I'd leave it to your discretion whether to just drop the last patch or not.

It seems we should go for the second version.