On Thu, Nov 14, 2019 at 05:25:48PM +0100, Mike Belopuhov wrote:
>
> 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
The problem here is that log_warn can change errno, if you want to do it this
way you will have to save errno and check against the saved variable.
If you want to stick with msg_sa it would probably make sense to also remove
sa and use msg_sa everywhere. Otherwise I'm ok with yours.
Regards,
Tobias
