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

[Markus Grönlund]

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