Received-SPF: pass (google.com: domain of linux-kernel+bounces-154117-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 22 Apr 2024 22:49:29 +0000
From: Justin Stitt <justinstitt@google.com>
To: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Jason Wessel <jason.wessel@windriver.com>, 
	Douglas Anderson <dianders@chromium.org>, kgdb-bugreport@lists.sourceforge.net, 
	linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH v2 0/7] kdb: Refactor and fix bugs in kdb_read()
Message-ID: <kvmf4hcnoeuogggx5jmcqjch32shyswjv5cqvg4hwdg4g27rup@t4ddszao3354>
References: <20240422-kgdb_read_refactor-v2-0-ed51f7d145fe@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240422-kgdb_read_refactor-v2-0-ed51f7d145fe@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