Hi, On Mon, Jul 15, 2019 at 04:46:09PM +0200, Antonio Quartulli wrote: > From: Antonio Quartulli <anto...@openvpn.net> > > get_default_gateway_ipv6() has always been implemented using > netlink, however, now that we have sitnl, we can re-use the > latter and get rid of the netlink code from route.c. > > Signed-off-by: Antonio Quartulli <anto...@openvpn.net>
Yeah, thanks. This particular patch is ok now, but it is calling functions that are no save to call. So, fixing needed. (I considered on whether to NAK this again, but since it is safe *today*, this is more for the work queue of cleanup to do). Down in networking_sitnl.c, in sitnl_route_best_gw(), you do strcpy(best_iface, res.iface); which *today* is safe, because best_iface is "char[16]" and res.iface is "char[IFNAMSIZ]", which happens to be 16 on linux *today*. Now what if folks decide "we need longer names, because docker? or anything?" - bam. This needs to be a strncpy(). Or "struct route_ipv6_gateway_info" -> iface needs to be [IFNAMSIZ] with an preceding "#ifndef IFNAMSIZ, #define IFNAMSIZ 16" for platforms that do not have said define... Or something else to guarantee that you never overflow *best_iface. Then, I'm a bit sad that your sitnl_route_best_gw() code pretty much lacks all documentation about what it *does*. My code in route.c that talks to netlink had quite a bit of explanations on "why a certain value is set in a certain way"... rtreq.nh.nlmsg_type = RTM_GETROUTE; rtreq.nh.nlmsg_flags = NLM_F_REQUEST; /* best match only */ rtreq.rtm.rtm_family = AF_INET6; rtreq.rtm.rtm_src_len = 0; /* not source dependent */ rtreq.rtm.rtm_dst_len = 128; /* exact dst */ rtreq.rtm.rtm_table = RT_TABLE_MAIN; rtreq.rtm.rtm_protocol = RTPROT_UNSPEC; while your code leaves off initialization for "anything that is default anyway" and just magically copies in values for the rest - including "0" for "rtm_dst_len" (both callers pass in "0" for prefixlen, which doesn't match the documentation I've read, back then, that says you must pass in a full prefix length for an exact route match - and if you pass in 0 anyway, why carry a prefixlen argument at all??). The rtm_dst_len puzzles me, to be honest... either I misread the documentation, or this is ignored by the kernel... the way I understood this, it's the number of bits that is matched in the address, so "0" would pretty much mean "match any route" - but what it returns is correct, so it's not doing "match any". *scratch head*. gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel