Hi Rasmus,
On Thu, 27 May 2021 at 17:01, Rasmus Villemoes
<rasmus.villem...@prevas.dk> wrote:
>
> On 21/05/2021 16.42, Heinrich Schuchardt wrote:
> > On 21.05.21 16:27, Tom Rini wrote:
> >> On Fri, May 21, 2021 at 04:15:39PM +0200, Heinrich Schuchardt wrote:
> >>> On 21.05.21 14:53, Rasmus Villemoes wrote:
> >>>> On 20/05/2021 19.51, Simon Glass wrote:
> >>>>> Hi Rasmus,
> >>>>>
> >>>>> On Thu, 20 May 2021 at 04:05, Rasmus Villemoes
> >>>>> <rasmus.villem...@prevas.dk> wrote:
> >>>>>>
> >>>>>> Most callers (or callers of callers, etc.) of vsnprintf() are not
> >>>>>> prepared for it to return a negative value.
> >>>>>>
> >>>>>> The only case where that can currently happen is %pD, and it's IMO
> >>>>>> more user-friendly to produce some output that clearly shows that some
> >>>>>> "impossible" thing happened instead of having the message completely
> >>>>>> ignored - or mishandled as for example log.c would currently do.
> >>>>>>
> >>>>>> Signed-off-by: Rasmus Villemoes <rasmus.villem...@prevas.dk>
> >>>>>> ---
> >>>>>> lib/vsprintf.c | 10 +---------
> >>>>>> 1 file changed, 1 insertion(+), 9 deletions(-)
> >>>>>
> >>>>> I think that is debatable. If we want the calling code to be fixed,
> >>>>> then it needs to get an error code back. Otherwise the error will be
> >>>>> apparent to the user but (perhaps) not ever debugged.
> >>>>
> >>>> But it is not the calling code that is at fault for the vsnprintf()
> >>>> implementation (1) being able to fail and (2) actually encountering an
> >>>> ENOMEM situation. There's _nothing_ the calling code can do about that.
> >>>
> >>> include/vsnprintf.h states:
> >>>
> >>> "This function follows C99 vsnprintf, but has some extensions:".
> >>>
> >>> The C99 spec says:
> >>>
> >>> "The vsnprintf function returns the number of characters that would have
> >>> been written had n been sufficiently large, not counting the
> >>> terminating null character, or a negative value if an encoding error
> >>> occurred."
> >>>
> >>> It is obvious that the calling code needs to be fixed if it cannot
> >>> handle negative return values.
> >>>
> >>> So NAK to the patch.
> >>>
> >>> Best regards
> >>>
> >>> Heinrich
> >>>
> >>>>
> >>>> The calling code can be said to be responsible for not passing NULL
> >>>> pointers, but that case is actually handled gracefully in various places
> >>>> in the printf code (both for %pD, but also plain %s).
> >>>>
> >>>>> The definition of printf() allows for the possibility of a negative
> >>>>> return value.
> >>>>
> >>>> First, please distinguish printf() from vsnprintf(). The former (in the
> >>>> normal userspace version) obviously can fail for the obvious EIO, ENOSPC
> >>>> reasons. The latter is indeed allowed to fail per the posix spec, but
> >>>> from a QoI perspective, I'd say it's much better to have a guarantee
> >>>> _for our particular implementation_ that it does not fail (meaning:
> >>>> returns a negative result). There's simply too many direct and indirect
> >>>> users of vsnprintf() that assume the result is non-negative; if we do
> >>>> not provide that guarantee, the alternative is to play a whack-a-mole
> >>>> game and add tons of error-checking code (adding bloat to the image),
> >>>> with almost never any good way to handle it.
> >>>>
> >>>> Take that log_info(" ... %pD") as an example. Suppose we "fix" log.c so
> >>>> that it ignores the message if vsnprintf (or vscnprintf, whatever)
> >>>> returns a negative result, just as print() currently does [which is the
> >>>> other thing that log_info could end up being handled by]. That means
> >>>> nothing gets printed on the console, and nobody gets told about the
> >>>> ENOMEM. In contrast, with this patch, we get
> >>>>
> >>>> Booting <%pD:ENOMEM>
> >>>>
> >>>> printed on the console, so at least _some_ part of the message gets out,
> >>>> and it's apparent that something odd happened. Of course, all of that is
> >>>> in the entirely unlikely sitation where the (efi) allocation would
> >>>> actually fail.
> >>>>
> >>>> If we don't want that <%pD:ENOMEM> thing, I'd still argue that we should
> >>>> ensure vsnprintf returns non-negative; e.g. by changing the "return
> >>>> PTR_ERR()" to a "goto out", i.e. simply stop the processing of the
> >>>> format string at the %pD which failed, but still go through the epilogue
> >>>> that ensures the resulting string becomes nul-terminated (another
> >>>> reasonable assumption made by tons of callers), and return how much got
> >>>> printed till then.
> >>
> >> So, how can we fix the callers without the above noted problems?
> >>
> >
> > The assumption that vsnprintf() is used to print to the console and that
> > writing some arbitrary string to the buffer is allowable is utterly wrong.
> >
> > vsnprintf_internal() is used to implement snprintf(). snprintf() is used
> > in numerous places where it will not lead to console output.
> >
> > Trying to solve one problem this patch creates a bunch of new ones.
>
> Heinrich, you do realize that the error handling you added in 256060e
> when you made it possible for vsnprintf() to return something negative
> is broken and incomplete? In multiple ways, even.
>
> First, let's look at vscnprint, which wasn't touched by 256060e.
>
> int vscnprintf(char *buf, size_t size, const char *fmt, va_list args)
> {
> int i;
>
> i = vsnprintf(buf, size, fmt, args);
>
> if (likely(i < size))
> return i;
>
> Integer promotion says that, should i be -ENOMEM or some other random
> -Esomething, that comparison is false (for any realistic value of the
> size parameter), so we won't actually pass on that negative value.
> Instead, we'll fall through to the logic that handles "oh, vsnprintf()
> didn't have enough room, so the length of the generated string, which is
> what I'm supposed to return, is size-1".
>
> Hence printf(), which uses vscnprintf(), would in that case receive
> CONFIG_SYS_PBSIZE-1 as result, and then it would go on to puts() the
> printbuffer[] - which isn't a nul-terminated string because you did that
> early return, so it has stack garbage.
>
> Let us look at printf() in more detail. Assuming vscnprintf() forwarded
> a negative value directly, do you see the problem here:
>
> uint i;
>
> i = vscnprintf(printbuffer, sizeof(printbuffer), fmt, args);
>
> /* Handle error */
> if (i <= 0)
> return i;
>
> So even if vscnprintf() was updated, printf() _still_ would go on to
> pass stack garbage to puts().
>
> ====
>
> Let me re-iterate what I believe any vsnprintf-implementation (and its
> close wrappers [v]s[c][n]printf) in a kernel or bootloader context must
> satisfy:
>
> - never return a negative value
> - given a non-zero size argument, must guarantee that the output buffer
> is a proper nul-terminated string.
>
> Without those guarantees, bugs like those described above will creep in,
> even if somebody fixes the issues I just pointed out. I'm not gonna send
> band-aid patches which will just propagate the problems to all other users.
>
> Not only do these guarantees make it easier to use the sprintf family,
> it also avoids lots of code bloat from unnecessary 'ret < 0' checks. It
> really has nothing to do with whether the output is destined for the
> console, _all_ users (direct or through several layers of helpers) of
> the sprintf family benefit from an implementation that provides these
> guarantees.
Gosh what a lot of stuff to read. Thank you both for all the analysis.
I feel that s...printf() should never return an error code, so I agree
with Rasmus, I suppose.
EFI's oddity here is just going to have to be EFI's problem. I am not
becoming more sympathetic to the EFI-ification of firmware, nor does
this sort of corner case progress the cause
If we need to return an error somehow, then I suggest:
- create a new function, e.g. vsprintf_err()
- carry around a flag in vsprintf.c to indicate the behaviour on error
- put support for the flag behind a CONFIG to avoid code-size bloat
Reviewed-by: Simon Glass <s...@chromium.org>
(but please update docs for pointer() to explain args and return value)
Regards,
Simon
