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