[RFC v4 6/7] sprintf: Add [V]SPRINTF_END()

Alejandro Colomar posted 7 patches 5 months ago
[RFC v4 6/7] sprintf: Add [V]SPRINTF_END()
Posted by Alejandro Colomar 5 months ago
These macros take the end of the array argument implicitly to avoid
programmer mistakes.  This guarantees that the input is an array, unlike

	snprintf(buf, sizeof(buf), ...);

which is dangerous if the programmer passes a pointer instead of an
array.

These macros are essentially the same as the 2-argument version of
strscpy(), but with a formatted string, and returning a pointer to the
terminating '\0' (or NULL, on error).

Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Marco Elver <elver@google.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 include/linux/sprintf.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/sprintf.h b/include/linux/sprintf.h
index a0dc35574521..33eb03d0b9b8 100644
--- a/include/linux/sprintf.h
+++ b/include/linux/sprintf.h
@@ -4,6 +4,10 @@
 
 #include <linux/compiler_attributes.h>
 #include <linux/types.h>
+#include <linux/array_size.h>
+
+#define SPRINTF_END(a, fmt, ...)  sprintf_end(a, ENDOF(a), fmt, ##__VA_ARGS__)
+#define VSPRINTF_END(a, fmt, ap)  vsprintf_end(a, ENDOF(a), fmt, ap)
 
 int num_to_str(char *buf, int size, unsigned long long num, unsigned int width);
 
-- 
2.50.0
Re: [RFC v4 6/7] sprintf: Add [V]SPRINTF_END()
Posted by Linus Torvalds 5 months ago
On Wed, 9 Jul 2025 at 19:49, Alejandro Colomar <alx@kernel.org> wrote:
>
> +#define SPRINTF_END(a, fmt, ...)  sprintf_end(a, ENDOF(a), fmt, ##__VA_ARGS__)
> +#define VSPRINTF_END(a, fmt, ap)  vsprintf_end(a, ENDOF(a), fmt, ap)

