RFR: 8251438: Issues with our POSIX set_signal_handler()

[David Holmes]

Hi Thomas,

On 11/12/2020 5:22 pm, Thomas Stuefe wrote:
> On Mon, 7 Dec 2020 21:45:57 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:
> 
>> This is a fix for a potential issue involving "The storage occupied by sa_handler and sa_sigaction may overlap, and a conforming application shall not use both simultaneously." https://pubs.opengroup.org/onlinepubs/009695399/functions/sigaction.html, when we in fact do assume different storages.
> 
> Hi Gerard,
> 
> I was confused about the patch. Its fine from a simplification standpoint - putting all those ? expressions into one place - but where were we relying on different storage?

We were reading both fields from the same structure without checking the 
flag value:

  void* oldhand = oldAct.sa_sigaction
                 ? CAST_FROM_FN_PTR(void*,  oldAct.sa_sigaction)
                 : CAST_FROM_FN_PTR(void*,  oldAct.sa_handler);

Cheers,
David

> Cheers, Thomas
> 
> src/hotspot/os/posix/signals_posix.cpp line 743:
> 
>> 741: // Implementation may use the same storage for both the sa_sigaction field and the sa_handler field,
>> 742: // so check for "sigAct.sa_flags == SA_SIGINFO"
>> 743: static address get_signal_handler(struct sigaction action) {
> 
> This copies the structure by value. While the compiler will probably optimize this out, I'd still not do it. Please use
> static address get_signal_handler(const struct sigaction*)
>   
> instead.
> 
> -------------
> 
> Changes requested by stuefe (Reviewer).
> 
> PR: https://git.openjdk.java.net/jdk/pull/1680
>