lib: sbi: Fix sbi_snprintf
PATCH 1/1 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. Finally, ensure all writes to 'out' go through printc and rename a goto label which clashed with it.
--- lib/sbi/sbi_console.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-)
lib/sbi/sbi_console.c +20 -15
@@ -76,20 +76,24 @@ 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) +return ; + +if (!out_len || *out_len >1 ) + **out = ch; +else if (*out_len ==1 ) + **out ='\0' ; + +if (out_len && *out_len >0 ) + --(*out_len); + +if (!out_len || *out_len >0 ) + ++(*out); } static int prints(char **out, u32 *out_len, const char *string, int width,
@@ -193,7 +197,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 +336,14 @@ 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' ; +printc (out, out_len,'\0' );return pc; }
Signature · 1 line
2.36.1
PATCH 1/1 lib: sbi: Fix sbi_snprintf
Quoted 31 lines · 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. Finally, ensure all writes to 'out' go through printc and rename a goto label which clashed with it.Fixes 9e8ff05cb61f ("Initial commit.")Signed-off-by Andrew Jones <[email protected]>--- lib/sbi/sbi_console.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-)lib/sbi/sbi_console.c +0
-9@@ -76,20 +76,24 @@ 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);
lib/sbi/sbi_console.c typedef __builtin_va_list va_list; · line 85
Quoted 10 lines
lib/sbi/sbi_console.c +5
-3- } - } - }else { +if (!out) {sbi_putc (ch); +return ; } + +if (!*out) +return ;
What if the output buffer is filled with zeros ?
Quoted 44 lines
lib/sbi/sbi_console.c +15
-4+ +if (!out_len || *out_len >1 ) + **out = ch; +else if (*out_len ==1 ) + **out ='\0' ; + +if (out_len && *out_len >0 ) + --(*out_len); + +if (!out_len || *out_len >0 ) + ++(*out); } static int prints(char **out, u32 *out_len, const char *string, int width,@@ -193,7 +197,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 +336,14 @@ 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' ; +printc (out, out_len,'\0' );return pc; } --2.36.1
Quoted 4 lines
--
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
Regards,
Anup
PATCH 1/1 lib: sbi: Fix sbi_snprintf
Quoted 36 lines · Anup Patel
Quoted 35 lines · 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. Finally, ensure all writes to 'out' go through printc and rename a goto label which clashed with it.Fixes 9e8ff05cb61f ("Initial commit.")Signed-off-by Andrew Jones <[email protected]>--- lib/sbi/sbi_console.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-)lib/sbi/sbi_console.c +1
-12@@ -76,20 +76,24 @@ 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) {
Quoted 8 lines
lib/sbi/sbi_console.c typedef __builtin_va_list va_list; · line 85
Quoted 6 lines
lib/sbi/sbi_console.c +4
-0sbi_putc (ch); +return ; } + +if (!*out) +return ;What if the output buffer is filled with zeros ?
This single dereferencing of 'out' won't reach the buffer. This check ensures we don't dereference NULL if sprintf/snprintf are called with str=NULL and is just a rewording of the original code. Actually, we probably shouldn't allow str=NULL for sprintf nor for snprintf with size != 0 at all. We could hang in sprintf and snprintf if we detect that.
Thanks,
drew
PATCH 1/1 lib: sbi: Fix sbi_snprintf
In reply to Andrew Jones? 2022-07-26???? 13:25 +0200?Andrew Jones???
Quoted 41 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. Finally, ensure all writes to 'out' go through printc and rename a goto label which clashed with it.Fixes 9e8ff05cb61f ("Initial commit.")Signed-off-by Andrew Jones <[email protected]>---
?lib/sbi/sbi_console.c | 35 ++++++++++++++++++++--------------- ?1 file changed, 20 insertions(+), 15 deletions(-)lib/sbi/sbi_console.c +0
-0@@ -76,20 +76,24 @@ typedef __builtin_va_list va_list;?
?static voidprintc(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;
????????}lib/sbi/sbi_console.c +4
-0+ +???????if (!*out) +???????????????return; +
lib/sbi/sbi_console.c
Quoted 10 lines
lib/sbi/sbi_console.c +10
-0+???????if (!out_len || *out_len > 1) +???????????????**out = ch; +???????else if (*out_len == 1) +???????????????**out = '\0'; + +???????if (out_len && *out_len > 0) +???????????????--(*out_len); + +???????if (!out_len || *out_len > 0) +???????????????++(*out);
Too many branches
static void printc(char **out, u32 *out_len, char ch)
{
if (out && *out && out_len) {
if (*out_len > 0) {
if (*out_len > 1) {
(*out)[0] = ch;
(*out)[1] = '\0';
}
(*out)++;
(*out_len)--;
} else
// add error message print
} else {
sbi_putc(ch);
}
}Quoted 16 lines
?}
?
?static int prints(char **out, u32*out_len, const char *string, int width,lib/sbi/sbi_console.c +1
-1@@ -193,7 +197,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 +0
-0@@ -332,13 +336,14 @@ static int print(char **out, u32 *out_len, const char *format, va_list args) ????????????????????????????????continue; ????????????????????????} ????????????????} else {
lib/sbi/sbi_console.c static int print(char **out, u32 *out_len, const char *format, va_list args) · line 346
Quoted 10 lines
lib/sbi/sbi_console.c +3
-2-???????????????out: +literal: ????????????????????????printc(out, out_len, *format); ????????????????????????++pc; ????????????????} ????????} + ????????if (out) -???????????????**out = '\0'; +???????????????printc(out, out_len, '\0');
this is not necessary
Quoted 7 lines
?
????????return pc;
?}Signature · 1 line
2.36.1
Regards,
Xiang W
PATCH 1/1 lib: sbi: Fix sbi_snprintf
Quoted 54 lines · Xiang W
? 2022-07-26???? 13:25 +0200?Andrew Jones???
Quoted 41 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. Finally, ensure all writes to 'out' go through printc and rename a goto label which clashed with it.Fixes 9e8ff05cb61f ("Initial commit.")Signed-off-by Andrew Jones <[email protected]>---
?lib/sbi/sbi_console.c | 35 ++++++++++++++++++++--------------- ?1 file changed, 20 insertions(+), 15 deletions(-)lib/sbi/sbi_console.c +0
-0@@ -76,20 +76,24 @@ typedef __builtin_va_list va_list;?
?static voidprintc(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;
????????}lib/sbi/sbi_console.c +4
-0+ +???????if (!*out) +???????????????return; +lib/sbi/sbi_console.c
Quoted 10 lines
lib/sbi/sbi_console.c +10
-0+???????if (!out_len || *out_len > 1) +???????????????**out = ch; +???????else if (*out_len == 1) +???????????????**out = '\0'; + +???????if (out_len && *out_len > 0) +???????????????--(*out_len); + +???????if (!out_len || *out_len > 0) +???????????????++(*out);Too many branches
Quoted 3 lines
static void
printc(char **out, u32 *out_len, char ch){ if (out && *out && out_len) {
It's possible to have non-NULL out/*out and NULL out_len (when sprintf is used), so we need at least one other branch.
Quoted 4 lines
if (*out_len > 0) {
if (*out_len > 1) {
(*out)[0] = ch;
(*out)[1] = '\0';Sure. It saves a branch and allows us to drop the extra if (out) branch in print() below which you pointed out.
Quoted 29 lines
} (*out)++; (*out_len)--; } else // add error message print } else { sbi_putc(ch); } }Quoted 19 lines
?}
?
?static int prints(char **out, u32*out_len, const char *string, int width,lib/sbi/sbi_console.c +1
-1@@ -193,7 +197,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
-1@@ -332,13 +336,14 @@ static int print(char **out, u32 *out_len, const char *format, va_list args) ????????????????????????????????continue; ????????????????????????} ????????????????} else { -???????????????out: +literal: ????????????????????????printc(out, out_len, *format);
Quoted 8 lines
lib/sbi/sbi_console.c static int print(char **out, u32 *out_len, const char *format, va_list args) · line 346
Quoted 7 lines
lib/sbi/sbi_console.c +2
-1????????????????????????++pc; ????????????????} ????????} + ????????if (out) -???????????????**out = '\0'; +???????????????printc(out, out_len, '\0');this is not necessary
I'll send a v2 with the "always write '\0'" approach integrated.
Thanks,
drew