Inspired by a patch from [Justin][1] I took a closer look at kdb_read().

Despite Justin's patch being a (correct) one-line manipulation it was a
tough patch to review because the surrounding code was hard to read and
it looked like there were unfixed problems.

This series isn't enough to make kdb_read() beautiful but it does make
it shorter, easier to reason about and fixes two buffer overflows and a
screen redraw problem!

[1]: https://lore.kernel.org/all/20240403-strncpy-kernel-debug-kdb-kdb_io-c-v1-1-7f78a08e9ff4@google.com/

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
Changes in v2:
- No code changes!
- I belatedly realized that one of the cleanups actually fixed a buffer
  overflow so there are changes to Cc: (to add stable@...) and to one
  of the patch descriptions.
- Link to v1: https://lore.kernel.org/r/20240416-kgdb_read_refactor-v1-0-b18c2d01076d@linaro.org

---
Daniel Thompson (7):
      kdb: Fix buffer overflow during tab-complete
      kdb: Use format-strings rather than '\0' injection in kdb_read()
      kdb: Fix console handling when editing and tab-completing commands
      kdb: Merge identical case statements in kdb_read()
      kdb: Use format-specifiers rather than memset() for padding in kdb_read()
      kdb: Replace double memcpy() with memmove() in kdb_read()
      kdb: Simplify management of tmpbuffer in kdb_read()

 kernel/debug/kdb/kdb_io.c | 133 ++++++++++++++++++++--------------------------
 1 file changed, 58 insertions(+), 75 deletions(-)
---
base-commit: dccce9b8780618986962ba37c373668bcf426866
change-id: 20240415-kgdb_read_refactor-2ea2dfc15dbb

Best regards,
-- 
Daniel Thompson <daniel.thompson@linaro.org>

Hi,

On Mon, Apr 22, 2024 at 05:35:53PM +0100, Daniel Thompson wrote:
> Inspired by a patch from [Justin][1] I took a closer look at kdb_read().
> 
> Despite Justin's patch being a (correct) one-line manipulation it was a
> tough patch to review because the surrounding code was hard to read and
> it looked like there were unfixed problems.
> 
> This series isn't enough to make kdb_read() beautiful but it does make
> it shorter, easier to reason about and fixes two buffer overflows and a
> screen redraw problem!
> 
> [1]: https://lore.kernel.org/all/20240403-strncpy-kernel-debug-kdb-kdb_io-c-v1-1-7f78a08e9ff4@google.com/
> 
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>

Seems to work nicely.

There is some weird behavior which was present before your patch and is
still present with it (let >< represent cursor position):

[0]kdb> test_ap>< (now press TAB)

[0]kdb> test_aperfmperf>< (so far so good, we got our autocomplete)

[0]kdb> test_ap><erfmperf (now, let's move the cursor back and press TAB again)

[0]kdb> test_aperfmperf><erfmperf

This is because the autocomplete engine is not considering the
characters after the cursor position. To be clear, this isn't really a
bug but rather a decision to be made about which functionality is
desired.

For example, my shell (zsh) will just simply move the cursor back to
the end of the complete match instead of re-writing stuff.

At any rate,
Tested-by: Justin Stitt <justinstitt@google.com>

> ---
> Changes in v2:
> - No code changes!
> - I belatedly realized that one of the cleanups actually fixed a buffer
>   overflow so there are changes to Cc: (to add stable@...) and to one
>   of the patch descriptions.
> - Link to v1: https://lore.kernel.org/r/20240416-kgdb_read_refactor-v1-0-b18c2d01076d@linaro.org
> 
> ---
> Daniel Thompson (7):
>       kdb: Fix buffer overflow during tab-complete
>       kdb: Use format-strings rather than '\0' injection in kdb_read()
>       kdb: Fix console handling when editing and tab-completing commands
>       kdb: Merge identical case statements in kdb_read()
>       kdb: Use format-specifiers rather than memset() for padding in kdb_read()
>       kdb: Replace double memcpy() with memmove() in kdb_read()
>       kdb: Simplify management of tmpbuffer in kdb_read()
> 
>  kernel/debug/kdb/kdb_io.c | 133 ++++++++++++++++++++--------------------------
>  1 file changed, 58 insertions(+), 75 deletions(-)
> ---
> base-commit: dccce9b8780618986962ba37c373668bcf426866
> change-id: 20240415-kgdb_read_refactor-2ea2dfc15dbb
> 
> Best regards,
> -- 
> Daniel Thompson <daniel.thompson@linaro.org>
> 

Thanks
Justin

On Mon, Apr 22, 2024 at 10:49:29PM +0000, Justin Stitt wrote:
> Hi,
>
> On Mon, Apr 22, 2024 at 05:35:53PM +0100, Daniel Thompson wrote:
> > Inspired by a patch from [Justin][1] I took a closer look at kdb_read().
> >
> > Despite Justin's patch being a (correct) one-line manipulation it was a
> > tough patch to review because the surrounding code was hard to read and
> > it looked like there were unfixed problems.
> >
> > This series isn't enough to make kdb_read() beautiful but it does make
> > it shorter, easier to reason about and fixes two buffer overflows and a
> > screen redraw problem!
> >
> > [1]: https://lore.kernel.org/all/20240403-strncpy-kernel-debug-kdb-kdb_io-c-v1-1-7f78a08e9ff4@google.com/
> >
> > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
>
> Seems to work nicely.
>
> There is some weird behavior which was present before your patch and is
> still present with it (let >< represent cursor position):
>
> [0]kdb> test_ap>< (now press TAB)
>
> [0]kdb> test_aperfmperf>< (so far so good, we got our autocomplete)
>
> [0]kdb> test_ap><erfmperf (now, let's move the cursor back and press TAB again)
>
> [0]kdb> test_aperfmperf><erfmperf
>
> This is because the autocomplete engine is not considering the
> characters after the cursor position. To be clear, this isn't really a
> bug but rather a decision to be made about which functionality is
> desired.
>
> For example, my shell (zsh) will just simply move the cursor back to
> the end of the complete match instead of re-writing stuff.

Interesting observation. I hadn't realized zsh does that. FWIW default
settings for both bash and gdb complete the same way as kdb. Overall
that makes me OK with the current kdb behaviour.

However I was curious about this and found "skip-completed-text" in
the GNU Readline documentation. I think that would give you zsh-like
completion in gdb if you ever want it!


> At any rate,
> Tested-by: Justin Stitt <justinstitt@google.com>

Thanks.


Daniel.