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

[Phil Sutter <phil@nwl.cc>]

[-- Attachment #1: Type: text/plain, Size: 775 bytes --]

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?

Thanks, Phil

[-- Attachment #2: __xtables_ipaddr_to_numeric.diff --]
[-- Type: text/plain, Size: 1197 bytes --]

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" */