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-...
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@lina...
--- 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,
Currently, when the user attempts symbol completion with the Tab key, kdb will use strncpy() to insert the completed symbol into the command buffer. Unfortunately it passes the size of the source buffer rather than the destination to strncpy() with predictably horrible results. Most obviously if the command buffer is already full but cp, the cursor position, is in the middle of the buffer, then we will write past the end of the supplied buffer.
Fix this by replacing the dubious strncpy() calls with memmove()/memcpy() calls plus explicit boundary checks to make sure we have enough space before we start moving characters around.
Reported-by: Justin Stitt justinstitt@google.com Closes: https://lore.kernel.org/all/CAFhGd8qESuuifuHsNjFPR-Va3P80bxrw+LqvC8deA8GziUJ... Cc: stable@vger.kernel.org Signed-off-by: Daniel Thompson daniel.thompson@linaro.org --- kernel/debug/kdb/kdb_io.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index 9443bc63c5a24..06dfbccb10336 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -367,14 +367,19 @@ static char *kdb_read(char *buffer, size_t bufsize) kdb_printf(kdb_prompt_str); kdb_printf("%s", buffer); } else if (tab != 2 && count > 0) { - len_tmp = strlen(p_tmp); - strncpy(p_tmp+len_tmp, cp, lastchar-cp+1); - len_tmp = strlen(p_tmp); - strncpy(cp, p_tmp+len, len_tmp-len + 1); - len = len_tmp - len; - kdb_printf("%s", cp); - cp += len; - lastchar += len; + /* How many new characters do we want from tmpbuffer? */ + len_tmp = strlen(p_tmp) - len; + if (lastchar + len_tmp >= bufend) + len_tmp = bufend - lastchar; + + if (len_tmp) { + /* + 1 ensures the '\0' is memmove'd */ + memmove(cp+len_tmp, cp, (lastchar-cp) + 1); + memcpy(cp, p_tmp+len, len_tmp); + kdb_printf("%s", cp); + cp += len_tmp; + lastchar += len_tmp; + } } kdb_nextline = 1; /* reset output line number */ break;
Hi,
On Mon, Apr 22, 2024 at 05:35:54PM +0100, Daniel Thompson wrote:
Currently, when the user attempts symbol completion with the Tab key, kdb will use strncpy() to insert the completed symbol into the command buffer. Unfortunately it passes the size of the source buffer rather than the destination to strncpy() with predictably horrible results. Most obviously if the command buffer is already full but cp, the cursor position, is in the middle of the buffer, then we will write past the end of the supplied buffer.
Fix this by replacing the dubious strncpy() calls with memmove()/memcpy() calls plus explicit boundary checks to make sure we have enough space before we start moving characters around.
Reported-by: Justin Stitt justinstitt@google.com Closes: https://lore.kernel.org/all/CAFhGd8qESuuifuHsNjFPR-Va3P80bxrw+LqvC8deA8GziUJ... Cc: stable@vger.kernel.org Signed-off-by: Daniel Thompson daniel.thompson@linaro.org
Nice! This is better than the conversions I tried to make earlier.
Your patch helps with https://github.com/KSPP/linux/issues/90
Reviewed-by: Justin Stitt justinstitt@google.com
kernel/debug/kdb/kdb_io.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index 9443bc63c5a24..06dfbccb10336 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -367,14 +367,19 @@ static char *kdb_read(char *buffer, size_t bufsize) kdb_printf(kdb_prompt_str); kdb_printf("%s", buffer); } else if (tab != 2 && count > 0) {
len_tmp = strlen(p_tmp);strncpy(p_tmp+len_tmp, cp, lastchar-cp+1);len_tmp = strlen(p_tmp);strncpy(cp, p_tmp+len, len_tmp-len + 1);len = len_tmp - len;kdb_printf("%s", cp);cp += len;lastchar += len;
/* How many new characters do we want from tmpbuffer? */len_tmp = strlen(p_tmp) - len;if (lastchar + len_tmp >= bufend)len_tmp = bufend - lastchar;if (len_tmp) {/* + 1 ensures the '\0' is memmove'd */memmove(cp+len_tmp, cp, (lastchar-cp) + 1);memcpy(cp, p_tmp+len, len_tmp);kdb_printf("%s", cp);cp += len_tmp;lastchar += len_tmp;}-- 2.43.0
Thanks Justin
Hi,
On Mon, Apr 22, 2024 at 9:37 AM Daniel Thompson daniel.thompson@linaro.org wrote:
Currently, when the user attempts symbol completion with the Tab key, kdb will use strncpy() to insert the completed symbol into the command buffer. Unfortunately it passes the size of the source buffer rather than the destination to strncpy() with predictably horrible results. Most obviously if the command buffer is already full but cp, the cursor position, is in the middle of the buffer, then we will write past the end of the supplied buffer.
Fix this by replacing the dubious strncpy() calls with memmove()/memcpy() calls plus explicit boundary checks to make sure we have enough space before we start moving characters around.
Reported-by: Justin Stitt justinstitt@google.com Closes: https://lore.kernel.org/all/CAFhGd8qESuuifuHsNjFPR-Va3P80bxrw+LqvC8deA8GziUJ... Cc: stable@vger.kernel.org Signed-off-by: Daniel Thompson daniel.thompson@linaro.org
kernel/debug/kdb/kdb_io.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
Boy, this function (and especially the tab handling) is hard to read. I spent a bit of time trying to grok it and, as far as I can tell, your patch makes things better and I don't see any bugs.
Reviewed-by: Douglas Anderson dianders@chromium.org
On Mon, Apr 22, 2024 at 04:51:49PM -0700, Doug Anderson wrote:
Hi,
On Mon, Apr 22, 2024 at 9:37 AM Daniel Thompson daniel.thompson@linaro.org wrote:
Currently, when the user attempts symbol completion with the Tab key, kdb will use strncpy() to insert the completed symbol into the command buffer. Unfortunately it passes the size of the source buffer rather than the destination to strncpy() with predictably horrible results. Most obviously if the command buffer is already full but cp, the cursor position, is in the middle of the buffer, then we will write past the end of the supplied buffer.
Fix this by replacing the dubious strncpy() calls with memmove()/memcpy() calls plus explicit boundary checks to make sure we have enough space before we start moving characters around.
Reported-by: Justin Stitt justinstitt@google.com Closes: https://lore.kernel.org/all/CAFhGd8qESuuifuHsNjFPR-Va3P80bxrw+LqvC8deA8GziUJ... Cc: stable@vger.kernel.org Signed-off-by: Daniel Thompson daniel.thompson@linaro.org
kernel/debug/kdb/kdb_io.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
Boy, this function (and especially the tab handling) is hard to read.
Too right. I even rewrote it using offsets rather than pointers at one point (it didn't really make it much clearer so I dropped that for now).
I spent a bit of time trying to grok it and, as far as I can tell, your patch makes things better and I don't see any bugs.
Reviewed-by: Douglas Anderson dianders@chromium.org
Thanks!
Daniel.
Currently when kdb_read() needs to reposition the cursor it uses copy and paste code that works by injecting an '\0' at the cursor position before delivering a carriage-return and reprinting the line (which stops at the '\0').
Tidy up the code by hoisting the copy and paste code into an appropriately named function. Additionally let's replace the '\0' injection with a proper field width parameter so that the string will be abridged during formatting instead.
Cc: stable@vger.kernel.org # Not a bug fix but it is needed for later bug fixes Signed-off-by: Daniel Thompson daniel.thompson@linaro.org --- kernel/debug/kdb/kdb_io.c | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index 06dfbccb10336..a42607e4d1aba 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -184,6 +184,13 @@ char kdb_getchar(void) unreachable(); }
+static void kdb_position_cursor(char *prompt, char *buffer, char *cp) +{ + kdb_printf("\r%s", kdb_prompt_str); + if (cp > buffer) + kdb_printf("%.*s", (int)(cp - buffer), buffer); +} + /* * kdb_read * @@ -249,12 +256,8 @@ static char *kdb_read(char *buffer, size_t bufsize) } *(--lastchar) = '\0'; --cp; - kdb_printf("\b%s \r", cp); - tmp = *cp; - *cp = '\0'; - kdb_printf(kdb_prompt_str); - kdb_printf("%s", buffer); - *cp = tmp; + kdb_printf("\b%s ", cp); + kdb_position_cursor(kdb_prompt_str, buffer, cp); } break; case 10: /* linefeed */ @@ -272,19 +275,14 @@ static char *kdb_read(char *buffer, size_t bufsize) memcpy(tmpbuffer, cp+1, lastchar - cp - 1); memcpy(cp, tmpbuffer, lastchar - cp - 1); *(--lastchar) = '\0'; - kdb_printf("%s \r", cp); - tmp = *cp; - *cp = '\0'; - kdb_printf(kdb_prompt_str); - kdb_printf("%s", buffer); - *cp = tmp; + kdb_printf("%s ", cp); + kdb_position_cursor(kdb_prompt_str, buffer, cp); } break; case 1: /* Home */ if (cp > buffer) { - kdb_printf("\r"); - kdb_printf(kdb_prompt_str); cp = buffer; + kdb_position_cursor(kdb_prompt_str, buffer, cp); } break; case 5: /* End */ @@ -390,13 +388,9 @@ static char *kdb_read(char *buffer, size_t bufsize) memcpy(cp+1, tmpbuffer, lastchar - cp); *++lastchar = '\0'; *cp = key; - kdb_printf("%s\r", cp); + kdb_printf("%s", cp); ++cp; - tmp = *cp; - *cp = '\0'; - kdb_printf(kdb_prompt_str); - kdb_printf("%s", buffer); - *cp = tmp; + kdb_position_cursor(kdb_prompt_str, buffer, cp); } else { *++lastchar = '\0'; *cp++ = key;
Hi,
On Mon, Apr 22, 2024 at 9:37 AM Daniel Thompson daniel.thompson@linaro.org wrote:
Currently when kdb_read() needs to reposition the cursor it uses copy and paste code that works by injecting an '\0' at the cursor position before delivering a carriage-return and reprinting the line (which stops at the '\0').
Tidy up the code by hoisting the copy and paste code into an appropriately named function. Additionally let's replace the '\0' injection with a proper field width parameter so that the string will be abridged during formatting instead.
Cc: stable@vger.kernel.org # Not a bug fix but it is needed for later bug fixes Signed-off-by: Daniel Thompson daniel.thompson@linaro.org
kernel/debug/kdb/kdb_io.c | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-)
Looks like a nice fix, but I think this'll create a compile warning on some compilers. The variable "tmp" is no longer used, I think.
Once the "tmp" variable is deleted, feel free to add my Reviewed-by.
NOTE: patch #7 in your series re-adds a user of "tmp", but since this one is "Cc: stable" you will need to delete it here and then re-add it in patch #7.
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index 06dfbccb10336..a42607e4d1aba 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -184,6 +184,13 @@ char kdb_getchar(void) unreachable(); }
+static void kdb_position_cursor(char *prompt, char *buffer, char *cp) +{
kdb_printf("\r%s", kdb_prompt_str);if (cp > buffer)kdb_printf("%.*s", (int)(cp - buffer), buffer);
nit: personally, I'd take the "if" statement out. I'm nearly certain that kdb_printf() can handle zero-length for the width argument and "buffer" can never be _after_ cp (so you can't get negative).
-Doug
On Mon, Apr 22, 2024 at 04:52:04PM -0700, Doug Anderson wrote:
Hi,
On Mon, Apr 22, 2024 at 9:37 AM Daniel Thompson daniel.thompson@linaro.org wrote:
Currently when kdb_read() needs to reposition the cursor it uses copy and paste code that works by injecting an '\0' at the cursor position before delivering a carriage-return and reprinting the line (which stops at the '\0').
Tidy up the code by hoisting the copy and paste code into an appropriately named function. Additionally let's replace the '\0' injection with a proper field width parameter so that the string will be abridged during formatting instead.
Cc: stable@vger.kernel.org # Not a bug fix but it is needed for later bug fixes Signed-off-by: Daniel Thompson daniel.thompson@linaro.org
kernel/debug/kdb/kdb_io.c | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-)
Looks like a nice fix, but I think this'll create a compile warning on some compilers. The variable "tmp" is no longer used, I think.
Once the "tmp" variable is deleted, feel free to add my Reviewed-by.
Good spot. I'll fix that.
NOTE: patch #7 in your series re-adds a user of "tmp", but since this one is "Cc: stable" you will need to delete it here and then re-add it in patch #7.
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index 06dfbccb10336..a42607e4d1aba 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -184,6 +184,13 @@ char kdb_getchar(void) unreachable(); }
+static void kdb_position_cursor(char *prompt, char *buffer, char *cp) +{
kdb_printf("\r%s", kdb_prompt_str);if (cp > buffer)kdb_printf("%.*s", (int)(cp - buffer), buffer);nit: personally, I'd take the "if" statement out. I'm nearly certain that kdb_printf() can handle zero-length for the width argument and "buffer" can never be _after_ cp (so you can't get negative).
The kernel will correctly format zero-width fields... but kdb_printf() will also inject the empty string into the log if running with LOGGING=1. It is true the dmesg output with LOGGING=1 is pretty nasty when doing line editing but I still didn't want to make it worse!
Oh... and we can't combine into one call to kdb_printf() since that renders into a fixed length static buffer and we could get unwanted truncation. I might just add a comment about that.
Daniel.
Currently, if the cursor position is not at the end of the command buffer and the user uses the Tab-complete functions, then the console does not leave the cursor in the correct position.
For example consider the following buffer with the cursor positioned at the ^:
md kdb_pro 10 ^
Pressing tab should result in:
md kdb_prompt_str 10 ^
However this does not happen. Instead the cursor is placed at the end (after then 10) and further cursor movement redraws incorrectly. The same problem exists when we double-Tab but in a different part of the code.
Fix this by sending a carriage return and then redisplaying the text to the left of the cursor.
Cc: stable@vger.kernel.org Signed-off-by: Daniel Thompson daniel.thompson@linaro.org --- kernel/debug/kdb/kdb_io.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index a42607e4d1aba..69549fe42e87b 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -364,6 +364,8 @@ static char *kdb_read(char *buffer, size_t bufsize) kdb_printf("\n"); kdb_printf(kdb_prompt_str); kdb_printf("%s", buffer); + if (cp != lastchar) + kdb_position_cursor(kdb_prompt_str, buffer, cp); } else if (tab != 2 && count > 0) { /* How many new characters do we want from tmpbuffer? */ len_tmp = strlen(p_tmp) - len; @@ -377,6 +379,9 @@ static char *kdb_read(char *buffer, size_t bufsize) kdb_printf("%s", cp); cp += len_tmp; lastchar += len_tmp; + if (cp != lastchar) + kdb_position_cursor(kdb_prompt_str, + buffer, cp); } } kdb_nextline = 1; /* reset output line number */
Hi,
On Mon, Apr 22, 2024 at 9:37 AM Daniel Thompson daniel.thompson@linaro.org wrote:
Currently, if the cursor position is not at the end of the command buffer and the user uses the Tab-complete functions, then the console does not leave the cursor in the correct position.
For example consider the following buffer with the cursor positioned at the ^:
md kdb_pro 10 ^
Pressing tab should result in:
md kdb_prompt_str 10 ^
However this does not happen. Instead the cursor is placed at the end (after then 10) and further cursor movement redraws incorrectly. The same problem exists when we double-Tab but in a different part of the code.
Fix this by sending a carriage return and then redisplaying the text to the left of the cursor.
Cc: stable@vger.kernel.org Signed-off-by: Daniel Thompson daniel.thompson@linaro.org
kernel/debug/kdb/kdb_io.c | 5 +++++ 1 file changed, 5 insertions(+)
Reviewed-by: Douglas Anderson dianders@chromium.org
The code that handles case 14 (down) and case 16 (up) has been copy and pasted despite being byte-for-byte identical. Combine them.
Cc: stable@vger.kernel.org # Not a bug fix but it is needed for later bug fixes Signed-off-by: Daniel Thompson daniel.thompson@linaro.org --- kernel/debug/kdb/kdb_io.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index 69549fe42e87b..f167894b11b8e 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -298,6 +298,7 @@ static char *kdb_read(char *buffer, size_t bufsize) } break; case 14: /* Down */ + case 16: /* Up */ memset(tmpbuffer, ' ', strlen(kdb_prompt_str) + (lastchar-buffer)); *(tmpbuffer+strlen(kdb_prompt_str) + @@ -312,15 +313,6 @@ static char *kdb_read(char *buffer, size_t bufsize) ++cp; } break; - case 16: /* Up */ - memset(tmpbuffer, ' ', - strlen(kdb_prompt_str) + (lastchar-buffer)); - *(tmpbuffer+strlen(kdb_prompt_str) + - (lastchar-buffer)) = '\0'; - kdb_printf("\r%s\r", tmpbuffer); - *lastchar = (char)key; - *(lastchar+1) = '\0'; - return lastchar; case 9: /* Tab */ if (tab < 2) ++tab;
Hi,
On Mon, Apr 22, 2024 at 9:38 AM Daniel Thompson daniel.thompson@linaro.org wrote:
The code that handles case 14 (down) and case 16 (up) has been copy and pasted despite being byte-for-byte identical. Combine them.
Cc: stable@vger.kernel.org # Not a bug fix but it is needed for later bug fixes Signed-off-by: Daniel Thompson daniel.thompson@linaro.org
kernel/debug/kdb/kdb_io.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
Reviewed-by: Douglas Anderson dianders@chromium.org
Currently when the current line should be removed from the display kdb_read() uses memset() to fill a temporary buffer with spaces. The problem is not that this could be trivially implemented using a format string rather than open coding it. The real problem is that it is possible, on systems with a long kdb_prompt_str, to write pas the end of the tmpbuffer.
Happily, as mentioned above, this can be trivially implemented using a format string. Make it so!
Cc: stable@vger.kernel.org Signed-off-by: Daniel Thompson daniel.thompson@linaro.org --- kernel/debug/kdb/kdb_io.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index f167894b11b8e..2cd17313fe652 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -299,11 +299,9 @@ static char *kdb_read(char *buffer, size_t bufsize) break; case 14: /* Down */ case 16: /* Up */ - memset(tmpbuffer, ' ', - strlen(kdb_prompt_str) + (lastchar-buffer)); - *(tmpbuffer+strlen(kdb_prompt_str) + - (lastchar-buffer)) = '\0'; - kdb_printf("\r%s\r", tmpbuffer); + kdb_printf("\r%*c\r", + (int)(strlen(kdb_prompt_str) + (lastchar - buffer)), + ' '); *lastchar = (char)key; *(lastchar+1) = '\0'; return lastchar;
Hi,
On Mon, Apr 22, 2024 at 9:38 AM Daniel Thompson daniel.thompson@linaro.org wrote:
Currently when the current line should be removed from the display kdb_read() uses memset() to fill a temporary buffer with spaces. The problem is not that this could be trivially implemented using a format string rather than open coding it. The real problem is that it is possible, on systems with a long kdb_prompt_str, to write pas
nit: s/pas/past
the end of the tmpbuffer.
Happily, as mentioned above, this can be trivially implemented using a format string. Make it so!
Cc: stable@vger.kernel.org Signed-off-by: Daniel Thompson daniel.thompson@linaro.org
kernel/debug/kdb/kdb_io.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
Reviewed-by: Douglas Anderson dianders@chromium.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!
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@lina...
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!
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.
linux-stable-mirror@lists.linaro.org
-
Daniel Thompson -
Doug Anderson -
Justin Stitt