Attention is currently required from: plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/454?usp=email )
Change subject: Fix unaligned access in macOS, FreeBSD, Solaris hwaddr ...................................................................... Patch Set 2: Code-Review-1 (5 comments) Patchset: PS2: The ifr memcpy() is well understood and necessary. The sockaddr_dl memcpy() needs at least some better comments, but maybe it's not necessary at all. File src/openvpn/route.c: http://gerrit.openvpn.net/c/openvpn/+/454/comment/b7cb53d2_d5e9c975 : PS2, Line 3677: * requires */ "need to memcpy() to a strict ifr to force 8-byte alignment required for member access" http://gerrit.openvpn.net/c/openvpn/+/454/comment/3ec0b40a_9dd8ee38 : PS2, Line 3691: * struct sockaddr_dl is 20 bytes in size and has On FreeBSD 13 this is ``` struct sockaddr_dl { u_char sdl_len; /* Total length of sockaddr */ u_char sdl_family; /* AF_LINK */ u_short sdl_index; /* if != 0, system given index for interface */ u_char sdl_type; /* interface type */ u_char sdl_nlen; /* interface name length, no trailing 0 reqd. */ u_char sdl_alen; /* link level address length */ u_char sdl_slen; /* link layer selector length */ char sdl_data[46]; /* minimum work area, can be larger; contains both if name and ll address */ }; ``` so the "20 bytes" comment should be qualified "on some of the BSDs" or so (NetBSD indeed has only 20 bytes). OpenBSD has ``` char sdl_data[24]; /* minimum work area, can be larger; contains both if name and ll address; big enough for IFNAMSIZ plus 8byte ll addr */ ``` http://gerrit.openvpn.net/c/openvpn/+/454/comment/5b60619c_c908b9eb : PS2, Line 3693: * and Ethernet interface name (max 16 bytes) the wording here confused me ("if it's hw address and interface name, in this order, why would we care about size of interface name?") - I'd swap the order around ("..for interface name (max 16 bytes) and hw address (max 6 bytes"). http://gerrit.openvpn.net/c/openvpn/+/454/comment/e6546e64_f9ad06e2 : PS2, Line 3703: * Since we only copied 32 bytes from cp to ifr but sdl If ifr_addr.sa_len is >16 (because "long address") the max() above will make us copy more than 32 bytes. So maybe we already have everything we need here, and can spare ourselves the double twist? OTOH, the code below does max(sizeof(), sa_len) - so if sa_len is shorter, we might copy too much with the second copy? -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/454?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Ia797c8801fa9a9bc10b6674efde5fdbd7132e4a8 Gerrit-Change-Number: 454 Gerrit-PatchSet: 2 Gerrit-Owner: plaisthos <arne-open...@rfc2549.org> Gerrit-Reviewer: cron2 <g...@greenie.muc.de> Gerrit-Reviewer: flichtenheld <fr...@lichtenheld.com> Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net> Gerrit-Attention: plaisthos <arne-open...@rfc2549.org> Gerrit-Comment-Date: Sat, 30 Dec 2023 15:03:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
