Merge lp:~stefan.goetz-deactivatedaccount/hipl/esp-destination-addresses into lp:hipl

[Stefan Götz]

Remove memory leaks and unchecked allocations from the firewall. Refactor the list of destination addresses associated with an ESP rule to use the hip_ll instead of the slist list implementation. Update all code handling this list so that memory allocations are checked, memory leaks are prevented, an error handling is performed properly.

For reviewing, the commits on this branch are fairly tidy and break down the changes nicely.

[Diego Biurrun (diego-biurrun)]

 review approve

On Tue, Mar 29, 2011 at 12:44:48AM +0000, Stefan Götz wrote:
>
> Remove memory leaks and unchecked allocations from the firewall.
> Refactor the list of destination addresses associated with an ESP rule
> to use the hip_ll instead of the slist list implementation. Update all
> code handling this list so that memory allocations are checked, memory
> leaks are prevented, an error handling is performed properly.

Does this get rid of all instances of hslist? Yay for getting rid of
one of the duplicated list implementations.

> For reviewing, the commits on this branch are fairly tidy and break
> down the changes nicely.

Indeed they are, thanks. The merge proposal is fine with me, below
are just some minor comments - apply at your own discretion.

If you wish to make this even simpler, you could push r5796 and r5797
from your branch to trunk right away. They are trivial and I hereby
approve them.

> --- firewall/conntrack.c 2011-03-24 17:34:21 +0000
> +++ firewall/conntrack.c 2011-03-29 00:44:44 +0000
> @@ -288,20 +287,19 @@
> -static struct esp_address *get_esp_address(const struct slist *addr_list,
> - const struct in6_addr *addr)
> +static struct esp_address *get_esp_address(const struct hip_ll *const addresses,
> + const struct in6_addr *const addr)

I think you could just push such constifications to trunk directly,
that would make your branches even smaller...

> @@ -321,42 +319,49 @@
>
> - if (!addr_list) {
> - HIP_DEBUG("Esp slist is empty\n");
> - }

I'd say the same for such annoying debug output that we have such
abundant amounts of in HIPL.

> @@ -626,79 +625,104 @@
> + HIP_ASSERT(esp_info);
> + HIP_ASSERT(locator);
> + HIP_ASSERT(seq);
> + HIP_ASSERT(tuple);

nit: Maybe

HIP_ASSERT(esp_info || locator || seq || tuple ||
           esp_info->new_spi == esp_info->old_spi);

?

