Problem with readline is its API. It's basically a bunch of
global variables with no clear dependencies between them. In this
specific case that I'm seeing: in interactive mode the
cmdComplete() causes instant crash of virsh/virt-admin:

==27999== Invalid write of size 1
==27999==    at 0x516EF71: _rl_init_line_state (readline.c:742)
==27999==    by 0x5170054: rl_initialize (readline.c:1192)
==27999==    by 0x516E5E4: readline (readline.c:379)
==27999==    by 0x1B7024: vshReadline (vsh.c:3048)
==27999==    by 0x140DCF: main (virsh.c:905)
==27999==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

This is because readline keeps a copy of pointer to
rl_line_buffer and the moment cmdComplete() returns and readline
takes over, it accesses the copy which is now a dangling pointer.

To fix this, just keep the original state of rl_line_buffer and
restore it.

Fixes: 41400ac1dda55b817388a4050aa823051bda2e05
Fixes: a0e1ada63c0afdc2af3b9405cbf637d8bd28700c
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 tools/vsh.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/vsh.c b/tools/vsh.c
index de869248b4..c91d756885 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -3511,6 +3511,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
     const vshClientHooks *hooks = ctl->hooks;
     const char *lastArg = NULL;
     const char **args = NULL;
+    char *old_rl_line_buffer = NULL;
     g_auto(GStrv) matches = NULL;
     char **iter;
 
@@ -3531,6 +3532,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
 
     vshReadlineInit(ctl);
 
+    old_rl_line_buffer = g_steal_pointer(&rl_line_buffer);
     if (!(rl_line_buffer = g_strdup(vshCommandOptArgvString(cmd, "string"))))
         rl_line_buffer = g_strdup("");
 
@@ -3540,6 +3542,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
 
     matches = vshReadlineCompletion(lastArg, 0, 0);
     g_clear_pointer(&rl_line_buffer, g_free);
+    rl_line_buffer = g_steal_pointer(&old_rl_line_buffer);
 
     if (!matches)
         return false;
-- 
2.44.1

On Mon, May 27, 2024 at 11:18:53 +0200, Michal Privoznik wrote:
> Problem with readline is its API. It's basically a bunch of
> global variables with no clear dependencies between them. In this
> specific case that I'm seeing: in interactive mode the
> cmdComplete() causes instant crash of virsh/virt-admin:
> 
> ==27999== Invalid write of size 1
> ==27999==    at 0x516EF71: _rl_init_line_state (readline.c:742)
> ==27999==    by 0x5170054: rl_initialize (readline.c:1192)
> ==27999==    by 0x516E5E4: readline (readline.c:379)
> ==27999==    by 0x1B7024: vshReadline (vsh.c:3048)
> ==27999==    by 0x140DCF: main (virsh.c:905)
> ==27999==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
> 
> This is because readline keeps a copy of pointer to
> rl_line_buffer and the moment cmdComplete() returns and readline
> takes over, it accesses the copy which is now a dangling pointer.
> 
> To fix this, just keep the original state of rl_line_buffer and
> restore it.
> 
> Fixes: 41400ac1dda55b817388a4050aa823051bda2e05
> Fixes: a0e1ada63c0afdc2af3b9405cbf637d8bd28700c
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  tools/vsh.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/vsh.c b/tools/vsh.c
> index de869248b4..c91d756885 100644
> --- a/tools/vsh.c
> +++ b/tools/vsh.c
> @@ -3511,6 +3511,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
>      const vshClientHooks *hooks = ctl->hooks;
>      const char *lastArg = NULL;
>      const char **args = NULL;
> +    char *old_rl_line_buffer = NULL;
>      g_auto(GStrv) matches = NULL;
>      char **iter;
>  
> @@ -3531,6 +3532,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
>  
>      vshReadlineInit(ctl);
>  
> +    old_rl_line_buffer = g_steal_pointer(&rl_line_buffer);
>      if (!(rl_line_buffer = g_strdup(vshCommandOptArgvString(cmd, "string"))))
>          rl_line_buffer = g_strdup("");
>  

