| 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: <aYLHuzt5AGO6h2yW@1wt.eu>
Date: Wed, 4 Feb 2026 05:14:51 +0100
From: Willy Tarreau <w@....eu>
To: david.laight.linux@...il.com
Cc: Thomas Weißschuh <linux@...ssschuh.net>,
linux-kernel@...r.kernel.org, Cheng Li <lechain@...il.com>
Subject: Re: [PATCH next 06/12] tools/nolibc/printf: Add support for left
alignment and %[tzLq]d"
Hi David,
On Tue, Feb 03, 2026 at 10:29:54AM +0000, david.laight.linux@...il.com wrote:
> From: David Laight <david.laight.linux@...il.com>
>
> Use a single 'flags' variable to hold both format flags and length modifiers.
> Use (1u << (c & 31)) for the flag bits to reduce code complexity.
>
> Add support for left justifying fields.
>
> Add support for length modifiers 't' and 'z' (both long) and 'q' and 'L'
> (both long long).
>
> Unconditionall generate the signed values (for %d) to remove a second
> set of checks for the size.
>
> Use 'signed int' for the lengths to make the pad test simpler.
>
> Signed-off-by: David Laight <david.laight.linux@...il.com>
> ---
> tools/include/nolibc/stdio.h | 88 ++++++++++++++++++++----------------
> 1 file changed, 50 insertions(+), 38 deletions(-)
>
> diff --git a/tools/include/nolibc/stdio.h b/tools/include/nolibc/stdio.h
> index 164d2384978e..1ce4d357a802 100644
> --- a/tools/include/nolibc/stdio.h
> +++ b/tools/include/nolibc/stdio.h
> @@ -240,20 +240,20 @@ char *fgets(char *s, int size, FILE *stream)
> }
>
>
> -/* minimal printf(). It supports the following formats:
> - * - %[l*]{d,u,c,x,p}
> - * - %s
> - * - unknown modifiers are ignored.
> +/* 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
> */
> typedef int (*__nolibc_printf_cb)(void *state, const char *buf, size_t size);
>
> +#define __PF_FLAG(c) (1u << ((c) & 0x1f))
This flag will be exposed to user code, you'll have to previx it with
_NOLIBC_.
> static __attribute__((unused, format(printf, 3, 0)))
> int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list args)
> {
> - char lpref, c;
> - unsigned long long v;
> - unsigned int written, width;
> - size_t len;
> + char c;
> + int len, written, width;
> + unsigned int flags;
> char tmpbuf[21];
> const char *outstr;
>
> @@ -265,6 +265,7 @@ int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list
> break;
>
> width = 0;
> + flags = 0;
> if (c != '%') {
> while (*fmt && *fmt != '%')
> fmt++;
> @@ -274,6 +275,13 @@ int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list
>
> c = *fmt++;
>
> + /* Flag characters */
> + for (; c >= 0x20 && c <= 0x3f; c = *fmt++) {
> + if ((__PF_FLAG(c) & (__PF_FLAG('-'))) == 0)
> + break;
> + flags |= __PF_FLAG(c);
> + }
Honestly I don't find that it improves readability here and makes one
keep doubts in background as "what if c == 'm' which will match '-'?".
I think that one would better be written as the usual:
for (; c >= 0x20 && c <= 0x3f; c = *fmt++) {
if (c == '-')
break;
flags |= __PF_FLAG(c);
}
Or even simpler since there's already a condition in the for() loop:
for (; c >= 0x20 && c != '-' && c <= 0x3f; c = *fmt++)
flags |= __PF_FLAG(c);
> /* width */
> while (c >= '0' && c <= '9') {
> width *= 10;
> @@ -282,41 +290,34 @@ int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list
> c = *fmt++;
> }
>
> - /* Length modifiers */
> - if (c == 'l') {
> - lpref = 1;
> - c = *fmt++;
> - if (c == 'l') {
> - lpref = 2;
> + /* Length modifiers are lower case except 'L' which is the same a 'q' */
> + if ((c >= 'a' && c <= 'z') || (c == 'L' && (c = 'q'))) {
Then please say "... which we replace by 'q'" so that we don't first read
that (c = 'q') as a possible typo. Also maybe add a mention to the fact
that "flags" will exclusively represent lowercase modifiers from now on?
> + if (__PF_FLAG(c) & (__PF_FLAG('l') | __PF_FLAG('t') | __PF_FLAG('z') |
> + __PF_FLAG('j') | __PF_FLAG('q'))) {
Even though I understand the value in checking bit positions (I use that
all the time as well), above this is just unreadable. Maybe you need a
different macro, maybe define another macro _NOLIBC_PF_LEN_MOD made of
the addition of all the flags to test against, I don't know, but the
construct, the line break in the middle of the expression and the
parenthesis needed for the macro just requires a lot of effort to
understand what's being tested. Alternately another possibility would
be to have another macro taking 4-5 char args and composing the flags
in one call, passing 0 or -1 for unused ones. This would also make
several parenthesis disappear which would help.
> + if (c == 'l' && fmt[0] == 'l') {
> + fmt++;
> + c = 'q';
> + }
> + /* These all miss "# -0+" */
> + flags |= __PF_FLAG(c);
> c = *fmt++;
> }
> - } else if (c == 'j') {
> - /* intmax_t is long long */
> - lpref = 2;
> - c = *fmt++;
> - } else {
> - lpref = 0;
> }
>
> if (c == 'c' || c == 'd' || c == 'u' || c == 'x' || c == 'p') {
> + unsigned long long v;
> + long long signed_v;
> char *out = tmpbuf;
>
> - if (c == 'p')
> + if ((c == 'p') || (flags & (__PF_FLAG('l') | __PF_FLAG('t') | __PF_FLAG('z')))) {
> v = va_arg(args, unsigned long);
> - else if (lpref) {
> - if (lpref > 1)
> - v = va_arg(args, unsigned long long);
> - else
> - v = va_arg(args, unsigned long);
> - } else
> + signed_v = (long)v;
> + } else if (flags & (__PF_FLAG('j') | __PF_FLAG('q'))) {
> + v = va_arg(args, unsigned long long);
> + signed_v = v;
> + } else {
> v = va_arg(args, unsigned int);
> -
> - if (c == 'd') {
> - /* sign-extend the value */
> - if (lpref == 0)
> - v = (long long)(int)v;
> - else if (lpref == 1)
> - v = (long long)(long)v;
> + signed_v = (int)v;
> }
>
> switch (c) {
> @@ -325,7 +326,7 @@ int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list
> out[1] = 0;
> break;
> case 'd':
> - i64toa_r(v, out);
> + i64toa_r(signed_v, out);
> break;
> case 'u':
> u64toa_r(v, out);
> @@ -366,14 +367,24 @@ int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list
>
> written += len;
>
> - while (width > len) {
> - unsigned int pad_len = ((width - len - 1) & 15) + 1;
> + /* An OPTIMIZER_HIDE_VAR() seems to stop gcc back-merging this
> + * code into one of the conditionals above.
Be careful, space indentation above.
> + */
> + __asm__ volatile("" : "=r"(len) : "0"(len));
and here.
> +
> + /* Output 'left pad', 'value' then 'right pad'. */
> + flags &= __PF_FLAG('-');
> + width -= len;
> + if (flags && cb(state, outstr, len) != 0)
> + return -1;
> + while (width > 0) {
> + int pad_len = ((width - 1) & 15) + 1;
> width -= pad_len;
> written += pad_len;
> if (cb(state, " ", pad_len) != 0)
> return -1;
> }
> - if (cb(state, outstr, len) != 0)
> + if (!flags && cb(state, outstr, len) != 0)
> return -1;
> }
>
> @@ -382,6 +393,7 @@ int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list
>
> return written;
> }
> +#undef _PF_FLAG
This one can be dropped once named as _NOLIBC_xxx
Willy
Powered by blists - more mailing lists