RFR(XXS): 8007312: null check signal semaphore in os::signal_notify windows

[David Holmes]

Thanks for looking that up :) I think we've gone way beyond due 
diligence on this one.

FYI in my testing the other thing I saw was that by this stage both the 
ReferenceHandler thread and the Finalizer thread, are created and 
running. So creating the Signal handler thread earlier - if in the 
future it should seem necessary - seems reasonably safe.

Thanks,
David

On 3/02/2013 1:25 AM, Markus Grönlund wrote:
> And it doesn't look like sema_post() do check for it (it only checks to see if the semaphore value pointed to by sp exceeds SEM_VALUE_MAX).
>
> http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/lib/libc/port/threads/sema.c#297
>
> sema_post() could probably be extended to also check for sp->magic == SEMA_MAGIC to ensure that the call is on a valid (initialized), semaphore.
>
> Cheers
> Markus
>
>
> -----Original Message-----
> From: Markus Grönlund
> Sent: den 2 februari 2013 16:03
> To: David Holmes
> Cc: hotspot-dev at openjdk.java.net
> Subject: RE: RFR(XXS): 8007312: null check signal semaphore in os::signal_notify windows
>
> Thanks for checking. This matches the initial quick tests I did on Solaris as well.
>
> "Curiously nothing. I put an infinite_sleep at the start of os::signal_init. On both linux and solaris ctrl-c did nothing while ctrl-/ (SIGQUIT) induced a core dump - as expected. No assertions triggered. It did show UserHandler was installed though.
>
> Maybe the ::sema_post(&sig_sem); is not issuing a non-zero return value when called on an non-sema_init:ed sema_t? (this is where I would expect something to happen, linux or bsd does not have any asserts on this).
>
> Thanks for verifying UserHandler() installation here.
>
> Cheers
> Markus
>
> -----Original Message-----
> From: David Holmes
> Sent: den 2 februari 2013 14:00
> To: Markus Grönlund
> Cc: hotspot-dev at openjdk.java.net
> Subject: Re: RFR(XXS): 8007312: null check signal semaphore in os::signal_notify windows
>
> On 2/02/2013 8:44 AM, David Holmes wrote:
>> On 2/02/2013 12:49 AM, Markus Grönlund wrote:
>>> Thanks David,
>>>
>>> Ok, so based on our discussions, we'll narrow this to only do a null
>>> check on the signal semaphore handle on Windows.
>>>
>>> I have updated the webrev, here:
>>> http://cr.openjdk.java.net/~mgronlun/8007312/webrev02/
>>
>> Looks okay to me.
>>
>> I'll try to test out what happens on Solaris/Linux if you manage to
>> sneak in a very early ctrl-C.
>
> Curiously nothing. I put an infinite_sleep at the start of os::signal_init. On both linux and solaris ctrl-c did nothing while ctrl-/ (SIGQUIT) induced a core dump - as expected. No assertions triggered. It did show UserHandler was installed though.
>
> Can't test windows.
>
> David
> -----
>
>> Thanks,
>> David
>>
>>>
>>> Also I changed the bug description to reflect the updated information
>>> (to the updated subject in the mail above):
>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8007312
>>>
>>>
>>> With this change we don't trap on the assert when pressing ctrl-c if
>>> the hotspot signal handler is not setup.
>>>
>>> Thanks a lot for your help David, have a great weekend.
>>>
>>> Cheers
>>> Markus
>>>
>>>
>>>
>>> -----Original Message-----
>>> From: David Holmes
>>> Sent: den 1 februari 2013 14:41
>>> To: Markus Grönlund
>>> Cc: hotspot-dev at openjdk.java.net
>>> Subject: Re: RFR(XXS): 8007312: Signal Dispatcher thread to start and
>>> register ctrl-break handler before TRACE_INITIALIZE
>>>
>>> Which brings us back to simply checking for not being initialized.
>>>
>>> David
>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> (windows analysis)
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Pressing ctrl-c before Hotspot signal/console handler has been
>>>>>>> registered actually asserts/stops the VM (which to the user
>>>>>>> appears like a crash) on non-product builds.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Before Hotspot registers its own jvm!consoleHandler with
>>>>>>> kernel32!CtrlRoutine, the C runtime default
>>>>>>> msvcr100!ctrlevent_capture is implicitly used - this calls back
>>>>>>> into jvm!UserHandler, which forwards into os::signal_notify()
>>>>>>> which uses uninitialized variables.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> // on Windows this creates the following issue when closing a
>>>>>>> NULL handle to a semaphore
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> void os::signal_notify(int signal_number) {
>>>>>>>
>>>>>>> BOOL ret;
>>>>>>>
>>>>>>>
>>>>>>> Atomic::inc(&pending_signals[signal_number]);
>>>>>>>
>>>>>>> ret = ::ReleaseSemaphore(sig_sem, 1, NULL);<<--- call
>>>>>>> ReleaseSemaphore on global handle sig_sem which has not been
>>>>>>> setup/created yet == is NULL (is created in os::signal_init_pd())
>>>>>>>
>>>>>>> assert(ret != 0, "ReleaseSemaphore() failed");<<-- assert traps
>>>>>>> here (GetLastError() == 0xc0000008 - An invalid HANDLE was
>>>>>>> specified)
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> }
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>> Markus