On Thu, 25 Jan 2024, Rasmus Villemoes wrote:
> On 25/01/2024 09.39, Lee Jones wrote:
> > There is an ongoing effort to replace the use of {v}snprintf() variants
> > with safer alternatives - for a more in depth view, see Jon's write-up
> > on LWN [0] and/or Alex's on the Kernel Self Protection Project [1].
> >
> > Whist executing the task, it quickly became apparent that the initial
> > thought of simply s/snprintf/scnprintf/ wasn't going to be adequate for
> > a number of cases. Specifically ones where the caller needs to know
> > whether the given string ends up being truncated. This is where
> > ssprintf() [based on similar semantics of strscpy()] comes in, since it
> > takes the best parts of both of the aforementioned variants. It has the
> > testability of truncation of snprintf() and returns the number of Bytes
> > *actually* written, similar to scnprintf(), making it a very programmer
> > friendly alternative.
> >
> > Here's some examples to show the differences:
> >
> > Success: No truncation - all 9 Bytes successfully written to the buffer
> >
> > ret = snprintf (buf, 10, "%s", "123456789"); // ret = 9
> > ret = scnprintf(buf, 10, "%s", "123456789"); // ret = 9
> > ret = ssprintf (buf, 10, "%s", "123456789"); // ret = 9
> >
> > Failure: Truncation - only 9 of 10 Bytes written; '-' is truncated
> >
> > ret = snprintf (buf, 10, "%s", "123456789-"); // ret = 10
> >
> > Reports: "10 Bytes would have been written if buf was large enough"
> > Issue: Programmers need to know/remember to check ret against "10"
>
> Yeah, so I'm not at all sure we need yet-another-wrapper with
> yet-another-hard-to-read-prefix when people can just RTFM and learn how
> to check for truncation or whatnot. But if you do this:
As wonderful as it would be for people to "just RTFM", we're seeing a
large number of cases where this isn't happening. Providing a more
programmer friendly way is thought, by people way smarter than me, to be
a solid means to solve this issue. Please also see Kees Cook's related
work to remove strlcpy() use.
> > +/**
> > + * vssprintf - Format a string and place it in a buffer
> > + * @buf: The buffer to place the result into
> > + * @size: The size of the buffer, including the trailing null space
> > + * @fmt: The format string to use
> > + * @args: Arguments for the format string
> > + *
> > + * The return value is the number of characters which have been written
> > into
> > + * the @buf not including the trailing '\0' or -E2BIG if the string was
> > + * truncated. If @size is == 0 the function returns 0.
> > + *
> > + * If you're not already dealing with a va_list consider using ssprintf().
> > + *
> > + * See the vsnprintf() documentation for format string extensions over C99.
> > + */
> > +int vssprintf(char *buf, size_t size, const char *fmt, va_list args)
> > +{
> > + int i;
> > +
> > + if (unlikely(!size))
> > + return 0;
>
> No, don't special-case size 0 here. Passing size==0 should just
> guarantee -E2BIG because that's essentially a programmer error, and the
> calling code is then at least much more likely to not believe that buf
> now contains a nul-terminated (empty) string.
>
> And since it's essentially a bug, there's no need to special-case size 0
> to avoid calling vsnprintf(), just let it be caught by the i >= size check.
Agree. Thanks for the feedback. I will change this.
> > + i = vsnprintf(buf, size, fmt, args);
> > +
> > + if (unlikely(i >= size))
> > + return -E2BIG;
> > +
> > + if (likely(i < size))
> > + return i;
>
> Those two ifs are mutually exclusive, so why the second if() and not
> just a direct "return i"? That final "return size-1" is unreachable, and
> confusing.
That's true. The last line of vscnprintf() essentially means that the
data was truncated, which is caught by the new check. So it should be
reworked to look like this:
```
if (likely(i < size))
return i;
return -E2BIG;
```
Thanks again. That's very helpful.
--
Lee Jones [李琼斯]
