[v1] util/readline: Fix out-of-bounds access in readline_insert_char().

[PATCH] util/readline: Fix out-of-bounds access in readline_insert_char().

Posted by phind.uet@gmail.com 1 week, 2 days ago

From: Nguyen Dinh Phi <phind.uet@gmail.com>

Currently, the readline_insert_char() function is guarded by the cursor
position (cmd_buf_index) rather than the actual buffer fill level(cmd_buf_size).
The current check is:
	if (rs->cmd_buf_index < READLINE_CMD_BUF_SIZE)

This logic is flawed because if the command buffer is full and a user moves the
cursor backward (e.g. by sending left arrow key), cmd_buf_index can be
decreased without descreasing of buffer size.
This allow subsequent insertions to increase cmd_buf_size past its maximum
limit of rs->cmd_buf.

Because in the ReadLineState struct, cmd_buf[READLINE_CMD_BUF_SIZE + 1] is
immediately followed by the cmd_buf_index integer, once the buffer size is
sufficiently inflated, the memmove() operation inside readline_insert_char()
can write past the end of cmd_buf[] and overwrite cmd_buf_index itself.

The subsequent line:
	rs->cmd_buf[rs->cmd_buf_index] = ch;

then writes the input character to an address determined by the now-corrupted
index.

By providing a specifically crafted input sequence via HMP, this flaw can be
used to redirect the write operation to overwrite any field within the
ReadLineState structure, which can lead to unpredictable behavior or
application crashes.

Fix this by adding the guard to check for buffer fullness and ensuring the 
index remains valid.

Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com>
---
 util/readline.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util/readline.c b/util/readline.c
