child-sa: Update status flags based on success of SA addition

Both variables `inbound_installed` and `outbound_state` are used in
`child_sa_t::destroy()` to determine whether inbound and outbound state
have to be deleted. They are assigned prior to the call to
`kernel_interface_t::add_sa()`. As this call may fail, the destructor may
try to delete a state which it has not been added.
By making the assignment of these variables dependent on the success of
the state addition, we can make sure, a `child_sa_t::destroy()` only
deletes states it has added.

Also removed the redundant checks for `my_spi` and `other_spi` being set
along with the check for the above flags. It seems that when the flags
are set, the SPIs *must* be set.

Signed-off-by: Thomas Egerer <thomas.egerer@secunet.com>

diff --git a/src/libcharon/sa/child_sa.c b/src/libcharon/sa/child_sa.c
index 1f50c49520717310de4b7c7c606f150b1277e34a..7262342075c507c9cc4a0bcc8de33de780e5dd5f 100644 (file)
--- a/src/libcharon/sa/child_sa.c
+++ b/src/libcharon/sa/child_sa.c
@@ -620,7 +620,7 @@ static status_t update_usebytes(private_child_sa_t *this, bool inbound)
 
        if (inbound)
        {
-               if (this->my_spi && this->inbound_installed)
+               if (this->inbound_installed)
                {
                        kernel_ipsec_sa_id_t id = {
                                .src = this->other_addr,
@@ -654,7 +654,7 @@ static status_t update_usebytes(private_child_sa_t *this, bool inbound)
        }
        else
        {
-               if (this->other_spi && (this->outbound_state & CHILD_OUTBOUND_SA))
+               if (this->outbound_state & CHILD_OUTBOUND_SA)
                {
                        kernel_ipsec_sa_id_t id = {
                                .src = this->my_addr,
@@ -939,7 +939,6 @@ static status_t install_internal(private_child_sa_t *this, chunk_t encr,
                this->my_cpi = cpi;
                dst_ts = my_ts;
                src_ts = other_ts;
-               this->inbound_installed = TRUE;
        }
        else
        {
@@ -954,7 +953,6 @@ static status_t install_internal(private_child_sa_t *this, chunk_t encr,
                {
                        tfc = this->config->get_tfc(this->config);
                }
-               this->outbound_state |= CHILD_OUTBOUND_SA;
        }
 
        DBG2(DBG_CHD, "adding %s %N SA", inbound ? "inbound" : "outbound",
@@ -1061,6 +1059,17 @@ static status_t install_internal(private_child_sa_t *this, chunk_t encr,
        other_ts->destroy(other_ts);
        free(lifetime);
 
+       if (status == SUCCESS)
+       {
+               if (inbound)
+               {
+                       this->inbound_installed = TRUE;
+               }
+               else
+               {
+                       this->outbound_state |= CHILD_OUTBOUND_SA;
+               }
+       }
        return status;
 }
 
@@ -1623,7 +1632,7 @@ static status_t update_sas(private_child_sa_t *this, host_t *me, host_t *other,
                                                   bool encap, uint32_t reqid)
 {
        /* update our (initiator) SA */
-       if (this->my_spi && this->inbound_installed)
+       if (this->inbound_installed)
        {
                kernel_ipsec_sa_id_t id = {
                        .src = this->other_addr,
@@ -1649,7 +1658,7 @@ static status_t update_sas(private_child_sa_t *this, host_t *me, host_t *other,
        }
 
        /* update his (responder) SA */
-       if (this->other_spi && (this->outbound_state & CHILD_OUTBOUND_SA))
+       if (this->outbound_state & CHILD_OUTBOUND_SA)
        {
                kernel_ipsec_sa_id_t id = {
                        .src = this->my_addr,
@@ -1960,7 +1969,7 @@ METHOD(child_sa_t, destroy, void,
                };
                charon->kernel->del_sa(charon->kernel, &id, &sa);
        }
-       if (this->other_spi && (this->outbound_state & CHILD_OUTBOUND_SA))
+       if (this->outbound_state & CHILD_OUTBOUND_SA)
        {
                kernel_ipsec_sa_id_t id = {
                        .src = this->my_addr,