Messages in this thread | | | From | Doug Anderson <> | Date | Thu, 7 May 2020 16:13:41 -0700 | Subject | Re: [PATCH] kdb: Eliminate strncpy() warnings by replacing with strscpy() |
| |
Hi,
On Tue, Mar 3, 2020 at 12:52 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Thu, Feb 13, 2020 at 7:06 AM Daniel Thompson > <daniel.thompson@linaro.org> wrote: > > > > Currently the code to manage the kdb history buffer uses strncpy() to > > copy strings to/and from the history and exhibits the classic "but > > nobody ever told me that strncpy() doesn't always terminate strings" > > bug. Modern gcc compilers recognise this bug and issue a warning. > > > > In reality these calls will only abridge the copied string if kdb_read() > > has *already* overflowed the command buffer. Thus the use of counted > > copies here is only used to reduce the secondary effects of a bug > > elsewhere in the code. > > > > Therefore transitioning these calls into strscpy() (without checking > > the return code) is appropriate. > > > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> > > --- > > kernel/debug/kdb/kdb_main.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > > index ba12e9f4661e..a4641be4123c 100644 > > --- a/kernel/debug/kdb/kdb_main.c > > +++ b/kernel/debug/kdb/kdb_main.c > > @@ -1102,12 +1102,12 @@ static int handle_ctrl_cmd(char *cmd) > > case CTRL_P: > > if (cmdptr != cmd_tail) > > cmdptr = (cmdptr-1) % KDB_CMD_HISTORY_COUNT; > > The above line (not touched by your patch) is slightly worrying to me. > I always have it in mind that "%" of numbers that might be negative > isn't an amazingly good idea. Some searches say that this must be > true: > > a == (a / b * b) + a % b > > ...which makes it feel like this is totally broken because "cmdptr" > will end up as -1. Huh? > > OK, after much digging and some printouts, I figured this out. cmdptr > is _unsigned_ and KDB_CMD_HISTORY_COUNT is a power of 2 (it's 32) > which makes this work. AKA if you start out at 0 and subtract 1, you > get 0xffffffff and then that "% 32" is 31 which is the answer that was > desired. Totally non-obvious. > > I guess a future change should make the above: > > cmdptr = (cmdptr + KDB_CMD_HISTORY_COUNT - 1) % > KDB_CMD_HISTORY_COUNT;
This has been sitting in the back of my mind for a while. Finally posted:
https://lore.kernel.org/r/20200507161125.1.I2cce9ac66e141230c3644b8174b6c15d4e769232@changeid
-Doug
| |