On 2024-11-19, at 14:20:31 +0100, Phil Sutter wrote:
> 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).
Seems reasonable. I was so focussed on the masks and bit-twiddling that
I lost sight of the fact that the code is looping to compare strings. :)
> 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.
Yes, I spotted those. I couldn't see how they were used, but I was
reasonably sure that they weren't related to this bug, so I stopped
looking.
> WDYT?
Agreed on both counts. Shall I incorporate your suggestions and send a
v2 or do you have something already prepared?
J.
Attachment:
signature.asc
Description: PGP signature