index 0f19674f52..6b1de1094a 100644
--- a/util/readline.c
+++ b/util/readline.c
@@ -84,7 +84,8 @@ static void readline_update(ReadLineState *rs)
 
 static void readline_insert_char(ReadLineState *rs, int ch)
 {
-    if (rs->cmd_buf_index < READLINE_CMD_BUF_SIZE) {
+    if (rs->cmd_buf_index <= rs->cmd_buf_size &&
+            rs->cmd_buf_size < READLINE_CMD_BUF_SIZE) {
         memmove(rs->cmd_buf + rs->cmd_buf_index + 1,
                 rs->cmd_buf + rs->cmd_buf_index,
                 rs->cmd_buf_size - rs->cmd_buf_index);
-- 
2.43.0

Re: [PATCH] util/readline: Fix out-of-bounds access in readline_insert_char().

Posted by Marc-André Lureau 1 week ago

Hi

On Thu, Apr 2, 2026 at 3:01 PM <phind.uet@gmail.com> wrote:
>
> From: Nguyen Dinh Phi <phind.uet@gmail.com>
>
> Currently, the readline_insert_char() function is guarded by the cursor
> position (cmd_buf_index) rather than the actual buffer fill level(cmd_buf_size).
> The current check is:
>         if (rs->cmd_buf_index < READLINE_CMD_BUF_SIZE)
>
> This logic is flawed because if the command buffer is full and a user moves the
> cursor backward (e.g. by sending left arrow key), cmd_buf_index can be
> decreased without descreasing of buffer size.
> This allow subsequent insertions to increase cmd_buf_size past its maximum
> limit of rs->cmd_buf.
>
> Because in the ReadLineState struct, cmd_buf[READLINE_CMD_BUF_SIZE + 1] is
> immediately followed by the cmd_buf_index integer, once the buffer size is
> sufficiently inflated, the memmove() operation inside readline_insert_char()
> can write past the end of cmd_buf[] and overwrite cmd_buf_index itself.
>
> The subsequent line:
>         rs->cmd_buf[rs->cmd_buf_index] = ch;
>
> then writes the input character to an address determined by the now-corrupted
> index.
>
> By providing a specifically crafted input sequence via HMP, this flaw can be
> used to redirect the write operation to overwrite any field within the
> ReadLineState structure, which can lead to unpredictable behavior or
> application crashes.
>
> Fix this by adding the guard to check for buffer fullness and ensuring the
> index remains valid.
>
> Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com>
> ---
>  util/readline.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/util/readline.c b/util/readline.c
> index 0f19674f52..6b1de1094a 100644
> --- a/util/readline.c
> +++ b/util/readline.c
> @@ -84,7 +84,8 @@ static void readline_update(ReadLineState *rs)
>
>  static void readline_insert_char(ReadLineState *rs, int ch)
>  {
> -    if (rs->cmd_buf_index < READLINE_CMD_BUF_SIZE) {
> +    if (rs->cmd_buf_index <= rs->cmd_buf_size &&
> +            rs->cmd_buf_size < READLINE_CMD_BUF_SIZE) {

Why change rs->cmd_buf_index < READLINE_CMD_BUF_SIZE to
rs->cmd_buf_index <= READLINE_CMD_BUF_SIZE ?

otherwise, good catch

>          memmove(rs->cmd_buf + rs->cmd_buf_index + 1,
>                  rs->cmd_buf + rs->cmd_buf_index,
>                  rs->cmd_buf_size - rs->cmd_buf_index);
> --
> 2.43.0
>
>


-- 
Marc-André Lureau

Re: [PATCH] util/readline: Fix out-of-bounds access in readline_insert_char().

Posted by Phi Nguyen 1 week ago

On 4/4/2026 2:41 PM, Marc-André Lureau wrote:
> Why change rs->cmd_buf_index < READLINE_CMD_BUF_SIZE to
> rs->cmd_buf_index <= READLINE_CMD_BUF_SIZE ?
Hi,
my patch is actually change from
     if (rs->cmd_buf_index < READLINE_CMD_BUF_SIZE)

to
     if (rs->cmd_buf_index <= rs->cmd_buf_size &&
             rs->cmd_buf_size < READLINE_CMD_BUF_SIZE) {

BR,
Phi

Re: [PATCH] util/readline: Fix out-of-bounds access in readline_insert_char().

Posted by Marc-André Lureau 1 week ago

Hi

On Sat, Apr 4, 2026 at 11:32 AM Phi Nguyen <phind.uet@gmail.com> wrote:
>
> On 4/4/2026 2:41 PM, Marc-André Lureau wrote:
> > Why change rs->cmd_buf_index < READLINE_CMD_BUF_SIZE to
> > rs->cmd_buf_index <= READLINE_CMD_BUF_SIZE ?
> Hi,
> my patch is actually change from
>      if (rs->cmd_buf_index < READLINE_CMD_BUF_SIZE)
>
> to
>      if (rs->cmd_buf_index <= rs->cmd_buf_size &&
>              rs->cmd_buf_size < READLINE_CMD_BUF_SIZE) {
>

Right, my bad. Wouldn't it be a programming error if "cmd_buf_index >
rs->cmd_buf_size" ?

Maybe make this an assert() ?

-- 
Marc-André Lureau

Re: [PATCH] util/readline: Fix out-of-bounds access in readline_insert_char().

Posted by Nguyen Dinh Phi [SG] 5 days, 14 hours ago

On 4/4/26 21:52, Marc-André Lureau wrote:
> Hi
> 
> On Sat, Apr 4, 2026 at 11:32 AM Phi Nguyen <phind.uet@gmail.com> wrote:
>>
>> On 4/4/2026 2:41 PM, Marc-André Lureau wrote:
>>> Why change rs->cmd_buf_index < READLINE_CMD_BUF_SIZE to
>>> rs->cmd_buf_index <= READLINE_CMD_BUF_SIZE ?
>> Hi,
>> my patch is actually change from
>>       if (rs->cmd_buf_index < READLINE_CMD_BUF_SIZE)
>>
>> to
>>       if (rs->cmd_buf_index <= rs->cmd_buf_size &&
>>               rs->cmd_buf_size < READLINE_CMD_BUF_SIZE) {
>>
> 
> Right, my bad. Wouldn't it be a programming error if "cmd_buf_index >
> rs->cmd_buf_size" ?
> 
> Maybe make this an assert() ?
> 
Hi,

That's right, I will send a new version to add the assert()

BR,
Phi