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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