I've wondered about this when I was reworking the parser code. Do we
even need to call 'vshReadlineCompletion' (which just does:

  return rl_completion_matches(text, vshReadlineParse);

from 'cmdComplete'? What does that do? Can't we use 'vshReadlineParse'
instead?

The fix makes sense as is, but I never really understood why
'cmdComplete' even needed readline in the first place.

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On 5/27/24 14:53, Peter Krempa wrote:
> On Mon, May 27, 2024 at 11:18:53 +0200, Michal Privoznik wrote:
>> Problem with readline is its API. It's basically a bunch of
>> global variables with no clear dependencies between them. In this
>> specific case that I'm seeing: in interactive mode the
>> cmdComplete() causes instant crash of virsh/virt-admin:
>>
>> ==27999== Invalid write of size 1
>> ==27999==    at 0x516EF71: _rl_init_line_state (readline.c:742)
>> ==27999==    by 0x5170054: rl_initialize (readline.c:1192)
>> ==27999==    by 0x516E5E4: readline (readline.c:379)
>> ==27999==    by 0x1B7024: vshReadline (vsh.c:3048)
>> ==27999==    by 0x140DCF: main (virsh.c:905)
>> ==27999==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
>>
>> This is because readline keeps a copy of pointer to
>> rl_line_buffer and the moment cmdComplete() returns and readline
>> takes over, it accesses the copy which is now a dangling pointer.
>>
>> To fix this, just keep the original state of rl_line_buffer and
>> restore it.
>>
>> Fixes: 41400ac1dda55b817388a4050aa823051bda2e05
>> Fixes: a0e1ada63c0afdc2af3b9405cbf637d8bd28700c
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  tools/vsh.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/tools/vsh.c b/tools/vsh.c
>> index de869248b4..c91d756885 100644
>> --- a/tools/vsh.c
>> +++ b/tools/vsh.c
>> @@ -3511,6 +3511,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
>>      const vshClientHooks *hooks = ctl->hooks;
>>      const char *lastArg = NULL;
>>      const char **args = NULL;
>> +    char *old_rl_line_buffer = NULL;
>>      g_auto(GStrv) matches = NULL;
>>      char **iter;
>>  
>> @@ -3531,6 +3532,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd)
>>  
>>      vshReadlineInit(ctl);
>>  
>> +    old_rl_line_buffer = g_steal_pointer(&rl_line_buffer);
>>      if (!(rl_line_buffer = g_strdup(vshCommandOptArgvString(cmd, "string"))))
>>          rl_line_buffer = g_strdup("");
>>  
> 
> I've wondered about this when I was reworking the parser code. Do we
> even need to call 'vshReadlineCompletion' (which just does:
> 
>   return rl_completion_matches(text, vshReadlineParse);
> 
> from 'cmdComplete'? What does that do? Can't we use 'vshReadlineParse'
> instead?

We could call rl_completion_matches(), sure. But what we can not do is
call just vshReadlineParse(). Unless we want to mimic what
rl_completion_matches() does:

https://git.savannah.gnu.org/cgit/readline.git/tree/complete.c#n2214

In particular, we would need to call vshReadlineParse() in a loop until
it returns NULL, then reorder returned string list. I just found it
easier to call vshReadlineCompletion() which is what readline would call
on <TAB><TAB> in interactive mode.
Oh, and IIRC there were some caveats around quoting (see vshReadlineInit()).

> 
> The fix makes sense as is, but I never really understood why
> 'cmdComplete' even needed readline in the first place.

If you or somebody else wants to attempt dropping it, be my guest. It's
just that currently the code is re-used in both interactive and
non-interactive modes. But maybe there's a way to do all of that
(including quoting) in a way that does not depend on readline.

> 
> Reviewed-by: Peter Krempa <pkrempa@redhat.com>
> 

Thanks.

Michal