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 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.

Additional thoughts are: the call order of these two function is
important in will it override buf or not. Order of evaluation of
function arguments is unspecified[1] and compile time dependent. It
seems that on non-x86 architectures, sometime it could be different from
left-to-right. That's why this bug was unnoticed for so much time.

[1] https://en.cppreference.com/w/c/language/eval_order

Thanks,

> 
> Interestingly, testing stumbled over this only on non-x86 architectures.
> Error message:
> 
>   extensions/libebt_arp.t: ERROR: line 11 (cannot find: ebtables -I INPUT -p ARP --arp-ip-src ! 1.2.3.4/255.0.255.255)
> 
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> Cc: Jan Engelhardt <jengelh@inai.de>
> Cc: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
> ---
>  libxtables/xtables.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/libxtables/xtables.c b/libxtables/xtables.c
> index 748a50fc..16a0640d 100644
> --- a/libxtables/xtables.c
> +++ b/libxtables/xtables.c
> @@ -1505,7 +1505,9 @@ void xtables_param_act(unsigned int status, const char *p1, ...)
>  
>  const char *xtables_ipaddr_to_numeric(const struct in_addr *addrp)
>  {
> -	static char buf[16];
> +	static char abuf[2][16];
> +	static int bufnum = 0;
> +	char *buf = abuf[++bufnum & 1];
>  	const unsigned char *bytep = (const void *)&addrp->s_addr;
>  
>  	sprintf(buf, "%u.%u.%u.%u", bytep[0], bytep[1], bytep[2], bytep[3]);
> -- 
> 2.42.1