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

[Tobias Heider]

On Thu, Nov 14, 2019 at 09:57:27AM -0700, Theo de Raadt wrote:
> > 
> > The problem here is that log_warn can change errno,
> 
> No, it specifically avoids touching errno.
> 
> log_warn(const char *emsg, ...)
> {
>         char            *nfmt;
>         va_list          ap;
>         int              saved_errno = errno;
> ...
>         errno = saved_errno;
> }
> 

Good to know, thanks! In that case I really prefer Mike's diff.
Here is an update with msg->msg_sa used consistently. We can also do it
the other way around, but I would prefer to use either sa or msg_sa.

Index: ikev2_msg.c
===================================================================
RCS file: /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 17:15:42 -0000
@@ -303,7 +303,6 @@ ikev2_msg_valid_ike_sa(struct iked *env,
 int
 ikev2_msg_send(struct iked *env, struct iked_message *msg)
 {
-       struct iked_sa          *sa = msg->msg_sa;
        struct ibuf             *buf = msg->msg_data;
        uint32_t                 natt = 0x00000000;
        int                      isnatt = 0;
@@ -338,7 +337,8 @@ ikev2_msg_send(struct iked *env, struct 
        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,11 +346,11 @@ ikev2_msg_send(struct iked *env, struct 
                        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)
+       if (msg->msg_sa == NULL)
                return (0);
 
        if ((m = ikev2_msg_copy(env, msg)) == NULL) {
@@ -360,11 +360,11 @@ ikev2_msg_send(struct iked *env, struct 
        m->msg_exchange = exchange;
 
        if (flags & IKEV2_FLAG_RESPONSE) {
-               TAILQ_INSERT_TAIL(&sa->sa_responses, m, msg_entry);
+               TAILQ_INSERT_TAIL(&msg->msg_sa->sa_responses, m, msg_entry);
                timer_set(env, &m->msg_timer, ikev2_msg_response_timeout, m);
                timer_add(env, &m->msg_timer, IKED_RESPONSE_TIMEOUT);
        } else {
-               TAILQ_INSERT_TAIL(&sa->sa_requests, m, msg_entry);
+               TAILQ_INSERT_TAIL(&msg->msg_sa->sa_requests, m, msg_entry);
                timer_set(env, &m->msg_timer, ikev2_msg_retransmit_timeout, m);
                timer_add(env, &m->msg_timer, IKED_RETRANSMIT_TIMEOUT);
        }