| Message ID | 20220727121201.11508-1-ajones@ventanamicro.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] lib: sbi: Fix sbi_snprintf | expand |
在 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@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. Regards, Xiang W > + **out = '\0'; > + } > + > + if (out_len && *out_len > 0) > + --(*out_len); > } > > static int prints(char **out, u32 *out_len, const char *string, int width, > @@ -193,7 +191,7 @@ static int print(char **out, u32 *out_len, const char *format, va_list args) > if (*format == '\0') > break; > if (*format == '%') > - goto out; > + goto literal; > /* Get flags */ > if (*format == '-') { > ++format; > @@ -332,13 +330,11 @@ static int print(char **out, u32 *out_len, const char *format, va_list args) > continue; > } > } else { > - out: > +literal: > printc(out, out_len, *format); > ++pc; > } > } > - if (out) > - **out = '\0'; > > return pc; > } > @@ -348,6 +344,9 @@ int sbi_sprintf(char *out, const char *format, ...) > va_list args; > int retval; > > + if (unlikely(!out)) > + sbi_panic("sbi_sprintf called with NULL output string\n"); > + > va_start(args, format); > retval = print(&out, NULL, format, args); > va_end(args); > @@ -360,6 +359,10 @@ int sbi_snprintf(char *out, u32 out_sz, const char *format, ...) > va_list args; > int retval; > > + if (unlikely(!out && out_sz != 0)) > + sbi_panic("sbi_snprintf called with NULL output string and " > + "output size is not zero\n"); > + > va_start(args, format); > retval = print(&out, &out_sz, format, args); > va_end(args); > -- > 2.36.1 > >
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@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
在 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@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
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; + **out = '\0'; + } + + if (out_len && *out_len > 0) + --(*out_len); } static int prints(char **out, u32 *out_len, const char *string, int width, @@ -193,7 +191,7 @@ static int print(char **out, u32 *out_len, const char *format, va_list args) if (*format == '\0') break; if (*format == '%') - goto out; + goto literal; /* Get flags */ if (*format == '-') { ++format; @@ -332,13 +330,11 @@ static int print(char **out, u32 *out_len, const char *format, va_list args) continue; } } else { - out: +literal: printc(out, out_len, *format); ++pc; } } - if (out) - **out = '\0'; return pc; } @@ -348,6 +344,9 @@ int sbi_sprintf(char *out, const char *format, ...) va_list args; int retval; + if (unlikely(!out)) + sbi_panic("sbi_sprintf called with NULL output string\n"); + va_start(args, format); retval = print(&out, NULL, format, args); va_end(args); @@ -360,6 +359,10 @@ int sbi_snprintf(char *out, u32 out_sz, const char *format, ...) va_list args; int retval; + if (unlikely(!out && out_sz != 0)) + sbi_panic("sbi_snprintf called with NULL output string and " + "output size is not zero\n"); + va_start(args, format); retval = print(&out, &out_sz, format, args); va_end(args);
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@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(-)