Re: git: 3e827cbaa364 - main - ipfilter: fix LINT-NOINET6 build

[John Baldwin]

On 7/12/25 09:39, Kristof Provost wrote:

The branch main has been updated by kp:

URL: 
https://cgit.FreeBSD.org/src/commit/?id=3e827cbaa3641d7137d7c7f1af326243bf46ae15

commit 3e827cbaa3641d7137d7c7f1af326243bf46ae15
Author:     Kristof Provost <k...@freebsd.org>
AuthorDate: 2025-07-12 13:00:38 +0000
Commit:     Kristof Provost <k...@freebsd.org>
CommitDate: 2025-07-12 13:04:16 +0000

     ipfilter: fix LINT-NOINET6 build

Event: Berlin 2025 Hackathon

     Sponsored by:   Rubicon Communications, LLC ("Netgate")
---
  sys/netpfil/ipfilter/netinet/ip_fil_freebsd.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sys/netpfil/ipfilter/netinet/ip_fil_freebsd.c 
b/sys/netpfil/ipfilter/netinet/ip_fil_freebsd.c
index 04850549db98..6eb6cf2a7a47 100644
--- a/sys/netpfil/ipfilter/netinet/ip_fil_freebsd.c
+++ b/sys/netpfil/ipfilter/netinet/ip_fil_freebsd.c
@@ -463,13 +463,14 @@ ipf_send_ip(fr_info_t *fin, mb_t *m)
  int
  ipf_send_icmp_err(int type, fr_info_t *fin, int dst)
  {
-       int err, hlen, xtra, iclen, ohlen, avail, code;
+       int err, hlen, xtra, iclen, ohlen, avail;
        struct in_addr dst4;
        struct icmp *icmp;
        struct mbuf *m;
        i6addr_t dst6;
        void *ifp;
  #ifdef USE_INET6
+       int code;
        ip6_t *ip6;
  #endif
        ip_t *ip, *ip2;
@@ -477,8 +478,8 @@ ipf_send_icmp_err(int type, fr_info_t *fin, int dst)
        if ((type < 0) || (type >= ICMP_MAXTYPE))
                return (-1);

- code = fin->fin_icode;

  #ifdef USE_INET6
+       code = fin->fin_icode;
        /* See NetBSD ip_fil_netbsd.c r1.4: */
        if ((code < 0) || (code >= sizeof(icmptoicmp6unreach)/sizeof(int)))
                return (-1);

I noticed this locally while testing another change, and this is the same
build fix I was contemplating.

However, I think the check is overly broad as the value of code depends on
what type of ICMP error is being sent, and this range check is only
valid for destination unreachable errors.  That is, I think the check
should be moved down to where the code is used which would make the
reason for the check more obvious and avoid handling other ICMP packets
incorrectly:

#ifdef USE_INET6
        else if (fin->fin_v == 6) {
                hlen = sizeof(ip6_t);
                ohlen = sizeof(ip6_t);
                iclen = hlen + offsetof(struct icmp, icmp_ip) + ohlen;
                type = icmptoicmp6types[type];
                if (type == ICMP6_DST_UNREACH)
                        code = icmptoicmp6unreach[code];

Here we should probably be doing something like:

       if (type == ICMP6_DST_UNREACH) {
           int code;

           code = fin->fin_code;
           if (code < 0 || code >= nitems(icmptoicmp6unreach)) {
               FREE_MB_T(m);
               return (-1);
           }
           code = icmptoicmp6unreach(code);
       }

This would also avoid having the mostly unused `code` variable hanging
around for the rest of the function.

--
John Baldwin