segfault in popupmenu.c

[Markus Braun]

Hi,

I currently run in a lot of segfaults when using VIM together with ALE plugin
and LSP when editing a C file. The segfault happens deep inside VIM code when
preparing balloon message for display so it should not have directly to do with
the plugin. I started to debug the problem but I'm kind of helpless that deep
in the code of VIM. I'll try to describe my findings as good as possible and
hope that someone with more insight might be able to fix the problem. My is VIM
version is 8.1.2321.

The segfault happens in popupmenu.c line 1240:

1238 // exclude spaces at the end of the string
1239 for (copylen = thislen; copylen > 0; --copylen)
1240 if (item->start[skip + copylen - 1] != ' ')
1241 break;

where the memory of "item->start" is not accessible.

Here are the values of hopefully all relevant variables from GDB/core file:

>>> print *item
$1 = {start = 0x3020007d7d <error: Cannot access memory at address 0x3020007d7d>, bytelen = 49, cells = 0, indent = 1}
>>> print line
$2 = 3
>>> print item_idx
$3 = 20
>>> print height
$4 = 5
>>> print ga
$5 = {ga_len = 3, ga_maxlen = 20, ga_itemsize = 24, ga_growsize = 20, ga_data = 0x55c3a3d1c490}
>>> print *((balpart_T *)ga.ga_data)@20
$6 = {[0] = {start = 0x55c3a3cef6f0 "Declared in global namespace\n\ntypedef __uint32_t uint32_t", bytelen = 28, cells = 28, indent = 0},
[1] = {start = 0x55c3a3cef70d "\ntypedef __uint32_t uint32_t", bytelen = 0, cells = 0, indent = 0},
[2] = {start = 0x55c3a3cef70e "typedef __uint32_t uint32_t", bytelen = 27, cells = 27, indent = 0},
[3] = {start = 0x0, bytelen = 0, cells = 0, indent = 0} <repeats 17 times>}
>>> p **array@5
$7 = {[0] = {pum_text = 0x55c3a3d2f390 "", pum_kind = 0x0, pum_extra = 0x0, pum_info = 0x0},
[1] = {pum_text = 0x55c3a3d2f1f0 "Declared in global namespace", pum_kind = 0x0, pum_extra = 0x0, pum_info = 0x0},
[2] = {pum_text = 0x55c3a3d2f360 "typedef __uint32_t uint32_t", pum_kind = 0x0, pum_extra = 0x0, pum_info = 0x0},
[3] = {pum_text = 0x0, pum_kind = 0x0, pum_extra = 0x0, pum_info = 0x0},
[4] = {pum_text = 0x55c3a3d251e0 "", pum_kind = 0x0, pum_extra = 0x0, pum_info = 0x0}}


Because there are several entries in "ga.ga_data" that have a "bytelen"
of 0 the for loop in line 1212 is never executed and thus the variable
"line" is never incremented. Thus the "item_idx" is incremented above
the value of "ga.ga_len" which causes accessing an invalid entry.

Markus Braun

--
Any errors in spelling, tact or fact are transmission errors

[Markus Braun]

> Because there are several entries in "ga.ga_data" that have a "bytelen"
> of 0 the for loop in line 1212 is never executed and thus the variable
> "line" is never incremented. Thus the "item_idx" is incremented above
> the value of "ga.ga_len" which causes accessing an invalid entry.

After thinking about this a second time I think the problem is that the
empty line in the input message is skipped because "bytelen" is 0.
So either the height should be calculated for the message excluding the
embedded empty line or an empty line should be inserted into the
formatted message.

[Bram Moolenaar]

Markus Braun wrote:

> > Because there are several entries in "ga.ga_data" that have a "bytelen"
> > of 0 the for loop in line 1212 is never executed and thus the variable
> > "line" is never incremented. Thus the "item_idx" is incremented above
> > the value of "ga.ga_len" which causes accessing an invalid entry.
>
> After thinking about this a second time I think the problem is that the
> empty line in the input message is skipped because "bytelen" is 0.
> So either the height should be calculated for the message excluding the
> embedded empty line or an empty line should be inserted into the
> formatted message.

What is the value of "mesg"? That's the only input, thus if we know
that we should be able to reproduce.

--
From "know your smileys":
:.-( Crying

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[Markus Braun]

On Wed, Nov 20, 2019 at 10:59:32PM +0100, Bram Moolenaar (Br...@moolenaar.net) wrote:
>
> Markus Braun wrote:
>
> > > Because there are several entries in "ga.ga_data" that have a "bytelen"
> > > of 0 the for loop in line 1212 is never executed and thus the variable
> > > "line" is never incremented. Thus the "item_idx" is incremented above
> > > the value of "ga.ga_len" which causes accessing an invalid entry.
> >
> > After thinking about this a second time I think the problem is that the
> > empty line in the input message is skipped because "bytelen" is 0.
> > So either the height should be calculated for the message excluding the
> > embedded empty line or an empty line should be inserted into the
> > formatted message.
>
> What is the value of "mesg"? That's the only input, thus if we know
> that we should be able to reproduce.

The value of "mesg" is "Declared in global namespace\n\ntypedef __uint32_t uint32_t"

The problem seems to be the consecutive '\n'. Because of that
ga.ga_data[1].start points to "\ntypedef __uint32_t uint32_t" and
ga.ga_data[1].bytelen is 0 which then causes skipping of this element
when preparing the formatted "array".

I've done a patch locally and it seems to fix the problem. But I don't
know if this is the right solution. Patch is attached. Maybe it makes my
above explanation little bit clearer ;-)