Re: [PATCH 5/6] vsh: Restore original rl_line_buffer after completion
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(a)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(a)redhat.com>
Thanks.
Michal