| Message ID | 20210724151411.9531-2-len.baker@gmx.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Remove all strcpy() uses | expand |
On Sat, Jul 24, 2021 at 7:05 PM Len Baker <len.baker@gmx.com> wrote: > > 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. ... > - 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. ... > + 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... ... > + par->init_sequence[i], j - i - 1, ...and get rid of the ' - i -1' part here.
Hi, On Sat, Jul 24, 2021 at 11:21:04PM +0300, Andy Shevchenko wrote: > On Sat, Jul 24, 2021 at 7:05 PM Len Baker <len.baker@gmx.com> wrote: > > > > 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. > > ... > > > - 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. > > ... > > > + 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... > > ... > > > + 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
On Sun, Jul 25, 2021 at 4:59 PM Len Baker <len.baker@gmx.com> wrote: > On Sat, Jul 24, 2021 at 11:21:04PM +0300, Andy Shevchenko wrote: > > On Sat, Jul 24, 2021 at 7:05 PM Len Baker <len.baker@gmx.com> wrote: ... > > > - 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. ... > > > + 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... > > > > ... > > > > > + 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? 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?
On Sun, Jul 25, 2021 at 09:51:18PM +0300, Andy Shevchenko wrote: > On Sun, Jul 25, 2021 at 4:59 PM Len Baker <len.baker@gmx.com> wrote: > > On Sat, Jul 24, 2021 at 11:21:04PM +0300, Andy Shevchenko wrote: > > > On Sat, Jul 24, 2021 at 7:05 PM Len Baker <len.baker@gmx.com> wrote: > > ... > > > > > - 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. Ok, I will update the commit changelog for the next version. > > ... > > > > > + 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... > > > > > > ... > > > > > > > + 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? > > I prefer my variant and I believe the compilers nowadays are clever > enough to understand this. Ok, understood. Thanks. > Have you tried to compile and compare the real assembly? I will test it. > -- > With Best Regards, > Andy Shevchenko Regards, Len
diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c index 3723269890d5..be20da3c4a5c 100644 --- a/drivers/staging/fbtft/fbtft-core.c +++ b/drivers/staging/fbtft/fbtft-core.c @@ -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;
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. Signed-off-by: Len Baker <len.baker@gmx.com> --- drivers/staging/fbtft/fbtft-core.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) -- 2.25.1