[PATCH v2] lib: sbi: Fix sbi_snprintf

[Xiang W]

在 2022-07-27星期三的 15:45 +0200，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.

Yes, I agree to add a comment to clarify

Regards,
Xiang W
> 
> Thanks,
> drew