Hi Jeremy,
On Mon, Nov 18, 2024 at 01:56:50PM +0000, Jeremy Sowden wrote:
[...]
> Remove the mask parameters from `is_same_interfaces`. Add a test-case.
Thanks for the fix and test-case!
Some remarks below:
[...]
> bool is_same_interfaces(const char *a_iniface, const char *a_outiface,
> - unsigned const char *a_iniface_mask,
> - unsigned const char *a_outiface_mask,
> - const char *b_iniface, const char *b_outiface,
> - unsigned const char *b_iniface_mask,
> - unsigned const char *b_outiface_mask)
> + const char *b_iniface, const char *b_outiface)
> {
> int i;
>
> for (i = 0; i < IFNAMSIZ; i++) {
> - if (a_iniface_mask[i] != b_iniface_mask[i]) {
> - DEBUGP("different iniface mask %x, %x (%d)\n",
> - a_iniface_mask[i] & 0xff, b_iniface_mask[i] & 0xff, i);
> - return false;
> - }
> - if ((a_iniface[i] & a_iniface_mask[i])
> - != (b_iniface[i] & b_iniface_mask[i])) {
> + if (a_iniface[i] != b_iniface[i]) {
> DEBUGP("different iniface\n");
> return false;
> }
> - if (a_outiface_mask[i] != b_outiface_mask[i]) {
> - DEBUGP("different outiface mask\n");
> - return false;
> - }
> - if ((a_outiface[i] & a_outiface_mask[i])
> - != (b_outiface[i] & b_outiface_mask[i])) {
> + if (a_outiface[i] != b_outiface[i]) {
> DEBUGP("different outiface\n");
> return false;
> }
My draft fix converts this to strncmp() calls, I don't think we should
inspect bytes past the NUL-char. Usually we parse into a zeroed
iptables_command_state, but if_indextoname(3P) does not define output
buffer contents apart from "shall place in this buffer the name of the
interface", so it may put garbage in there (although unlikely).
Another thing is a potential follow-up: There are remains in
nft_arp_post_parse() and ipv6_post_parse(), needless filling of the mask
buffers. They may be dropped along with the now unused mask fields in
struct xtables_args.
WDYT?
Thanks, Phil
