RFR(M): 8141529: Fix handling of _JAVA_SR_SIGNUM
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Nov 13 14:45:43 UTC 2015
On 11/13/15 6:38 AM, David Holmes wrote:
> On 13/11/2015 7:53 PM, Thomas Stüfe wrote:
>> Hi Goetz,
>>
>> sorry for not looking at this earlier. This is a nice cleanup. Some
>> remarks:
>>
>> http://cr.openjdk.java.net/~goetz/webrevs/8141529-NSIG/webrev.01/src/os/aix/vm/os_aix.cpp.udiff.html
>>
>>
>> + if (sig > MAX2(SIGSEGV, SIGBUS) && // See 4355769.
>> + sig < NSIG) { // Must be legal signal and fit
>> into sigflags[].
>>
>> I do not like much the MAX2() construct. I would like it better to
>> explicitly check whether the SR signal is one of the "forbidden" ones
>> the VM uses.
>
> I must confess I had not looked into 4355769 but this check seems
> rather spurious. It is not at all clear to me what signals could be
> used here - other than SIGUSR1 or SIGUSR2 (if -Xrs is specified), or
> else a real-time signal (modulo discussion below). Hijacking anything
> else seems rather suspect.
>
>> Maybe keep a mask defined centrally for each platform which contains
>> signals the VM needs for itself ?
>
> Such masks already exist.
>
>> +sigset_t os::Aix::sigs = { 0 };
>>
>> I would not initialize the signal set this way. sigset_t is an opaque
>> type; the only way to initialize it is with one of sigemptyset() or
>> sigfillset().
>
> Good catch - I overlooked that.
>
>> http://cr.openjdk.java.net/~goetz/webrevs/8141529-NSIG/webrev.01/src/os/aix/vm/os_aix.hpp.udiff.html
>>
>>
>> + static struct sigaction sigact[NSIG]; // saved preinstalled
>> sigactions
>> + static sigset_t sigs; // mask of signals that have
>>
>> + static int sigflags[NSIG];
>>
>> I know this is not in the scope of your change, but I would like to see
>> those removed from os::Aix and put into os_aix.cpp at static filescope.
>> There is no need at all to export those, and you would get rid of the
>> signal.h dependency you know have when including os_aix.hpp.
>>
>>
>> http://cr.openjdk.java.net/~goetz/webrevs/8141529-NSIG/webrev.01/src/os/bsd/vm/jsig.c.udiff.html
>>
>> http://cr.openjdk.java.net/~goetz/webrevs/8141529-NSIG/webrev.01/src/os/bsd/vm/os_bsd.cpp.udiff.html
>>
>>
>> On BSD, we have realtime signals.
>>
>> http://fxr.watson.org/fxr/source/sys/signal.h
>> #define SIGRTMAX 126
>> and NSIG does not contain them:
>> #define NSIG 32
>>
>> The max. possible signal number would be 126, which unfortunately does
>> not even fit into a 64bit mask.
>
> So this simply limits the signal choice to not be a real-time signal -
> same as today.
>
>> So the code in jsig.c is broken for the case that someone wants to
>> register realtime signals, if the VM were to ever use realtime signals
>> itself, which now is not the case.
>>
>> The same is true for os_bsd.cpp, where signal chaining will not work if
>> the application did have handler for real time signals pre-installed
>> before jvm is loaded.
>
> Chaining is only used when the JVM will catch signals. Aren't all the
> real-time signals going to be blocked by the VM by default and so
> chaining is not needed as no handler will exist in the VM ?? (Unless a
> real-time signal is supplied for SR_signum)
>
> I must admit I don't know if any of this code actually works for
> real-time signals.
>
>> Solaris:
>>
>> The only platform where NSIG is missing?
>>
>> Here, we calculate the max. signal number dynamically in os_solaris.cpp,
>> presumably because SIGRTMAX is not a constant and can be changed using
>> system configuration. But then, on Linux we have the same situation
>> (SIGRTMAX is dynamic) and there we do not go through the trouble of
>> calculating the max. signal number dynamically. Instead we just use
>> NSIG=64 and rely on the fact that NSIG is larger than the largest
>> possible dynamic value for SIGRTMAX.
>
> Linux ensures that _NSIG (and thus NSIG) includes all the real-time
> signals. But libc can expose a subset and steal some for its own use.
>
>> Solaris does not seem to have NSIG defined, but I am sure there is also
>> a max. possible value for SIGRTMAX (the default seems to be 48). So, one
>> could probably safely define NSIG for Solaris too, so that we have NSIG
>> defined on all Posix platforms.
>
> Solaris doesn't have any of this SR_signum related code. A more
> general cleanup of signal related code would potentially involve a lot
> of cleanup.
I see this on my s10x_u11wos_24a X86 machine:
$ rgrep NSIG /usr/include | grep -w NSIG
/usr/include/sys/signal.h:#define NSIG 49 /* valid signals
range from 1 to NSIG-1 */
/usr/include/sys/signal.h:#define MAXSIG 48 /* size of
u_signal[], NSIG-1 <= MAXSIG */
But Solaris doesn't use NSIG because it does have the SR_signum
code (as David said)...
Dan
>
> David
> -----
>
>>
>> On Thu, Nov 12, 2015 at 8:24 PM, Lindenmaier, Goetz
>> <goetz.lindenmaier at sap.com <mailto:goetz.lindenmaier at sap.com>> wrote:
>>
>> Hi David, Dmitry,
>>
>> I've come up with a new webrev:
>> http://cr.openjdk.java.net/~goetz/webrevs/8141529-NSIG/webrev.01/
>>
>> I hit on some more issues:
>> - As proposed, I replaced MAXSIGNUM by NSIG
>> - On AIX, NSIG=255. Therefore storing bits in a word does not
>> work.
>> I'm now using bitset functionality from signal.h as it's done
>> in other places.
>> sigset_t is >> NSIG on linux, so it's no good idea to use it
>> there.
>>
>>
>> Why do we not do this on all platforms, provided sigset_t contains all
>> signals (incl. realtime signals) ?
>>
>> - In the os files I found another bit vector that now is too
>> small: sigs.
>> I adapted that, too. Removed the dead declaration of this on
>> solaris.
>>
>> Best regards,
>> Goetz.
>>
>>
>>
>> Kind Regards, Thomas
>>
>> > -----Original Message-----
>> > From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com
>> <mailto:dmitry.samersoff at oracle.com>]
>> > Sent: Donnerstag, 12. November 2015 10:05
>> > To: Lindenmaier, Goetz; David Holmes; hotspot-runtime-
>> > dev at openjdk.java.net <mailto:dev at openjdk.java.net>;
>> serviceability-dev
>> > Subject: Re: RFR(M): 8141529: Fix handling of _JAVA_SR_SIGNUM
>> >
>> > Goetz,
>> >
>> > *BSD including OS X also defines NSIG (just checked) and if my
>> memory is
>> > not bogus, AIX defines it too.
>> >
>> > So you may consider to use NSIG on all platform.
>> >
>> > -Dmitry
>> >
>> > On 2015-11-12 11:36, Lindenmaier, Goetz wrote:
>> > > OK I'll change it to NSIG. That's used in other places in
>> os_linux, too.
>> > > So it's really more consistent.
>> > >
>> > > Best regards,
>> > > Goetz
>> > >
>> > >> -----Original Message-----
>> > >> From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com
>> <mailto:dmitry.samersoff at oracle.com>]
>> > >> Sent: Donnerstag, 12. November 2015 09:22
>> > >> To: David Holmes; Lindenmaier, Goetz; hotspot-runtime-
>> > >> dev at openjdk.java.net <mailto:dev at openjdk.java.net>;
>> serviceability-dev
>> > >> Subject: Re: RFR(M): 8141529: Fix handling of _JAVA_SR_SIGNUM
>> > >>
>> > >> David,
>> > >>
>> > >> I think it's better to use NSIG (without underscore) defined
>> in signal.h
>> > >>
>> > >> -Dmitry
>> > >>
>> > >>
>> > >> On 2015-11-12 10:35, David Holmes wrote:
>> > >>> Hi Goetz,
>> > >>>
>> > >>> Adding in serviceability-dev
>> > >>>
>> > >>> On 9/11/2015 6:22 PM, Lindenmaier, Goetz wrote:
>> > >>>> Hi,
>> > >>>>
>> > >>>> The environment variable _JAVA_SR_SIGNUM can be set to a
>> signal
>> > >> number
>> > >>>> do be used by the JVM's suspend/resume mechanism.
>> > >>>>
>> > >>>> If set, a signal handler is installed and the current signal
>> handler
>> > >>>> is saved to an array.
>> > >>>> On linux, this array had size MAXSIGNUM=32, and
>> _JAVA_SR_SIGNUM
>> > >> was
>> > >>>> allowed
>> > >>>> to range up to _NSIG=65. This could cause memory corruption.
>> > >>>>
>> > >>>> Further, in jsig.c, an unsinged int is used to set a bit for
>> signals.
>> > >>>> This also
>> > >>>> is too small, as only 32 signals can be supported.
>> Further, the
>> > >>>> signals are mapped
>> > >>>> wrong to these bits. '0' is not a valid signal, but '32'
>> was. 1<<32
>> > >>>> happens to map to
>> > >>>> zero, so the signal could be stored, but this probably
>> was not
>> > >>>> intended that way.
>> > >>>>
>> > >>>> This change increases MAXSIGNUM to 65 on linux, and to 64 on
>> aix. It
>> > >>>> introduces
>> > >>>> proper checking of the signal read from the env var, and
>> issues a
>> > >>>> warning if it
>> > >>>> does not use the signal set. It adapts the data types in
>> jisig.c
>> > >>>> properly.
>> > >>>>
>> > >>>> Please review this change. I please need a sponsor.
>> > >>>>
>> http://cr.openjdk.java.net/~goetz/webrevs/8141529-NSIG/webrev.00
>> > >>>
>> > >>> This all sounds very good to me. (I must find out why Solaris
>> is not
>> > >>> involved here :) ).
>> > >>>
>> > >>> On Linux you didn't add the bounds check to
>> os::Linux::set_our_sigflags.
>> > >>>
>> > >>> I'm also wondering about documenting where we are
>> determining the
>> > >>> maximum from? Is it simply _NSIG on some/all distributions?
>> And I see
>> > >>> _NSIG is supposed to be the biggest signal number + one. Also
>> linux
>> > >>> defines NSIG = _NSIG so which should we be using?
>> > >>>
>> > >>> Thanks,
>> > >>> David
>> > >>>
>> > >>>> Best regards,
>> > >>>> Goetz.
>> > >>>>
>> > >>
>> > >>
>> > >> --
>> > >> Dmitry Samersoff
>> > >> Oracle Java development team, Saint Petersburg, Russia
>> > >> * I would love to change the world, but they won't give me the
>> sources.
>> >
>> >
>> > --
>> > Dmitry Samersoff
>> > Oracle Java development team, Saint Petersburg, Russia
>> > * I would love to change the world, but they won't give me the
>> sources.
>>
>>
More information about the hotspot-runtime-dev
mailing list