> + for (unsigned idx = 0; idx < addresses_in_locator; idx += 1) {

Hmmm, declarations inside for loops - I think we generally avoid this.

> + HIP_IFEL(hip_ll_add_first(&new_esp->dst_addresses, esp_address)
> + != 0, -1,

nit: That looks weird, keep it on one line please.

> + HIP_ASSERT(esp_info);
> + HIP_ASSERT(addr);
> + HIP_ASSERT(tuple);

see above

Diego

review: Approve

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Hi Diego!

> Does this get rid of all instances of hslist?

Not yet. More hslist killer branches will follow.

> I think you could just push such constifications to trunk directly,
> that would make your branches even smaller...

Thanks for the tip. It made me commit a broken code to trunk. And you'll get rice cake for it - bastard ;-)

> > @@ -626,79 +625,104 @@
> > + HIP_ASSERT(esp_info);
> > + HIP_ASSERT(locator);
> > + HIP_ASSERT(seq);
> > + HIP_ASSERT(tuple);
>
> nit: Maybe
>
> HIP_ASSERT(esp_info || locator || seq || tuple ||
> esp_info->new_spi == esp_info->old_spi);
>
> ?

Not a fan: you lose the information, which pointer exactly was NULL which could aid debugging a lot.

> > + for (unsigned idx = 0; idx < addresses_in_locator; idx += 1) {
>
> Hmmm, declarations inside for loops - I think we generally avoid this.

But, but, but they are nice! Because then they have exactly the scope and visibility that they should have. Is there a con? In doc/HACKING, I found example code that declares such variables outside the for() scope, but there is no explicit rule about it.

> > + HIP_IFEL(hip_ll_add_first(&new_esp->dst_addresses, esp_address)
> > + != 0, -1,
>
> nit: That looks weird, keep it on one line please.

I agree that it looks weird, but where exactly does the weirdness to line-length tradeoff lie? I'm never quite sure how aggressively I should keep lines below 80 columns...

Stefan

[Diego Biurrun (diego-biurrun)]

On Tue, Mar 29, 2011 at 06:07:29PM +0000, Stefan Götz wrote:
>
> > Does this get rid of all instances of hslist?
>
> Not yet. More hslist killer branches will follow.

I'm looking forward to them :)

> > I think you could just push such constifications to trunk directly,
> > that would make your branches even smaller...
>
> Thanks for the tip. It made me commit a broken code to trunk. And
> you'll get rice cake for it - bastard ;-)

Your feeble attempt at passing blame around fails miserably ;-p

Splitting off sensible parts from larger changes and pushing them into
trunk is a high art. I'm sure you will learn it eventually, but beware
- the road to mastery is littered with rice cakes ;-p

> > > @@ -626,79 +625,104 @@
> > > + HIP_ASSERT(esp_info);
> > > + HIP_ASSERT(locator);
> > > + HIP_ASSERT(seq);
> > > + HIP_ASSERT(tuple);
> >
> > nit: Maybe
> >
> > HIP_ASSERT(esp_info || locator || seq || tuple ||
> > esp_info->new_spi == esp_info->old_spi);
> >
> > ?
>
> Not a fan: you lose the information, which pointer exactly was NULL
> which could aid debugging a lot.

Yeah, you guys convinced me...

> > > + for (unsigned idx = 0; idx < addresses_in_locator; idx += 1) {
> >
> > Hmmm, declarations inside for loops - I think we generally avoid this.
>
> But, but, but they are nice! Because then they have exactly the scope
> and visibility that they should have. Is there a con? In doc/HACKING,
> I found example code that declares such variables outside the for()
> scope, but there is no explicit rule about it.

Since we dropped the idea of -Wdeclaration-after-statement I guess we
can allow them.

> > > + HIP_IFEL(hip_ll_add_first(&new_esp->dst_addresses, esp_address)
> > > + != 0, -1,
> >
> > nit: That looks weird, keep it on one line please.
>
> I agree that it looks weird, but where exactly does the weirdness to
> line-length tradeoff lie? I'm never quite sure how aggressively I
> should keep lines below 80 columns...

Not that aggressively :)

I don't consider it a hard rule, but it sure helps readability if used
with a measure of flexibility. Keeping such expressions together is
where I'd let a line get longer...

Diego

[Christof Mroz (christof-mroz)]

Hi Stefan, thanks for sane linked list handling, at last.

Unfortunately, `hipfw -kd` segfaults during a ping test:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000404e1d in update_esp_address (addresses=0x6d5720,
    addr=0x7fffffffe920, upd_id=0x0) at firewall/conntrack.c:354
354 *esp_addr->update_id = *upd_id;
(gdb) bt
#0 0x0000000000404e1d in update_esp_address (addresses=0x6d5720,
    addr=0x7fffffffe920, upd_id=0x0) at firewall/conntrack.c:354
#1 0x0000000000406ca4 in handle_i2 (common=0x7fffffffd9a0,
    tuple=0x6d45c0, ctx=0x7fffffffe910)
    at firewall/conntrack.c:1066
#2 0x00000000004080c8 in check_packet (ip6_src=0x7fffffffe920,
    ip6_dst=0x7fffffffe930, common=0x7fffffffd9a0, tuple=0x6d45c0,
    verify_responder=0, accept_mobile=1, ctx=0x7fffffffe910)
    at firewall/conntrack.c:1647
#3 0x0000000000408e45 in conntrack (ip6_src=0x7fffffffe920,
    ip6_dst=0x7fffffffe930, buf=0x7fffffffd9a0, ctx=0x7fffffffe910)
    at firewall/conntrack.c:1958
#4 0x0000000000414198 in filter_hip (ip6_src=0x7fffffffe920,
    ip6_dst=0x7fffffffe930, buf=0x7fffffffd9a0, hook=2,
    in_if=0x7fffffffd944 "eth1", out_if=0x7fffffffd954 "eth2",
    ctx=0x7fffffffe910) at firewall/firewall.c:1042
#5 0x0000000000414218 in hip_fw_handle_hip_output (
    ctx=0x7fffffffe910) at firewall/firewall.c:1062
#6 0x0000000000414472 in hip_fw_handle_hip_forward (
    ctx=0x7fffffffe910) at firewall/firewall.c:1172
#7 0x000000000041541b in hip_fw_handle_packet (
    buf=0x7fffffffd910 "\360\002", hndl=0x6b12d0, ip_version=4,
    ctx=0x7fffffffe910) at firewall/firewall.c:1644
#8 0x0000000000415fcd in hipfw_main (rule_file=0x0,
    kill_old=true, limit_capabilities=false)
    at firewall/firewall.c:1881
#9 0x000000000042187e in main (argc=2, argv=0x7fffffffeac8)
    at firewall/main.c:188
(gdb) p esp_addr
$1 = (struct esp_address *) 0x6d50a0
(gdb) p upd_id
$2 = (const uint32_t * const) 0x0
(gdb)

More about that below. Apart from that, the rest is either unrelated or nitpicking.

