Re: iked(8): fix error handling in msg_send

[Mike Belopuhov]

Tobias Heider writes:

> Hi,
>
> in the error case ikev2_msg_send the accesses the sa before checking for
> NULL. The diff adds explicit checks in those cases.
> If sendtofrom fails for any other reason than EADDRNOTAVAIL and sa is not NULL
> we should continue instead of returning (-1) so that the error is handled with
> retransmission.
>
> ok?
>

Hi Tobias,

you can write a simpler diff w/o repeating log_warn:

diff --git a/sbin/iked/ikev2_msg.c b/sbin/iked/ikev2_msg.c
index 2baea5f5508..396fea88c16 100644
--- a/sbin/iked/ikev2_msg.c
+++ b/sbin/iked/ikev2_msg.c
@@ -338,7 +338,8 @@ ikev2_msg_send(struct iked *env, struct iked_message *msg)
        if (sendtofrom(msg->msg_fd, ibuf_data(buf), ibuf_size(buf), 0,
            (struct sockaddr *)&msg->msg_peer, msg->msg_peerlen,
            (struct sockaddr *)&msg->msg_local, msg->msg_locallen) == -1) {
-               if (errno == EADDRNOTAVAIL) {
+               log_warn("%s: sendtofrom", __func__);
+               if (msg->msg_sa != NULL && errno == EADDRNOTAVAIL) {
                        sa_state(env, msg->msg_sa, IKEV2_STATE_CLOSING);
                        timer_del(env, &msg->msg_sa->sa_timer);
                        timer_set(env, &msg->msg_sa->sa_timer,
@@ -346,8 +347,8 @@ ikev2_msg_send(struct iked *env, struct iked_message *msg)
                        timer_add(env, &msg->msg_sa->sa_timer,
                            IKED_IKE_SA_DELETE_TIMEOUT);
                }
-               log_warn("%s: sendtofrom", __func__);
-               return (-1);
+               if (msg->msg_sa != NULL)
+                       return (-1);
        }
 
        if (!sa)


Regards,
Mike


> Index: ikev2_msg.c
> ===================================================================
> RCS file: /mount/openbsd/cvs/src/sbin/iked/ikev2_msg.c,v
> retrieving revision 1.58
> diff -u -p -r1.58 ikev2_msg.c
> --- ikev2_msg.c       13 Nov 2019 12:24:40 -0000      1.58
> +++ ikev2_msg.c       14 Nov 2019 15:37:11 -0000
> @@ -339,15 +339,20 @@ ikev2_msg_send(struct iked *env, struct 
>           (struct sockaddr *)&msg->msg_peer, msg->msg_peerlen,
>           (struct sockaddr *)&msg->msg_local, msg->msg_locallen) == -1) {
>               if (errno == EADDRNOTAVAIL) {
> -                     sa_state(env, msg->msg_sa, IKEV2_STATE_CLOSING);
> -                     timer_del(env, &msg->msg_sa->sa_timer);
> -                     timer_set(env, &msg->msg_sa->sa_timer,
> -                         ikev2_ike_sa_timeout, msg->msg_sa);
> -                     timer_add(env, &msg->msg_sa->sa_timer,
> -                         IKED_IKE_SA_DELETE_TIMEOUT);
> +                     if (sa != NULL) {
> +                             sa_state(env, sa, IKEV2_STATE_CLOSING);
> +                             timer_del(env, &sa->sa_timer);
> +                             timer_set(env, &sa->sa_timer,
> +                                 ikev2_ike_sa_timeout, sa);
> +                             timer_add(env, &sa->sa_timer,
> +                                 IKED_IKE_SA_DELETE_TIMEOUT);
> +                     }
> +                     log_warn("%s: sendtofrom", __func__);
> +                     return (-1);
>               }
>               log_warn("%s: sendtofrom", __func__);
> -             return (-1);
> +             if (!sa)
> +                     return (-1);
>       }
>  
>       if (!sa)