RFR: 8283337: Posix signal handler modification warning triggering incorrectly

[Kevin Walls]

On Thu, 17 Mar 2022 19:23:10 GMT, Kevin Walls <kevinw at openjdk.org> wrote:

> PosixSignals::print_signal_handler() is incorrectly detecting a changed signal handler.  The signal handler for SIGBREAK/SIGQUIT is now set in src/hotspot/os/posix/signals_posix.cpp with the bool parameter do_check set to false.
> 
> set_signal_handler should only store a handler in vm_handlers when do_check is true.
> 
> However I don't see a simple way of getting a valid warning for the SIGQUIT handler, if it is added later when os::initialize_jdk_signal_support() calls os::signal().
> 
> If only signals added directly by src/hotspot/os/posix/signals_posix.cpp have the warning for a handler changing, then we never had a warning for SIGQUIT, so just this simple change is needed to remove the bogus warning.

Thanks both --

Yes Thomas your memory is fine I think the summary is good. 8-)
The VMs that could be accidentally killed by attaching were those that were discovered by scanning for PIDs, and that were slow to startup.  I think we'll all agree it was good to make things more robust.

Not sure about removing do_check_signal_periodically ?  It still checks the others handlers, even if vm_handlers.get(SIGBREAK) returns null.

Ideally yes we would save the second SIGQUIT handler - I think the problem there is that it is set later by os::signal() which doesn't have a way to distinguish itself from any other native code setting a handler.  It is the kind of signal action setting code which this saved handler mechanism is trying to notice (and it does notice it!)    

I think that os::signal() call to set the BREAK/QUIT handler was never being saved in vm_handlers, so we haven't actually been checking for changed handlers there.


David's comment about reversing these lines 
(inside check_signal_handler(int sig) at the point where we have detected a change):

 838     os::print_signal_handlers(tty, buf, O_BUFLEN);
 839     do_check_signal_periodically[sig] = false;

..and checking do_check_signal_periodically[sig] in print_signal_handlers:


Yes, I almost added checking do_check_signal_periodically[sig] in print_signal_handlers.  

There is some duplication of warnings, but they are different: if periodic check finds a mismatch, it warns (shows which handler has changed):

  Warning: SIGQUIT handler modified!

...then calls print_handlers which warns more specifically (what the handler is, and what we expected):

  *** Handler was modified!
  *** Expected: javaSignalHandler in libjvm.so, mask=11100100110111111111111111111110, flags=SA_RESTART|SA_SIGINFO


Actually, the periodic check can get really verbose: it prints all handlers, for each individual handler it sees changed.

os::run_periodic_checks()
calls
 889   check_signal_handler(SIGSEGV);
 890   check_signal_handler(SIGILL);
..etc...

...each of which might make this call:
 838     os::print_signal_handlers(tty, buf, O_BUFLEN);

...so thankfully we don't see this very often.  I get it now that line 838 should probably be printing ONE handler:  
 os::print_signal_handler(tty, sig, buf, O_BUFLEN);


This is a removal of a regression so don't want to feature-creep too much, but I'll try that change.

-------------

PR: https://git.openjdk.java.net/jdk/pull/7858