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
