On Tue, Nov 19, 2024 at 08:07:00PM +0000, Jeremy Sowden wrote:
> 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?
Sent a v2, please review.
Thanks, Phil