> === modified file 'firewall/conntrack.c'
> [...]
> @@ -288,20 +287,19 @@
> [...]
> - while (list) {
> - esp_addr = list->data;
> + while (node) {
> + struct esp_address *const esp_addr = node->ptr;

const struct esp_address *const

> + if ((esp_addr = malloc(sizeof(*esp_addr)))) {
> + esp_addr->dst_addr = *addr;
> esp_addr->update_id = NULL;
> + if (upd_id == NULL || (esp_addr->update_id = malloc(sizeof(*esp_addr->update_id)))) {
> + *esp_addr->update_id = *upd_id;

Looks like if upd_id == NULL, the branch will be entered and a NULL dereference triggered.
Also, the return value of malloc() is not checked for NULL.

Did you find out why the update ID is dynamically allocated in the first place?

> + addr = hip_ll_del_first(&esp_tuple->dst_addresses, NULL);
> + while (addr) {
> free(addr->update_id);
> free(addr);
> -
> - free(list);
> - list = esp_tuple->dst_addr_list;
> + addr = hip_ll_del_first(&esp_tuple->dst_addresses, NULL);
> }

Code duplication can be avoided via

while (addr = hip_ll_del_first(......

review: Needs Fixing

[Diego Biurrun (diego-biurrun)]

On Tue, Mar 29, 2011 at 06:38:23PM +0000, Christof Mroz wrote:
> Review: Needs Fixing
> Hi Stefan, thanks for sane linked list handling, at last.
>
> Unfortunately, `hipfw -kd` segfaults during a ping test:

You actually tested somebody else's merge proposal?
10 points for Gryffindor :-)

Diego

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Hi Diego!

> Splitting off sensible parts from larger changes and pushing them into
> trunk is a high art.  I'm sure you will learn it eventually, but beware
> - the road to mastery is littered with rice cakes ;-p

Oooh, the rice cake strong in this one is.

>> > Hmmm, declarations inside for loops - I think we generally avoid this.
>>
>> But, but, but they are nice! Because then they have exactly the scope
>> and visibility that they should have. Is there a con? In doc/HACKING,
>> I found example code that declares such variables outside the for()
>> scope, but there is no explicit rule about it.
>
> Since we dropped the idea of -Wdeclaration-after-statement I guess we
> can allow them.

Awesome!

>> I agree that it looks weird, but where exactly does the weirdness to
>> line-length tradeoff lie? I'm never quite sure how aggressively I
>> should keep lines below 80 columns...
>
> Not that aggressively :)
>
> I don't consider it a hard rule, but it sure helps readability if used
> with a measure of flexibility.  Keeping such expressions together is
> where I'd let a line get longer...

I'll try to read your mind better from now on ;-)

Stefan

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Hi Christof!

Thanks so much for testing - I really appreciate it!

Wasn't there a plan about some automated testbed? What happened to
that? It'd be fairly cool if one could inject one's branches into such
a testbed for this kind of testing...

>> +    while (node) {
>> +        struct esp_address *const esp_addr = node->ptr;
>
> const struct esp_address *const

ok

>> +    if ((esp_addr = malloc(sizeof(*esp_addr)))) {
>> +        esp_addr->dst_addr  = *addr;
>>          esp_addr->update_id = NULL;
>> +        if (upd_id == NULL || (esp_addr->update_id = malloc(sizeof(*esp_addr->update_id)))) {
>> +            *esp_addr->update_id = *upd_id;
>
> Looks like if upd_id == NULL, the branch will be entered and a NULL dereference triggered.
> Also, the return value of malloc() is not checked for NULL.

ok - obviously, I got the logic wrong there.

> Did you find out why the update ID is dynamically allocated in the first place?

Yes: the update_id for a given destination address may be known or
not. Its value range occupies 32 bits and no magic value is reserved
to indicate the 'unknown' state. Thus, a VCP (very clever person)
decided to encode the 'unknown' state as a NULL pointer. If the
pointer is not NULL, it points to the actual known update ID.

I planned to replace this memory-leak infested mess in a different branch.

> Code duplication can be avoided via
>
> while (addr = hip_ll_del_first(...)) { ... }

Doh - of course!

>> +    HIP_ASSERT(esp_info);
>> +    HIP_ASSERT(locator);
>> +    HIP_ASSERT(seq);
>> +    HIP_ASSERT(tuple);
>> +    HIP_ASSERT(esp_info->new_spi == esp_info->old_spi);
>
> That last assertion looks odd: could this be handled gracefully? I don't know what the RFC says on this though.

The way I understand the code, this is indeed an assertion about code
consistency, i.e. no outsider can trigger this assertion, only someone
who messes up the code in an unintended way.

>> +        HIP_IFEL((new_esp = calloc(1, sizeof(*new_esp))) == NULL, -1,
>> +                 "Allocating esp_tuple object failed");
>>          new_esp->spi   = ntohl(esp_info->new_spi);
>>          new_esp->tuple = tuple;
>> +        hip_ll_init(&new_esp->dst_addresses);
>
> Is this still the state of the art, with your newly added initializer macro?

Yes - the initializer macro is only for static initializations.

>> +        for (unsigned idx = 0; idx < addresses_in_locator; idx += 1) {
>
> Now I'm even more confused about the C99 thing... Please, please tell me that this declaration is allowed :)

Diego just gave it his blessing :)

>> +            struct esp_address *const esp_address =
>> +                malloc(sizeof(*esp_address));
>> +            HIP_IFEL(esp_address == NULL, -1,
>> +                     "Allocating esp_address object for address %i failed",
>> +                     idx);
>> +            esp_address->dst_addr = addresses[idx].address;
>> +            HIP_IFEL(hip_ll_add_first(&new_esp->dst_addresses, esp_address)
>> +                     != 0, -1,
>> +                     "Appending esp_address object %i to list of destination addresses in ESP tuple failed",
>> +                     idx);
>> +            HIP_IFEL((esp_address->u...

[lp:~stefan.goetz-deactivatedaccount/hipl/esp-destination-addresses]

5803. By Stefan Götz on 2011-03-30

Pulled trunk rev 5810

5804. By Stefan Götz on 2011-03-30

Reformat long lines for readability.

5805. By Stefan Götz on 2011-03-30

Add a 'const' qualifier to a local variable to improve correctness. Add a comment that explains an unintuitive return statement.

5806. By Stefan Götz on 2011-03-30

Fix NULL pointer access bug. In the old code, upd_id was dereferenced even if it was NULL. The new code handles errors via HIP_IFEL(), its control flow is more intuitive and thus the error handling is better manageable.

5807. By Stefan Götz on 2011-03-30

Remove unnecessary code duplication.

5808. By Stefan Götz on 2011-03-30

Fix a memory leak bug. In the old code, the memory for a 'struct esp_address'
object was not freed when the subsequent call to 'hip_ll_add_first()' failed.
The new code handles failures in initializing a 'struct esp_address' ojbect
more explicitly and contains the necessary de-allocations in case of a
failure.

[Diego Biurrun (diego-biurrun)]

On Tue, Mar 29, 2011 at 11:13:28PM +0000, Stefan Götz wrote:
>
> > Splitting off sensible parts from larger changes and pushing them into
> > trunk is a high art.  I'm sure you will learn it eventually, but beware
> > - the road to mastery is littered with rice cakes ;-p
>
> Oooh, the rice cake strong in this one is.

Indeed, my number one spot in the eternal rice cake table will likely
remain uncontested for eons to come ;-)

Diego

[Diego Biurrun (diego-biurrun)]

On Tue, Mar 29, 2011 at 09:08:34PM +0200, Diego Biurrun wrote:
> On Tue, Mar 29, 2011 at 06:38:23PM +0000, Christof Mroz wrote:
> > Review: Needs Fixing
> > Hi Stefan, thanks for sane linked list handling, at last.
> >
> > Unfortunately, `hipfw -kd` segfaults during a ping test:
>
> You actually tested somebody else's merge proposal?
> 10 points for Gryffindor :-)

Oh, and many thanks for helping with reviews - we can use all the extra
eyes we can get. It's great to not have to rely on my puny C skills to
keep HIPL in a good shape :)

Diego

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Hi Christof!

>> +            struct esp_address *const esp_address =
>> +                malloc(sizeof(*esp_address));
>> +            HIP_IFEL(esp_address == NULL, -1,
>> +                     "Allocating esp_address object for address %i failed",
>> +                     idx);
>> +            esp_address->dst_addr = addresses[idx].address;
>> +            HIP_IFEL(hip_ll_add_first(&new_esp->dst_addresses, esp_address)
>> +                     != 0, -1,
>> +                     "Appending esp_address object %i to list of destination addresses in ESP tuple failed",
>> +                     idx);
>> +            HIP_IFEL((esp_address->update_id =
>> +                          malloc(sizeof(*esp_address->update_id))) == NULL, -1,
>> +                     "Allocating update_id object for address %i failed",
>> +                     idx);
>> +            *esp_address->update_id = seq->update_id;
>>          }
>> +
>> +        return new_esp;
>>      }
>> -    return new_esp;
>> +
>> +out_err:
>> +    free_esp_tuple(new_esp);
>> +    return NULL;
>>  }
>
> It's also easier to follow IMO if you deallocated it explicitly and not relied on free_esp_tuple() to clean up a half-initialized item.

I changed my mind: the esp_tuple is always in a state that
free_esp_tuple() can handle so it makes sense to use free_esp_tuple()
instead of re-implementing it.

Stefan

[Christof Mroz (christof-mroz)]

On Wed, 30 Mar 2011 23:28:31 +0200, Stefan Götz
<email address hidden> wrote:

>> It's also easier to follow IMO if you deallocated it explicitly and not
>> relied on free_esp_tuple() to clean up a half-initialized item.
>
> I changed my mind: the esp_tuple is always in a state that
> free_esp_tuple() can handle so it makes sense to use free_esp_tuple()
> instead of re-implementing it.

OK, but it took me a while to understand where the memory is deallocated
(if at all) in case of an error, so a comment would be nice.

Unmerged revisions

=== modified file 'firewall/conntrack.c'
--- firewall/conntrack.c	2011-03-29 23:02:26 +0000
+++ firewall/conntrack.c	2011-03-30 11:11:01 +0000
@@ -91,22 +91,21 @@
 /**
  * prints out the list of addresses of esp_addr_list
  *
- * @param addr_list list of addresses
+ * @param addresses list of addresses
  *
  */
-static void print_esp_addr_list(const struct slist *addr_list)
+static void print_esp_addresses(const struct hip_ll *const addresses)
 {
-    const struct slist *list = addr_list;
-    struct esp_address *addr = NULL;
+    const struct hip_ll_node *node = addresses->head;
 
     HIP_DEBUG("ESP dst addr list:\n");
-    while (list) {
-        addr = list->data;
+    while (node) {
+        const struct esp_address *const addr = node->ptr;
         HIP_DEBUG("addr: %s\n", addr_to_numeric(&addr->dst_addr));
         if (addr && addr->update_id != NULL) {
             HIP_DEBUG("upd id: %d\n", *addr->update_id);
         }
-        list = list->next;
+        node = node->next;
     }
     HIP_DEBUG("\n");
 }
@@ -135,7 +134,7 @@
               esp_tuple->spi, esp_tuple->new_spi, esp_tuple->spi_update_id,
               esp_tuple->tuple->direction);
 
-    print_esp_addr_list(esp_tuple->dst_addr_list);
+    print_esp_addresses(&esp_tuple->dst_addresses);
 }
 
 /**
@@ -288,20 +287,19 @@
 /**
  * Find an entry from the given list that matches to the given address
  *
- * @param addr_list the list to be searched for
+ * @param addresses the list to be searched for
  * @param addr the address to matched from the list
  * @return the entry from the list that matched to the given address, or NULL if not found
  */
-static struct esp_address *get_esp_address(const struct slist *addr_list,
-                                           const struct in6_addr *addr)
+static struct esp_address *get_esp_address(const struct hip_ll *const addresses,
+                                           const struct in6_addr *const addr)
 {
-    const struct slist *list     = addr_list;
-    struct esp_address *esp_addr = NULL;
+    const struct hip_ll_node *node = addresses->head;
 
     HIP_DEBUG("get_esp_address\n");
 
-    while (list) {
-        esp_addr = list->data;
+    while (node) {
+        const struct esp_address *const esp_addr = node->ptr;
         HIP_DEBUG("addr: %s \n", addr_to_numeric(&esp_addr->dst_addr));
 
         HIP_DEBUG_HIT("111", &esp_addr->dst_addr);
@@ -309,9 +307,15 @@
 
         if (IN6_ARE_ADDR_EQUAL(&esp_addr->dst_addr, addr)) {
             HIP_DEBUG("addr found\n");
-            return esp_addr;
+            /* cannot return esp_addr because
+             * a) it is const but this function's return type is not
+             * b) it is const for good reason: we do not intend to modify it
+             * c) casting esp_addr to 'struct esp_address*' causes a compiler
+             *    error.
+             */
+            return node->ptr;
         }
-        list = list->next;
+        node = node->next;
     }
     HIP_DEBUG("get_esp_address: addr %s not found\n", addr_to_numeric(addr));
     return NULL;
@@ -321,42 +325,53 @@
  * Insert an address into a list of addresses. If same address exists already,
  * the update_id is replaced with the new value.
  *
- * @param addr_list the address list
+ * @param addresses the address list
  * @param addr the address to be added
  * @param upd_id update id
  *
- * @return the address list
+ * @return true on success, false if insufficient memory is available for a new
+ *         esp address object.
  */
-static struct slist *update_esp_address(struct slist *addr_list,
-                                        const struct in6_addr *addr,
-                                        const uint32_t *upd_id)
+static bool update_esp_address(struct hip_ll *const addresses,
+                               const struct in6_addr *const addr,
+                               const uint32_t *const upd_id)
 {
-    struct esp_address *esp_addr = get_esp_address(addr_list, addr);
-    HIP_DEBUG("update_esp_address: address: %s \n", addr_to_numeric(addr));
+    bool                remove_esp_addr = false;
+    int                 err             = 0;
+    struct esp_address *esp_addr        = get_esp_address(addresses, addr);
+    HIP_DEBUG("address: %s \n", addr_to_numeric(addr));
 
-    if (!addr_list) {
-        HIP_DEBUG("Esp slist is empty\n");
+    // if necessary, allocate a new esp_address object
+    if (!esp_addr) {
+        HIP_IFEL(!(esp_addr = malloc(sizeof(*esp_addr))), -1,
+                 "Allocating esp_address object failed");
+        remove_esp_addr     = true;
+        esp_addr->dst_addr  = *addr;
+        esp_addr->update_id = NULL; // gets set below
+        HIP_IFEL(hip_ll_add_first(addresses, esp_addr) != 0, -1,
+                 "Inserting ESP address object into list of destination addresses failed");
     }
-    if (esp_addr != NULL) {
-        if (upd_id != NULL) {
-            if (esp_addr->update_id == NULL) {
-                esp_addr->update_id = malloc(sizeof(uint32_t));
-            }
-            *esp_addr->update_id = *upd_id;
+
+    // update the update ID
+    if (upd_id) {
+        if (!esp_addr->update_id) {
+            HIP_IFEL(!(esp_addr->update_id = malloc(sizeof(*esp_addr->update_id))),
+                     -1, "Allocating update ID object failed");
         }
-        HIP_DEBUG("update_esp_address: found and updated\n");
-        return addr_list;
-    }
-    esp_addr = malloc(sizeof(struct esp_address));
-    memcpy(&esp_addr->dst_addr, addr, sizeof(struct in6_addr));
-    if (upd_id != NULL) {
-        esp_addr->update_id  = malloc(sizeof(uint32_t));
         *esp_addr->update_id = *upd_id;
-    } else {
-        esp_addr->update_id = NULL;
-    }
-    HIP_DEBUG("update_esp_address: addr created and added\n");
-    return append_to_slist(addr_list, esp_addr);
+    }
+
+    return true;
+
+out_err:
+    if (esp_addr && remove_esp_addr) {
+        if (hip_ll_get(addresses, 0) == esp_addr) {
+            hip_ll_del_first(addresses, NULL);
+        }
+        free(esp_addr->update_id);
+        free(esp_addr);
+    }
+    return false;
 }
 
 /**
@@ -377,7 +392,7 @@
     while (list) {
         struct esp_tuple *tuple = list->data;
         if (spi == tuple->spi) {
-            if (dst_addr && get_esp_address(tuple->dst_addr_list, dst_addr) != NULL) {
+            if (dst_addr && get_esp_address(&tuple->dst_addresses, dst_addr) != NULL) {
                 HIP_DEBUG("connection found by esp\n");
                 return tuple->tuple;
             } else if (!dst_addr) {
@@ -524,23 +539,15 @@
 static void free_esp_tuple(struct esp_tuple *esp_tuple)
 {
     if (esp_tuple) {
-        struct slist       *list = esp_tuple->dst_addr_list;
         struct esp_address *addr = NULL;
 
         // remove eventual cached anchor elements for this esp tuple
         esp_prot_conntrack_remove_state(esp_tuple);
 
         // remove all associated addresses
-        while (list) {
-            esp_tuple->dst_addr_list = remove_link_slist(esp_tuple->dst_addr_list,
-                                                         list);
-            addr = list->data;
-
+        while ((addr = hip_ll_del_first(&esp_tuple->dst_addresses, NULL))) {
             free(addr->update_id);
             free(addr);
-
-            free(list);
-            list = esp_tuple->dst_addr_list;
         }
 
         esp_tuple->tuple = NULL;
@@ -618,7 +625,7 @@
 }
 
 /**
- * create new ESP tuple based on the given parameters
+ * Create an ESP tuple based on the parameters from a HIP message.
  *
  * @param esp_info a pointer to the ESP info parameter in the control message
  * @param locator a pointer to the locator
@@ -626,79 +633,109 @@
  * @param tuple a pointer to the corresponding tuple
  * @return the created tuple (caller frees) or NULL on failure (e.g. SPIs do not match)
  */
-static struct esp_tuple *esp_tuple_from_esp_info_locator(const struct hip_esp_info *esp_info,
-                                                         const struct hip_locator *locator,
-                                                         const struct hip_seq *seq,
-                                                         struct tuple *tuple)
+static struct esp_tuple *esp_tuple_from_esp_info_locator(const struct hip_esp_info *const esp_info,
+                                                         const struct hip_locator *const locator,
+                                                         const struct hip_seq *const seq,
+                                                         struct tuple *const tuple)
 {
-    struct esp_tuple                        *new_esp      = NULL;
-    const struct hip_locator_info_addr_item *locator_addr = NULL;
-    int                                      n            = 0;
-
-    if (esp_info && locator && esp_info->new_spi == esp_info->old_spi) {
-        HIP_DEBUG("esp_tuple_from_esp_info_locator: new spi 0x%lx\n", esp_info->new_spi);
-        /* check that old spi is found */
-        new_esp        = calloc(1, sizeof(struct esp_tuple));
+    int               err     = 0;
+    struct esp_tuple *new_esp = NULL;
+
+    HIP_ASSERT(esp_info);
+    HIP_ASSERT(locator);
+    HIP_ASSERT(seq);
+    HIP_ASSERT(tuple);
+    HIP_ASSERT(esp_info->new_spi == esp_info->old_spi);
+
+    HIP_DEBUG("new spi 0x%lx\n", esp_info->new_spi);
+
+    const unsigned addresses_in_locator =
+        (hip_get_param_total_len(locator) - sizeof(struct hip_locator)) /
+        sizeof(struct hip_locator_info_addr_item);
+    HIP_DEBUG("%d addresses in locator\n", addresses_in_locator);
+    if (addresses_in_locator > 0) {
+        const struct hip_locator_info_addr_item *const addresses =
+            (const struct hip_locator_info_addr_item *) (locator + 1);
+
+        HIP_IFEL((new_esp = calloc(1, sizeof(*new_esp))) == NULL, -1,
+                 "Allocating esp_tuple object failed");
         new_esp->spi   = ntohl(esp_info->new_spi);
         new_esp->tuple = tuple;
+        hip_ll_init(&new_esp->dst_addresses);
 
-        n = (hip_get_param_total_len(locator) - sizeof(struct hip_locator)) /
-            sizeof(struct hip_locator_info_addr_item);
-        HIP_DEBUG("esp_tuple_from_esp_info_locator: %d addresses in locator\n", n);
-        if (n > 0) {
-            locator_addr = (const struct hip_locator_info_addr_item *)
-                           (locator + 1);
-            while (n > 0) {
-                struct esp_address *esp_address = malloc(sizeof(struct esp_address));
-                memcpy(&esp_address->dst_addr,
-                       &locator_addr->address,
-                       sizeof(struct in6_addr));
-                esp_address->update_id  = malloc(sizeof(uint32_t));
-                *esp_address->update_id = seq->update_id;
-                new_esp->dst_addr_list  = append_to_slist(new_esp->dst_addr_list,
-                                                          esp_address);
-                n--;
-                if (n > 0) {
-                    locator_addr++;
+        for (unsigned idx = 0; idx < addresses_in_locator; idx += 1) {
+            struct esp_address *const esp_address =
+                malloc(sizeof(*esp_address));
+            if (esp_address) {
+                esp_address->dst_addr = addresses[idx].address;
+                if ((esp_address->update_id = malloc(sizeof(*esp_address->update_id)))) {
+                    *esp_address->update_id = seq->update_id;
+                    if (hip_ll_add_first(&new_esp->dst_addresses, esp_address) == 0) {
+                        continue;
+                    } else {
+                        HIP_ERROR("Appending esp_address object %i to list of destination addresses in ESP tuple failed", idx);
+                    }
+                    free(esp_address->update_id);
+                } else {
+                    HIP_OUT_ERR(-1, "Allocating update_id object for address %i failed", idx);
                 }
+                free(esp_address);
+            } else {
+                HIP_ERROR("Allocating esp_address object for address %i failed", idx);
             }
-        } else {
-            free(new_esp);
-            new_esp = NULL;
+            goto out_err;
         }
+
+        return new_esp;
     }
-    return new_esp;
+
+out_err:
+    free_esp_tuple(new_esp);
+    return NULL;
 }
 
 /**
- * create a new esp_tuple from the given parameters
+ * Create an esp_tuple object from an esp_info message parameter and with a
+ * specific destination address.
  *
  * @param esp_info a pointer to an ESP info parameter in the control message
  * @param addr a pointer to an address
  * @param tuple a pointer to a tuple structure
  * @return the created ESP tuple (caller frees) or NULL on failure (e.g. SPIs don't match)
  */
-static struct esp_tuple *esp_tuple_from_esp_info(const struct hip_esp_info *esp_info,
-                                                 const struct in6_addr *addr,
-                                                 struct tuple *tuple)
+static struct esp_tuple *esp_tuple_from_esp_info(const struct hip_esp_info *const esp_info,
+                                                 const struct in6_addr *const addr,
+                                                 struct tuple *const tuple)
 {
-    struct esp_address *esp_address;
-    struct esp_tuple   *new_esp = NULL;
+    HIP_ASSERT(esp_info);
+    HIP_ASSERT(addr);
+    HIP_ASSERT(tuple);
 
-    if (esp_info) {
-        new_esp        = calloc(1, sizeof(struct esp_tuple));
+    struct esp_tuple *const new_esp = calloc(1, sizeof(*new_esp));
+    if (new_esp) {
         new_esp->spi   = ntohl(esp_info->new_spi);
         new_esp->tuple = tuple;
-
-        esp_address = malloc(sizeof(struct esp_address));
-
-        memcpy(&esp_address->dst_addr, addr, sizeof(struct in6_addr));
-
-        esp_address->update_id = NULL;
-        new_esp->dst_addr_list = append_to_slist(new_esp->dst_addr_list,
-                                                 esp_address);
+        hip_ll_init(&new_esp->dst_addresses);
+
+        struct esp_address *const esp_address = malloc(sizeof(*esp_address));
+        if (esp_address) {
+            esp_address->dst_addr  = *addr;
+            esp_address->update_id = NULL;
+            if (hip_ll_add_first(&new_esp->dst_addresses, esp_address) == 0) {
+                return new_esp;
+            } else {
+                HIP_ERROR("Inserting esp_address object into ESP destination address list failed");
+            }
+        } else {
+            HIP_ERROR("Allocating esp_address object failed");
+        }
+        free(esp_address);
+    } else {
+        HIP_ERROR("Allocating esp_tuple object failed");
     }
-    return new_esp;
+    free(new_esp);
+
+    return NULL;
 }
 
 /**
@@ -1061,9 +1098,9 @@
         esp_tuple->spi           = ntohl(spi->new_spi);
         esp_tuple->new_spi       = 0;
         esp_tuple->spi_update_id = 0;
-        esp_tuple->dst_addr_list = NULL;
-        esp_tuple->dst_addr_list = update_esp_address(esp_tuple->dst_addr_list,
-                                                      ip6_src, NULL);
+        hip_ll_init(&esp_tuple->dst_addresses);
+        HIP_IFEL(!update_esp_address(&esp_tuple->dst_addresses, ip6_src, NULL),
+                 -1, "adding or updating ESP destination address failed");
         esp_tuple->tuple = other_dir;
 
         other_dir->esp_tuples = append_to_slist(other_dir->esp_tuples, esp_tuple);
@@ -1132,9 +1169,9 @@
         esp_tuple->spi           = ntohl(spi->new_spi);
         esp_tuple->new_spi       = 0;
         esp_tuple->spi_update_id = 0;
-        esp_tuple->dst_addr_list = NULL;
-        esp_tuple->dst_addr_list = update_esp_address(esp_tuple->dst_addr_list,
-                                                      ip6_src, NULL);
+        hip_ll_init(&esp_tuple->dst_addresses);
+        HIP_IFEL(!update_esp_address(&esp_tuple->dst_addresses, ip6_src, NULL),
+                 -1, "adding or updating ESP destination address failed");
         esp_tuple->tuple = other_dir;
 
         insert_esp_tuple(esp_tuple);
@@ -1210,9 +1247,10 @@
                        (locator + 1);
 
         while (n > 0) {
-            esp_tuple->dst_addr_list = update_esp_address(esp_tuple->dst_addr_list,
-                                                          &locator_addr->address,
-                                                          &seq->update_id);
+            HIP_IFEL(!update_esp_address(&esp_tuple->dst_addresses,
+                                         &locator_addr->address,
+                                         &seq->update_id), 0,
+                     "adding or updating ESP destination address failed");
             n--;
 
             if (n > 0) {
@@ -1252,9 +1290,10 @@
         print_esp_tuple(esp_tuple);
 
         while (n > 0) {
-            esp_tuple->dst_addr_list = update_esp_address(esp_tuple->dst_addr_list,
-                                                          &locator_addr->address,
-                                                          &seq->update_id);
+            HIP_IFEL(!update_esp_address(&esp_tuple->dst_addresses,
+                                         &locator_addr->address,
+                                         &seq->update_id), 0,
+                     "adding or updating ESP destination address failed");
             n--;
 
             if (n > 0) {
@@ -1345,6 +1384,10 @@
 
             /* we have to consider the src ip address in case of cascading NATs (see above FIXME) */
             esp_tuple = esp_tuple_from_esp_info(esp_info, ip6_src, other_dir_tuple);
+            if (!esp_tuple) {
+                free(data);
+                HIP_OUT_ERR(0, "Unable to create esp_tuple object from update message");
+            }
 
             other_dir_tuple->esp_tuples = append_to_slist(other_dir_esps,
                                                           esp_tuple);

=== modified file 'firewall/firewall_defines.h'
--- firewall/firewall_defines.h	2011-01-11 13:59:46 +0000
+++ firewall/firewall_defines.h	2011-03-30 11:11:01 +0000
@@ -82,7 +82,7 @@
     uint32_t      spi;
     uint32_t      new_spi;
     uint32_t      spi_update_id;
-    struct slist *dst_addr_list;
+    struct hip_ll dst_addresses;
     struct tuple *tuple;
     /* tracking of the ESP SEQ number */
     uint32_t seq_no;