Re: [PATCH iptables] libxtables: Fix xtables_ipaddr_to_numeric calls with xtables_ipmask_to_numeric

[Vitaly Chikunov <vt@altlinux.org>]

On Sat, Mar 23, 2024 at 02:56:43PM +0100, Phil Sutter wrote:
> On Sat, Mar 23, 2024 at 06:06:41AM +0300, Vitaly Chikunov wrote:
> > Frequently when addr/mask is printed xtables_ipaddr_to_numeric and
> > xtables_ipmask_to_numeric are called together in one printf call but
> > xtables_ipmask_to_numeric internally calls xtables_ipaddr_to_numeric
> > which prints into the same static buffer causing buffer to be
> > overwritten and addr/mask incorrectly printed in such call scenarios.
> > 
> > Make xtables_ipaddr_to_numeric to use two static buffers rotating their
> > use. This simplistic approach will leave ABI not changed and cover all
> > such use cases.
> 
> I don't quite like the cat'n'mouse game this opens, although it's
> unlikely someone calls it a third time before copying the buffer.
> 
> What do you think about the attached solution?

Your approach is indeed much better. But why double underscore prefix
to a function name, this sounds like reserved identifiers.

 https://en.cppreference.com/w/c/language/identifier

Thanks,
  
> 
> Thanks, Phil

> diff --git a/libxtables/xtables.c b/libxtables/xtables.c
> index f2fcc5c22fb61..54df1bc9336dd 100644
> --- a/libxtables/xtables.c
> +++ b/libxtables/xtables.c
> @@ -1511,12 +1511,19 @@ void xtables_param_act(unsigned int status, const char *p1, ...)
>  	va_end(args);
>  }
>  
> +static void
> +__xtables_ipaddr_to_numeric(const struct in_addr *addrp, char *bufp)
> +{
> +	const unsigned char *bytep = (const void *)&addrp->s_addr;
> +
> +	sprintf(bufp, "%u.%u.%u.%u", bytep[0], bytep[1], bytep[2], bytep[3]);
> +}
> +
>  const char *xtables_ipaddr_to_numeric(const struct in_addr *addrp)
>  {
>  	static char buf[16];
> -	const unsigned char *bytep = (const void *)&addrp->s_addr;
>  
> -	sprintf(buf, "%u.%u.%u.%u", bytep[0], bytep[1], bytep[2], bytep[3]);
> +	__xtables_ipaddr_to_numeric(addrp, buf);
>  	return buf;
>  }
>  
> @@ -1583,7 +1590,8 @@ const char *xtables_ipmask_to_numeric(const struct in_addr *mask)
>  	cidr = xtables_ipmask_to_cidr(mask);
>  	if (cidr == (unsigned int)-1) {
>  		/* mask was not a decent combination of 1's and 0's */
> -		sprintf(buf, "/%s", xtables_ipaddr_to_numeric(mask));
> +		buf[0] = '/';
> +		__xtables_ipaddr_to_numeric(mask, buf + 1);
>  		return buf;
>  	} else if (cidr == 32) {
>  		/* we don't want to see "/32" */