[PATCH v2] lib: sbi: Fix sbi_snprintf

[Andrew Jones]

On Wed, Jul 27, 2022 at 09:27:52PM +0800, Xiang W wrote:
> 在 2022-07-27星期三的 14:12 +0200，Andrew Jones写道：
> > printc would happily write to 'out' even when 'out_len' was zero,
> > potentially overflowing buffers. Rework printc to not do that and
> > also ensure the null byte is written at the last position when
> > necessary, as stated in the snprintf man page. Also, panic if
> > sprintf or snprintf are called with NULL output strings (except
> > the special case of snprintf having a NULL output string and
> > a zero output size, allowing it to be used to get the number of
> > characters that would have been written). Finally, rename a
> > goto label which clashed with 'out'.
> > 
> > Fixes: 9e8ff05cb61f ("Initial commit.")
> > Signed-off-by: Andrew Jones <ajones at ventanamicro.com>
> > ---
> > 
> > v2:
> >   - Simply forbid *out == NULL by panicing when it's detected.
> >     (The error message for snprintf has been split over two
> >      lines to avoid going over 80 chars. I'd prefer error messages
> >      not be split, but that seems like the general practice for
> >      opensbi.)
> >   - Drop some branches, particularly the extra 'if (out)' in print(),
> >     by always writing a '\0' on each printc [Xiang W]
> > 
> > 
> >  lib/sbi/sbi_console.c | 35 +++++++++++++++++++----------------
> >  1 file changed, 19 insertions(+), 16 deletions(-)
> > 
> > diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
> > index 34c843d3f9e3..e300d710b5c8 100644
> > --- a/lib/sbi/sbi_console.c
> > +++ b/lib/sbi/sbi_console.c
> > @@ -76,20 +76,18 @@ typedef __builtin_va_list va_list;
> >  
> >  static void printc(char **out, u32 *out_len, char ch)
> >  {
> > -       if (out) {
> > -               if (*out) {
> > -                       if (out_len && (0 < *out_len)) {
> > -                               **out = ch;
> > -                               ++(*out);
> > -                               (*out_len)--;
> > -                       } else {
> > -                               **out = ch;
> > -                               ++(*out);
> > -                       }
> > -               }
> > -       } else {
> > +       if (!out) {
> >                 sbi_putc(ch);
> > +               return;
> >         }
> > +
> > +       if (!out_len || *out_len > 1) {
> > +               *(*out)++ = ch;
> Before this, it was not determined that *out was not NULL.
> 
> Argument checking before calling the function is not a substitute
> for checking the function itself, the function should do what it
> has to do. Otherwise, others need more understanding work during
> secondary development.

I disagree. Argument checking in private helper functions is overly
defensive programming. Indeed, helper functions should expect the
entry points of its module to do the argument checking. It is
reasonable to add a comment pointing out that we know *out can't be
NULL here, though, as printc is the only place we do the dereferencing.

Thanks,
drew