| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20260208171456.16547cac@pumpkin>
Date: Sun, 8 Feb 2026 17:14:56 +0000
From: David Laight <david.laight.linux@...il.com>
To: Willy Tarreau <w@....eu>
Cc: Thomas Weißschuh <linux@...ssschuh.net>,
linux-kernel@...r.kernel.org, Cheng Li <lechain@...il.com>
Subject: Re: [PATCH v2 next 07/11] tools/nolibc/printf: Add support for
conversion flags "#- +" and format "%X"
On Sun, 8 Feb 2026 16:47:23 +0100
Willy Tarreau <w@....eu> wrote:
> On Fri, Feb 06, 2026 at 07:11:17PM +0000, david.laight.linux@...il.com wrote:
> > -/* simple printf(). It supports the following formats:
> > - * - %[-][width][{l,t,z,ll,L,j,q}]{d,u,c,x,p,s,m,%}
> > - * - %%
> > - * - invalid formats are copied to the output buffer
> > +/* printf(). Supports most of the normal integer and string formats.
> > + * - %[#-+ ][width][{l,t,z,ll,L,j,q}]{d,i,u,c,x,X,p,s,m,%}
> > + * - %% generates a single %
> > + * - %m outputs strerror(errno).
> > + * - # only affects %x and prepends 0x to non-zero values.
> > + * - %o (octal) isn't supported.
> > + * - %X outputs a..f the same as %x.
> > + * - No support for zero padding, precision or variable widths.
> > + * - No support for wide characters.
> > + * - invalid formats are copied to the output buffer.
> > */
>
> Thanks for updating this one, it does help quite a bit.
>
> > /* This code uses 'flag' variables that are indexed by the low 6 bits
> > @@ -279,7 +285,7 @@ int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list
> > unsigned int written, width;
> > unsigned int flags, ch_flag;
> > size_t len;
> > - char tmpbuf[21];
> > + char tmpbuf[32 + 24];
>
> The previous buffer was sized to store a 64-bit int. I couldn't figure
> what these 32 and 24 correspond to with the new supported specifiers.
> Maybe please add a short comment on the line to hint about what they
> correspond to ?
The could just be 2 here, but the next patch needs extra space for
all the '0' bytes.
I do like to make char[] arrays multiple of 4 bytes as well.
That is one reason for rounding the 21 up to 24.
It does also mean the line doesn't need changing to support octal.
>
> > const char *outstr;
> >
> > written = 0;
> > @@ -334,19 +340,32 @@ int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list
> >
> > /* Conversion specifiers. */
> >
> > - /* Numeric conversion specifiers. */
> > - ch_flag = _NOLIBC_PF_CHAR_IS_ONE_OF(ch, 'c', 'd', 'i', 'u', 'x', 'p');
> > - if (ch_flag != 0) {
> > + /* Numeric and pointer conversion specifiers.
> > + *
> > + * Use an explicit bound check (rather than _NOLIBC_PF_CHAR_IS_ONE_OF())
> > + * so that 'X' can be allowed through.
> > + * 'X' gets treated and 'x' because _NOLIBC_PF_FLAG() returns the same
> > + * value for both.
> > + */
> > + if ((ch < 'a' || ch > 'z') && ch != 'X')
> > + goto non_numeric_conversion;
> > +
> > + /* We need to check for "%p" or "%#x" later, merging here gives better code.
> > + * But '#' collides with 'c' so shift right.
> > + */
> > + ch_flag = _NOLIBC_PF_FLAG(ch) | (flags & _NOLIBC_PF_FLAG('#')) >> 1;
> > + if (_NOLIBC_PF_FLAGS_CONTAIN(ch_flag, 'c', 'd', 'i', 'u', 'x', 'p', 's')) {
> > unsigned long long v;
> > long long signed_v;
> > - char *out = tmpbuf;
> > + char *out = tmpbuf + 32;
>
> OK so you seem to be reserving a part of the buffer for certain uses ?
Sign and zero pad (next patch).
Ok, make a comment...
>
> > + int sign = 0;
> >
> > /* 'long' is needed for pointer/string conversions and ltz lengths.
> > * A single test can be used provided 'p' (the same bit as '0')
> > * is masked from flags.
> > */
> > if (_NOLIBC_PF_FLAGS_CONTAIN(ch_flag | (flags & ~_NOLIBC_PF_FLAG('p')),
> > - 'p', 'l', 't', 'z')) {
> > + 'p', 's', 'l', 't', 'z')) {
> > v = va_arg(args, unsigned long);
> > signed_v = (long)v;
> > } else if (_NOLIBC_PF_FLAGS_CONTAIN(flags, 'j', 'q')) {
> > @@ -365,40 +384,62 @@ int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list
> > goto do_output;
> > }
> >
> > + if (_NOLIBC_PF_FLAGS_CONTAIN(ch_flag, 's')) {
> > + /* "%s" - character string. */
> > + if (!v) {
> > + outstr = "(null)";
> > + len = 6;
> > + goto do_output;
> > + }
> > + outstr = (void *)v;
> > +do_strnlen_output:
> > + len = strnlen(outstr, INT_MAX);
>
> I get why you turned strlen() to strnlen(INT_MAX) (result being an int)
> but this will not change anything IMHO in that the rest of a 2GB+ string
> will be written in multiple passes and will overflow the output anyway.
> Thus I think that sticking to strlen() remains simpler and less confusing.
Wrong reason.
The next patch changes it to:
len = strnlen(outstr, precision);
and I didn't want to change the name of the label as well as this line.
>
> (...)
> > - else if (ch == 'm') {
> > +
> > +non_numeric_conversion:
> > + if (ch == 'm') {
> > #ifdef NOLIBC_IGNORE_ERRNO
> > outstr = "unknown error";
> > + len = __builtin_strlen(outstr);
> > #else
> > outstr = strerror(errno);
> > + goto do_strnlen_output;
> > #endif /* NOLIBC_IGNORE_ERRNO */
>
> It's simlper (and smaller) to use the common label for both here:
>
> #ifdef NOLIBC_IGNORE_ERRNO
> outstr = "unknown error";
> #else
> outstr = strerror(errno);
> #endif /* NOLIBC_IGNORE_ERRNO */
> + goto do_strnlen_output;
If you look closely strerror() gets inlined here (at least with the makefile
that builds the test program).
I think you'll get smaller code by marking some of the functions noinline.
There is an 'interesting' option for 64bit.
Put "errno=" into 'sign', set 'v = errno', clear '.' (or set precision to 1)
and jump to the 'output in decimal' code.
strerror() could then be implemented using snprintf(buf, n, "%m");
Related is inlining the 'divide by reciprocal' u64toa() code into sprintf()
- that might save it spilling to stack and be smaller.
Then implement atoi() (etc) using snprintf().
Might generate smaller code overall - but it isn't a serious suggestion.
David
>
> Overall OK to me.
Ta
>
> Willy
Powered by blists - more mailing lists