Version 1.6.0

[Ondrej Zajicek]

On Sat, Apr 30, 2016 at 01:20:14AM +0200, Toke Høiland-Jørgensen wrote:
> >Hi
> >
> >I would like to thank Toke Høiland-Jørgensen for the Babel protocol
> >implementation, which was finally merged despite my tardy code reviews.
> 
> Yay, awesome! You're very welcome, and congratulations on the release! :)

Hi

Some comments to my changes. I originally thought that i would just do
some minor formatting changes, but i end with some heavy code changes and
plenty of bugfixes. Hopefully i did not make much more errors.


Some issues i found and fixed (and noted down):

 - modified ge_mod64k() still does not work:

   static inline u16 ge_mod64k(u16 a, u16 b)
   { return (a == b || b - a > 0x8000); }

   Due to C integer promotion rules, b - a is computed in (signed) integers
   and not in u16s, so if b = 1 and a = 2, you get -1, so > is false.
   The solution is to use uint as the type for a, b, so it is computed in
   unsigned integers and result can be safely typecasted to u16:

   static inline int ge_mod64k(uint a, uint b)
   { return (u16)(a - b) < 0x8000; }

 - in some cases (e.g. babel_handle_route_request()), check for plen == 0
   or check if prefix is IPA_NONE is used where check for AE 0 should be
   used instead. Note that these are two different cases - there is the
   default route ::/0 (with AE 2), which is a regular route, and there is
   'wildcard' AE 0, which represent all routes.

 - p->entry_slab allocated but not used

 - struct babel_tlv_header - should also be packed (AFAIK there are some
   archs that has structures padded to alignment of at least int).

 - babel_get_attr() - router-id is printed from a->u.data, but it is
   stored in a->u.ptr->data instead (one more indirection)

 - babel_process_packet() - check for framing error accesses tlv->length
   without checking that it can be accessed (i.e., if there is only one
   remaining byte, the tlv->length is out of bounds).

 - babel_process_packet() - Pad1 TLV is not correctly handled.

 - babel_process_packet() - memory leak in sl_alloc() / sl_free() for
   'cur' variable - if the TLV parsing loop ends correctly, it is not
   freed.

 - babel_send_unicast() - if there is a packet waiting in the socket and
   another TLVs in the queue, the packet is overwritten and TX sequence
   for multicast packets in queue is interrupted.

 - log(L_ERR, ...) should be usually either log(L_REMOTE, ...), or in
   some cases TRACE().

 - babel_read_router_id(), babel_read_next_hop() changed to PARSE_IGNORE
   as these functions just modify the state and do not generate parsed data.

 - babel_read_router_id() - endianity problem for router_id (the value is
   read directly without using get_u64()).

 - babel_read_ihu() - supposes that addr field is zeroed, but nobody
   zeroes it.

 - babel_read_route_request() - non-wildcard may have plen 0 (route ::/0)

 - babel_read_update() - may read after buffer if BABEL_AE_IP6_LL and
   omitted are set

 - babel_read_update() - does BABEL_AE_IP6_LL make sense?

 - babel_write_queue() - there is a queue argument, but the function
   processes ifa->tlv_queue anyways. So i guess unicast messages did not work.
   Also the queue argument uses call by value, should use call by reference.

 - babel_handle_update() - in 'if (msg->router_id == s->router_id) return;'
   should be r->router_id instead of s->router_id. (as the source and msg
   have always the same router-id value).


Feel free to comment any of my changes.


I found also some problems that were hard to fix and not severe, mainly
related to route recalculation and triggered updates. These were left in
the code. I will comment them later.

-- 
Elen sila lumenn' omentielvo

Ondrej 'Santiago' Zajicek (email: santiago at crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://trubka.network.cz/pipermail/bird-users/attachments/20160501/015a0c54/attachment.asc>