lib: sbi: Fix sbi_snprintf

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 <[email protected]>

---

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(-)

lib/sbi/sbi_console.c +0 -0

index 34c843d3f9e3..e300d710b5c8 100644
@@ -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,

lib/sbi/sbi_console.c +1 -1

@@ -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;

lib/sbi/sbi_console.c +1 -3

@@ -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;
?}

lib/sbi/sbi_console.c +0 -0

@@ -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);

lib/sbi/sbi_console.c +0 -0

@@ -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);

Signature · 1 line

2.36.1

? 2022-07-27???? 14:12 +0200?Andrew Jones???

Quoted 46 lines

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 <[email protected]>

---

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(-)

lib/sbi/sbi_console.c +0 -0

index 34c843d3f9e3..e300d710b5c8 100644
@@ -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); -???????????????????????}
-???????????????}

Quoted 8 lines

-???????} 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.

Quoted 57 lines

? 2022-07-27???? 14:12 +0200?Andrew Jones???

Quoted 46 lines

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 <[email protected]>

---

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(-)

lib/sbi/sbi_console.c +0 -0

index 34c843d3f9e3..e300d710b5c8 100644
@@ -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); -???????????????????????}
-???????????????}

Quoted 8 lines

-???????} 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.

Quoted 4 lines

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