[PATCH] babel: Initialise objects from slab completely

[andreas at rammhold.de]

On 23:08 22.11.20, Toke Høiland-Jørgensen wrote:
> The babel protocol code was initialising objects returned from the slab
> allocator by assigning to each of the struct members individually, but
> wasn't touching the NODE member while doing so. This leads to warnings on
> debug builds since commit:
> 
> baac7009063d ("List expensive check.")
> 
> So let's change the babel code to make sure we also zero-initialise the
> list node. For the packet parser, just move the existing memset() out of
> babel_read_tlv() and in all other places, use struct assignment for
> initialising members, which is idiomatic for Bird.
> 
> Signed-off-by: Toke Høiland-Jørgensen <toke at toke.dk>
> ---
>  proto/babel/babel.c   | 31 ++++++++++++++++++-------------
>  proto/babel/packets.c |  7 ++++---
>  2 files changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/proto/babel/babel.c b/proto/babel/babel.c
> index 618abaa8a..588b29eaa 100644
> --- a/proto/babel/babel.c
> +++ b/proto/babel/babel.c
> @@ -114,10 +114,12 @@ babel_get_source(struct babel_proto *p, struct babel_entry *e, u64 router_id)
>      return s;
>  
>    s = sl_alloc(p->source_slab);
> -  s->router_id = router_id;
> -  s->expires = current_time() + BABEL_GARBAGE_INTERVAL;
> -  s->seqno = 0;
> -  s->metric = BABEL_INFINITY;
> +  *s = (struct babel_source) {
> +    .router_id = router_id,
> +    .expires = current_time() + BABEL_GARBAGE_INTERVAL,
> +    .seqno = 0,
> +    .metric = BABEL_INFINITY,
> +  };
>    add_tail(&e->sources, NODE s);
>  
>    return s;
> @@ -160,10 +162,10 @@ babel_get_route(struct babel_proto *p, struct babel_entry *e, struct babel_neigh
>      return r;
>  
>    r = sl_alloc(p->route_slab);
> -  memset(r, 0, sizeof(*r));
> -
> -  r->e = e;
> -  r->neigh = nbr;
> +  *r = (struct babel_route) {
> +    .e = e,
> +    .neigh = nbr
> +  };
>    add_tail(&e->routes, NODE r);
>    add_tail(&nbr->routes, NODE &r->neigh_route);
>  
> @@ -326,11 +328,14 @@ babel_add_seqno_request(struct babel_proto *p, struct babel_entry *e,
>    sr = sl_alloc(p->seqno_slab);
>  
>  found:
> -  sr->router_id = router_id;
> -  sr->seqno = seqno;
> -  sr->hop_count = hop_count;
> -  sr->count = 0;
> -  sr->expires = current_time() + BABEL_SEQNO_REQUEST_EXPIRY;
> +  *sr = (struct babel_seqno_request) {
> +    .router_id = router_id,
> +    .seqno = seqno,
> +    .hop_count = hop_count,
> +    .count = 0,
> +    .expires = current_time() + BABEL_SEQNO_REQUEST_EXPIRY,
> +  };
> +
>    babel_lock_neighbor(sr->nbr = nbr);
>    add_tail(&e->requests, NODE sr);
>  
> diff --git a/proto/babel/packets.c b/proto/babel/packets.c
> index d4ecf649d..98037ce69 100644
> --- a/proto/babel/packets.c
> +++ b/proto/babel/packets.c
> @@ -1144,7 +1144,6 @@ babel_read_tlv(struct babel_tlv *hdr,
>      return PARSE_ERROR;
>  
>    state->current_tlv_endpos = tlv_data[hdr->type].min_length;
> -  memset(msg, 0, sizeof(*msg));
>  
>    int res = tlv_data[hdr->type].read_tlv(hdr, msg, state);
>    if (res != PARSE_SUCCESS)
> @@ -1278,7 +1277,7 @@ babel_send_unicast(union babel_msg *msg, struct babel_iface *ifa, ip_addr dest)
>    struct babel_msg_node *msgn = sl_alloc(p->msg_slab);
>    list queue;
>  
> -  msgn->msg = *msg;
> +  *msgn = (struct babel_msg_node) { .msg = *msg };
>    init_list(&queue);
>    add_tail(&queue, NODE msgn);
>    babel_write_queue(ifa, &queue);
> @@ -1305,7 +1304,7 @@ babel_enqueue(union babel_msg *msg, struct babel_iface *ifa)
>  {
>    struct babel_proto *p = ifa->proto;
>    struct babel_msg_node *msgn = sl_alloc(p->msg_slab);
> -  msgn->msg = *msg;
> +  *msgn = (struct babel_msg_node) { .msg = *msg };
>    add_tail(&ifa->msg_queue, NODE msgn);
>    babel_kick_queue(ifa);
>  }
> @@ -1387,6 +1386,8 @@ babel_process_packet(struct babel_pkt_header *pkt, int len,
>      }
>  
>      msg = sl_alloc(p->msg_slab);
> +    memset(msg, 0, sizeof(*msg));

Likely something for another commit but maybe adding an sl_allocz
variant that always returns zeroed memory would be helpful? I think
Maria wrote something similar in their mail earlier.

> +
>      res = babel_read_tlv(tlv, &msg->msg, &state);
>      if (res == PARSE_SUCCESS)
>      {
> -- 
> 2.29.2
> 

This removes most of the traces I've reported previously
(Message-Id:<20201122154715.waewttuqdm6jm4gb at wrt>).

If BIRD does Test-By lines:

Tested-By: Andreas Rammhold <andreas at rammhold.de>