Remove all strcpy() uses
Subsystems (3)
v2 cover Remove all strcpy() uses
strcpy() performs no bounds checking on the destination buffer. This could result in linear overflows beyond the end of the buffer, leading to all kinds of misbehaviors. So, this serie removes all strcpy uses from the "staging/fbtft" subsystem.
Also, refactor the code a bit to follow the kernel coding-style and avoid unnecessary variable initialization.
Changelog v1 -> v2
- Add two new commits to clean the code.
- Use the "%*ph" format specifier instead of strscpy() function (Geert Uytterhoeven)
Len Baker (3):
staging/fbtft: Remove all strcpy() uses staging/fbtft: Remove unnecessary variable initialization staging/fbtft: Fix braces coding style
drivers/staging/fbtft/fbtft-core.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-)
--
2.25.1
PATCH v2 1/3 staging/fbtft: Remove all strcpy() uses
strcpy() performs no bounds checking on the destination buffer. This could result in linear overflows beyond the end of the buffer, leading to all kinds of misbehaviors. The safe replacement is strscpy() but in this case it is simpler to use the "%*ph" format specifier.
--- drivers/staging/fbtft/fbtft-core.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
drivers/staging/fbtft/fbtft-core.c +6 -11
@@ -992,8 +992,6 @@ static int fbtft_init_display_from_property(struct fbtft_par *par)int fbtft_init_display (struct fbtft_par *par) {int buf[64 ]; -char msg[128 ]; -char str[16 ];int i =0 ;int j;
@@ -1036,17 +1034,14 @@ int fbtft_init_display(struct fbtft_par *par)switch (par->init_sequence[i]) {case -1 : i++; +/* make debug message */ -strcpy (msg,"" ); - j = i +1 ; -while (par->init_sequence[j] >=0 ) { -sprintf (str,"0x%02X " , par->init_sequence[j]); -strcat (msg, str); - j++; - } +for (j = i +1 ; par->init_sequence[j] >=0 ; j++); +fbtft_par_dbg (DEBUG_INIT_DISPLAY , par, -"init: write(0x%02X) %s\n" , - par->init_sequence[i], msg); +"init: write(0x%02X) %*ph\n" , + par->init_sequence[i], j - i -1 , + &par->init_sequence[i +1 ]);/* Write */ j =0 ;
--
2.25.1
Re: [PATCH v2 1/3] staging/fbtft: Remove all strcpy() uses
Quoted 5 lines · Len Baker
strcpy()performs no bounds checking on the destination buffer. This could result in linear overflows beyond the end of the buffer, leading to all kinds of misbehaviors. The safe replacement isstrscpy()but in this case it is simpler to use the "%*ph" format specifier.
...
Quoted 1 line
- char msg[128];
128 / 4 = 32. So, this buffer is enough to debug print only up to 32 bytes. Hence %*ph replacement won't cut output earlier than requested.
...
Quoted 1 line
+ for (j = i + 1; par->init_sequence[j] >= 0; j++);
Why is i + 1 initial for the j? You may rather access the 'i + 1 + j'th element in the array...
...
Quoted 1 line
+ par->init_sequence[i], j - i - 1,
...and get rid of the ' - i -1' part here.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v2 1/3] staging/fbtft: Remove all strcpy() uses
Hi,
Quoted 7 lines · Andy Shevchenko
Quoted 5 lines · Len Baker
strcpy()performs no bounds checking on the destination buffer. This could result in linear overflows beyond the end of the buffer, leading to all kinds of misbehaviors. The safe replacement isstrscpy()but in this case it is simpler to use the "%*ph" format specifier.
Quoted 6 lines
...
Quoted 1 line
- char msg[128];
128 / 4 = 32. So, this buffer is enough to debug print only up to 32 bytes. Hence %*ph replacement won't cut output earlier than requested.
I'm sorry, but I don't understand what you are trying to explain. Moreover, with the "0x%02X " in the sprintf followed by the strcat, the msg buffer can print 128/5 values (25 hex values).
The %*ph replacement can print up to 64 bytes, so I don't see any problem here.
Quoted 5 lines
...
Quoted 1 line
+ for (j = i + 1; par->init_sequence[j] >= 0; j++);
Quoted 8 lines
Why is i + 1 initial for the j? You may rather access the 'i + 1 + j'th element in the array...
...
Quoted 1 line
+ par->init_sequence[i], j - i - 1,
...and get rid of the ' - i -1' part here.
Yes, it was the first idea but I prefer this method since we save aritmethic operations. In other words, if I use what you suggest, the index for par->init_sequence is calculated as a "sum" every iteration. But if the performance is not an issue and you believe that the above is more clear, I have no problem. What do you prefer?
Thanks,
Len
Re: [PATCH v2 1/3] staging/fbtft: Remove all strcpy() uses
Quoted 2 lines · Len Baker
Quoted 1 line · Andy Shevchenko
On Sat, Jul 24, 2021 at 7:05 PM Len Baker <[email protected]> wrote:
...
Quoted 11 lines
Quoted 4 lines
Quoted 1 line
- char msg[128];
128 / 4 = 32. So, this buffer is enough to debug print only up to 32 bytes. Hence %*ph replacement won't cut output earlier than requested.
I'm sorry, but I don't understand what you are trying to explain. Moreover, with the "0x%02X " in the sprintf followed by the strcat, the msg buffer can print 128/5 values (25 hex values).
The %*ph replacement can print up to 64 bytes, so I don't see any problem here.
Right. That's what I am trying to say and the hint here is to combine this part into a phrase in the commit message in the next version of the patch.
...
Quoted 9 lines
Quoted 9 lines
Quoted 1 line
+ for (j = i + 1; par->init_sequence[j] >= 0; j++);
Why is i + 1 initial for the j? You may rather access the 'i + 1 + j'th element in the array...
...
Quoted 1 line
+ par->init_sequence[i], j - i - 1,
Quoted 7 lines
Quoted 1 line
...and get rid of the ' - i -1' part here.
Yes, it was the first idea but I prefer this method since we save aritmethic operations. In other words, if I use what you suggest, the index for
par->init_sequenceis calculated as a "sum" every iteration. But if the performance is not an issue and you believe that the above is more clear, I have no problem. What do you prefer?
I prefer my variant and I believe the compilers nowadays are clever enough to understand this. Have you tried to compile and compare the real assembly?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v2 1/3] staging/fbtft: Remove all strcpy() uses
Quoted 15 lines · Andy Shevchenko
Quoted 2 lines · Len Baker
Quoted 1 line · Andy Shevchenko
On Sat, Jul 24, 2021 at 7:05 PM Len Baker <[email protected]> wrote:
...
Quoted 9 lines
Quoted 4 lines
Quoted 1 line
- char msg[128];
128 / 4 = 32. So, this buffer is enough to debug print only up to 32 bytes. Hence %*ph replacement won't cut output earlier than requested.
I'm sorry, but I don't understand what you are trying to explain. Moreover, with the "0x%02X " in the sprintf followed by the strcat, the msg buffer can print 128/5 values (25 hex values).
Quoted 6 lines
Quoted 2 lines
The %*ph replacement can print up to 64 bytes, so I don't see any problem here.
Right. That's what I am trying to say and the hint here is to combine this part into a phrase in the commit message in the next version of the patch.
Ok, I will update the commit changelog for the next version.
Quoted 14 lines
...
Quoted 11 lines
Quoted 10 lines
Quoted 1 line
+ for (j = i + 1; par->init_sequence[j] >= 0; j++);
Why is i + 1 initial for the j? You may rather access the 'i + 1 + j'th element in the array...
...
Quoted 1 line
+ par->init_sequence[i], j - i - 1,
...and get rid of the ' - i -1' part here.
Quoted 8 lines
Quoted 5 lines
Yes, it was the first idea but I prefer this method since we save aritmethic operations. In other words, if I use what you suggest, the index for
par->init_sequenceis calculated as a "sum" every iteration. But if the performance is not an issue and you believe that the above is more clear, I have no problem. What do you prefer?I prefer my variant and I believe the compilers nowadays are clever enough to understand this.
Ok, understood. Thanks.
Quoted 1 line
Have you tried to compile and compare the real assembly?
I will test it.
Quoted 3 lines
--
With Best Regards,
Andy Shevchenko
Regards,
Len
PATCH v2 2/3 staging/fbtft: Remove unnecessary variable initialization
In reply to Len BakerRemove the initialization of the variable "i" since it is written a few lines later.
--- drivers/staging/fbtft/fbtft-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/fbtft/fbtft-core.c +1 -1
@@ -992,7 +992,7 @@ static int fbtft_init_display_from_property(struct fbtft_par *par)int fbtft_init_display (struct fbtft_par *par) {int buf[64 ]; -int i =0 ; +int i;int j;/* sanity check */
--
2.25.1
PATCH v2 3/3 staging/fbtft: Fix braces coding style
In reply to Len BakerAdd braces to the "for" loop and remove braces from the "if" statement. This way the kernel coding style is followed.
--- drivers/staging/fbtft/fbtft-core.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
drivers/staging/fbtft/fbtft-core.c +6 -5
@@ -1003,9 +1003,11 @@ int fbtft_init_display(struct fbtft_par *par) }/* make sure stop marker exists */ -for (i =0 ; i <FBTFT_MAX_INIT_SEQUENCE ; i++) +for (i =0 ; i <FBTFT_MAX_INIT_SEQUENCE ; i++) {if (par->init_sequence[i] ==-3 )break ; + } +if (i ==FBTFT_MAX_INIT_SEQUENCE ) {dev_err (par->info->device,"missing stop marker at end of init sequence\n" );
@@ -1016,10 +1018,9 @@ int fbtft_init_display(struct fbtft_par *par) i =0 ;while (i <FBTFT_MAX_INIT_SEQUENCE ) { -if (par->init_sequence[i] ==-3 ) { -/* done */ -return 0 ; - } +if (par->init_sequence[i] ==-3 ) +return 0 ;/* done */ +if (par->init_sequence[i] >=0 ) {dev_err (par->info->device,"missing delimiter at position %d\n" , i);
--
2.25.1
Re: [PATCH v2 3/3] staging/fbtft: Fix braces coding style
Hi Len,
Quoted 4 lines · Len Baker
Add braces to the "for" loop and remove braces from the "if" statement. This way the kernel coding style is followed.
Signed-off-by Len Baker <[email protected]>
Thanks for your patch!
drivers/staging/fbtft/fbtft-core.c int fbtft_init_display(struct fbtft_par *par) · line 1022
Quoted 11 lines
drivers/staging/fbtft/fbtft-core.c +0
-4@@ -1016,10 +1018,9 @@ int fbtft_init_display(struct fbtft_par *par) i =0 ;while (i <FBTFT_MAX_INIT_SEQUENCE ) { -if (par->init_sequence[i] ==-3 ) { -/* done */ -return 0 ; - }
These braces should not be removed, due to the presence of the comment.
drivers/staging/fbtft/fbtft-core.c int fbtft_init_display(struct fbtft_par *par) · line 1026
Quoted 6 lines
drivers/staging/fbtft/fbtft-core.c +3
-0+if (par->init_sequence[i] ==-3 ) +return 0 ;/* done */ +if (par->init_sequence[i] >=0 ) {dev_err (par->info->device,"missing delimiter at position %d\n" , i);
Gr{oetje,eeting}s,
Geert
Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected] In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH v2 3/3] staging/fbtft: Fix braces coding style
Hi,
Quoted 15 lines · Geert Uytterhoeven
Hi Len,
Quoted 4 lines · Len Baker
Add braces to the "for" loop and remove braces from the "if" statement. This way the kernel coding style is followed.
Signed-off-by Len Baker <[email protected]>Thanks for your patch!
Quoted 5 lines
drivers/staging/fbtft/fbtft-core.c +0
-0@@ -1016,10 +1018,9 @@ int fbtft_init_display(struct fbtft_par *par)
Quoted 9 lines
drivers/staging/fbtft/fbtft-core.c int fbtft_init_display(struct fbtft_par *par) · line 1022
Quoted 6 lines
drivers/staging/fbtft/fbtft-core.c +0
-4i =0 ;while (i <FBTFT_MAX_INIT_SEQUENCE ) { -if (par->init_sequence[i] ==-3 ) { -/* done */ -return 0 ; - }These braces should not be removed, due to the presence of the comment.
Ok, I leave it as is.
Thanks,
Len