So I like vsprintf_end() more as a name ("like more" not being "I love
it", but at least it makes me think it's a bit more self-explanatory).

But I don't love screaming macros. They historically scream because
they are unsafe, but they shouldn't be unsafe in the first place.

And I don't think those [V]SPRINTF_END() and ENDOF() macros are unsafe
- they use our ARRAY_SIZE() macro which does not evaluate the
argument, only the type, and is safe to use.

So honestly, this interface looks easy to use, but the screaming must stop.

And none of this has *anything* to do with "end" in this form anyway.

IOW, why isn't this just

  #define sprintf_array(a,...) snprintf(a, ARRAY_SIZE(a), __VA_ARGS__)

which is simpler and more direct, doesn't use the "end" version that
is pointless (it's _literally_ about the size of the array, so
'snprintf' is the right thing to use), doesn't scream, and has a
rather self-explanatory name.

Naming matters.

                Linus
Re: [RFC v4 6/7] sprintf: Add [V]SPRINTF_END()
Posted by Alejandro Colomar 5 months ago
Hi Linus,

On Thu, Jul 10, 2025 at 08:52:13AM -0700, Linus Torvalds wrote:
> On Wed, 9 Jul 2025 at 19:49, Alejandro Colomar <alx@kernel.org> wrote:
> >
> > +#define SPRINTF_END(a, fmt, ...)  sprintf_end(a, ENDOF(a), fmt, ##__VA_ARGS__)
> > +#define VSPRINTF_END(a, fmt, ap)  vsprintf_end(a, ENDOF(a), fmt, ap)
> 
> So I like vsprintf_end() more as a name ("like more" not being "I love
> it", but at least it makes me think it's a bit more self-explanatory).

:-)

> But I don't love screaming macros. They historically scream because
> they are unsafe, but they shouldn't be unsafe in the first place.
> 
> And I don't think those [V]SPRINTF_END() and ENDOF() macros are unsafe
> - they use our ARRAY_SIZE() macro which does not evaluate the
> argument, only the type, and is safe to use.

Yup, it's safe to use.

> So honestly, this interface looks easy to use, but the screaming must stop.
> 
> And none of this has *anything* to do with "end" in this form anyway.

That same thing happened through my head while doing it, but I didn't
think of a better name.

In shadow, we have many interfaces for which we have an uppercase macro
version of many functions that gets array sizes and other extra safety
measures where we can.  (So there, the uppercase versions are indeed
extra safety, instead of the historical "there be dragons".  I use the
uppercase to mean "this does some magic to be safer".)

> IOW, why isn't this just
> 
>   #define sprintf_array(a,...) snprintf(a, ARRAY_SIZE(a), __VA_ARGS__)

Agree.  This is a better name for the kernel.

> which is simpler and more direct, doesn't use the "end" version that
> is pointless (it's _literally_ about the size of the array, so
> 'snprintf' is the right thing to use),

I disagree with snprintf(3), but not because of the input, but rather
because of the output.  I think an API similar to strscpy() would be
better, so it can return an error code for truncation.  In fact, up to
v2, I had a stprintf() (T for truncation) that did exactly that.
However, I found out I could do the same with sprintf_end(), which would
mean one less function to grok, which is why I dropped that part.

I'll use your suggested name, as I like it.  Expect v5 in a few minutes.

> doesn't scream, and has a
> rather self-explanatory name.
> 
> Naming matters.

+1


Have a lovely day!
Alex

> 
>                 Linus

-- 
<https://www.alejandro-colomar.es/>
Re: [RFC v4 6/7] sprintf: Add [V]SPRINTF_END()
Posted by Alejandro Colomar 5 months ago
Hi Linus,

On Thu, Jul 10, 2025 at 08:30:59PM +0200, Alejandro Colomar wrote:
> > IOW, why isn't this just
> > 
> >   #define sprintf_array(a,...) snprintf(a, ARRAY_SIZE(a), __VA_ARGS__)
> 
> Agree.  This is a better name for the kernel.

Oops, I misread.  I thought you were implementing it as

	#define sprintf_array(a, ...)  sprintf_end(a, ENDOF(a), __VA_ARGS__)

So, I prefer my implementation because it returns NULL on truncation.
Compare usage:

	if (linus_sprintf_array(a, "foo") >= ARRAY_SIZE(a))
		goto fail;

	if (alex_sprintf_array(a, "foo") == NULL)
		goto fail;

Another approach would be to have

	if (third_sprintf_array(a, "foo") < 0)  // -E2BIG
		goto fail;

Which was my first approach, but since we have sprintf_end(), let's just
reuse it.


Cheers,
Alex

-- 
<https://www.alejandro-colomar.es/>
Re: [RFC v4 6/7] sprintf: Add [V]SPRINTF_END()
Posted by Linus Torvalds 5 months ago
On Thu, 10 Jul 2025 at 14:21, Alejandro Colomar <alx@kernel.org> wrote:
>
> So, I prefer my implementation because it returns NULL on truncation.

As I pointed out, your implementation is WRONG.

If you want to return an error on truncation, do it right.  Not by
returning NULL, but by actually returning an error.

For example, in the kernel, we finally fixed 'strcpy()'. After about a
million different versions of 'copy a string' where every single
version was complete garbage, we ended up with 'strscpy()'. Yeah, the
name isn't lovely, but the *use* of it is:

 - it returns the length of the result for people who want it - which
is by far the most common thing people want

 - it returns an actual honest-to-goodness error code if something
overflowed, instead of the absoilutely horrible "source length" of the
string that strlcpy() does and which is fundamentally broken (because
it requires that you walk *past* the end of the source,
Christ-on-a-stick what a broken interface)

 - it can take an array as an argument (without the need for another
name - see my earlier argument about not making up new names by just
having generics)

Now, it has nasty naming (exactly the kind of 'add random character'
naming that I was arguing against), and that comes from so many
different broken versions until we hit on something that works.

strncpy is horrible garbage. strlcpy is even worse. strscpy actually
works and so far hasn't caused issues (there's a 'pad' version for the
very rare situation where you want 'strncpy-like' padding, but it
still guarantees NUL-termination, and still has a good return value).

Let's agree to *not* make horrible garbage when making up new versions
of sprintf.

             Linus