On Wed, Jun 8, 2016 at 5:53 PM, Selva Nair <selva.n...@gmail.com> wrote:
> On Wed, Jun 8, 2016 at 8:20 AM, Steffan Karger <stef...@karger.me> wrote:
>>
>> --- a/src/openvpn/push.c
>> +++ b/src/openvpn/push.c
>> @@ -239,11 +239,21 @@ incoming_push_message (struct context *c, const
>> struct buffer *buffer)
>> {
>> c->options.push_option_types_found |= option_types_found;
>>
>> + /* delay bringing tun/tap up until --push parms received from
>> remote */
>> if (status == PUSH_MSG_REPLY)
>> - do_up (c, true, c->options.push_option_types_found ); /* delay
>> bringing tun/tap up until --push parms received from remote */
>> + {
>> + if (!do_up (c, true, c->options.push_option_types_found))
>> + {
>> + msg (D_PUSH_ERRORS, "Failed to open tun/tap interface");
>> + c->sig->signal_received = SIGUSR1; /* connection reset */
>> + c->sig->signal_text = "open-tuntap-failed";
>
>
> Please try to avoid that potential overwrite of a pending signal --- as this
> runs on the client, c->sig here is a pointer to the global siginfo_static.
>
> Use register_signal (c, SIGUSR1, "open-tuntap-failed") instead -- it will
> at least protect SIGTERM. I'm looking into how to prioritize signals, and it
> would help to have all signals set through a few functions such as
> register_signal, throw_signal_soft etc ..
Sure, please tell me what to use. I never had to deal with our
signals before, so I simply copied this from a few lines below, where
(as far as I could see) the did what I wanted it to do.
I had already replaced this with
throw_signal_soft( SIGUSR1, "process-push-msg-failed" );
based on feedback from Gert (v2, yet to come). In my tests that also
works as I want it to (abort the connection attempt, try again). Can
you explain when I need to use register_signal(), throw_signal() or
throw_signal_soft()?
> By the way "open-tun-tap failed" may be a confusing as a real open tun/tap
> error would have resulted in a FATAL exit -- the error handled here could
> happen even if tun/tap has been successfully opened, isn't it?
Yes, noticed that too, and is actually fixed by 5/5. I didn't notice
4/5 still got the confusing text.
-Steffan
