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:
> +/**
> + * 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.
> + 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.
Rasmus
