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

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 <kp@FreeBSD.org>
> AuthorDate: 2025-07-12 13:00:38 +0000
> Commit:     Kristof Provost <kp@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