[vim/vim] Address the ASAN errors uncovered by new netbeans tests (#7248)

[Yegappan Lakshmanan]

Validate the buffer pointer before dereferencing it. Free the sign type name.

---

You can view, comment on, or merge this pull request online at:

  https://github.com/vim/vim/pull/7248

Commit Summary

• Validate the buffer pointer before derefencing it. Free the sign type name

File Changes

• M src/netbeans.c (10)
• M src/testdir/test_netbeans.vim (1)

Patch Links:

• https://github.com/vim/vim/pull/7248.patch
• https://github.com/vim/vim/pull/7248.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

[Yegappan Lakshmanan]

@yegappan pushed 1 commit.

• 1a2ff85 Use the right type for cast

—
You are receiving this because you are subscribed to this thread.

View it on GitHub or unsubscribe.

[Bram Moolenaar]

Thanks. Without the code changes I can't reproduce the error in valgrind. It did happen without the added sleep commands, it's not obvious why. Did the test failure cause the buffer to be wiped out? Perhaps we can have a test that gets us in this situation, so we test what the code change fixes.

[Yegappan Lakshmanan]

@yegappan pushed 1 commit.

• ad69d43 Add a test for reproducing the ASAN error with the netbeans interface

—
You are receiving this because you are subscribed to this thread.

[codecov[bot]]

Codecov Report

Merging #7248 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@

##           master    #7248      +/-   ##

==========================================

+ Coverage   88.84%   88.87%   +0.02%     

==========================================

  Files         148      148              

  Lines      162494   162561      +67     

==========================================

+ Hits       144367   144473     +106     

+ Misses      18127    18088      -39     

Impacted Files	Coverage Δ
src/netbeans.c	84.26% <100.00%> (+0.51%)	⬆️
src/if_xcmdsrv.c	88.55% <0.00%> (-0.36%)	⬇️
src/fold.c	87.29% <0.00%> (-0.26%)	⬇️
src/eval.c	96.44% <0.00%> (-0.23%)	⬇️
src/dict.c	91.96% <0.00%> (-0.20%)	⬇️
src/vim9compile.c	93.14% <0.00%> (-0.06%)	⬇️
src/message.c	88.81% <0.00%> (-0.05%)	⬇️
src/time.c	94.71% <0.00%> (ø)
src/getchar.c	86.12% <0.00%> (ø)
src/version.c	92.13% <0.00%> (ø)
... and 23 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fd3a4b...ad69d43. Read the comment docs.

[Bram Moolenaar]

Closed #7248 via 32e5ec0.

[Yegappan Lakshmanan]

Hi Bram,

On Tue, Nov 3, 2020 at 8:51 AM Bram Moolenaar <vim-dev...@256bit.org> wrote:

Thanks. Without the code changes I can't reproduce the error in valgrind. It did happen without the added sleep commands, it's not obvious why. Did the test failure cause the buffer to be wiped out? Perhaps we can have a test that gets us in this situation, so we test what the code change fixes.

This is a regression caused by the changes made to support channels

(7.4.1182: Still socket code intertwined with netbeans).

Before this change, NETBEANS_OPEN used to return TRUE as long as the

netbeans socket handle was valid (nb_sock != -1). When nb_sock is set to -1,

all the buffer references are also cleared.

In the latest code, NETBEANS_OPEN returns channel_can_write_to(nb_channel).

So if the channel is not writable then this will return FALSE. So when a buffer

is wiped out, if the netbeans channel is not writable, then the reference to the

buffer is not cleared (resulting in a dangling pointer). Later when the netbeans

connection is closed using the nbclose command, the buffers are cleared.

But as the buffer pointer is no longer valid, this results in the ASAN error.

For now, the added check for the sane buffer pointer addresses this problem.

But I am wondering whether we should change NETBEANS_OPEN to

check for nb_channel != NULL.

Regards,

Yegappan

[vim-dev ML]

[Bram Moolenaar]

Thanks for looking into this. Obviously avoiding memory access errors is important. That some memory is release only later (perhaps much later) is less important.
if you know how to fix it that is welcome, but only if there is no risk of breaking something else.

Hmm, perhaps it should be:
#define NETBEANS_OPEN ((nb_channel) != NULL && channel_can_write_to(nb_channel))
No, channel_can_write_to() checks for the NULL pointer, thus that's not needed.

Or were you referring to what happens in netbeans_file_killed() ?

—
You are receiving this because you commented.

[Yegappan Lakshmanan]

Hi Bram,

On Fri, Nov 6, 2020 at 1:01 PM Bram Moolenaar <vim-dev...@256bit.org> wrote:

Thanks for looking into this. Obviously avoiding memory access errors is important. That some memory is release only later (perhaps much later) is less important.
if you know how to fix it that is welcome, but only if there is no risk of breaking something else.

Hmm, perhaps it should be:
#define NETBEANS_OPEN ((nb_channel) != NULL && channel_can_write_to(nb_channel))
No, channel_can_write_to() checks for the NULL pointer, thus that's not needed.

Or were you referring to what happens in netbeans_file_killed() ?

Yes. I am referring to the netbeans_file_killed() function. If the netbeans

connection is not open, then this function will not cleanup the buffer

reference. Later when the netbeans connection is closed using nbclose,

it will try to access the dangling buffer pointer.

We can modify this function to always cleanup the buffer reference even

if the netbeans connection is closed.

- Yegappan

[vim-dev ML]

[Bram Moolenaar]

Yegappan wrote:

> On Fri, Nov 6, 2020 at 1:01 PM Bram Moolenaar <vim-dev...@256bit.org>
> wrote:
>
> > Thanks for looking into this. Obviously avoiding memory access errors is
> > important. That some memory is release only later (perhaps much later) is
> > less important.
> > if you know how to fix it that is welcome, but only if there is no risk of
> > breaking something else.
> >
> > Hmm, perhaps it should be:
> > #define NETBEANS_OPEN ((nb_channel) != NULL &&
> > channel_can_write_to(nb_channel))
> > No, channel_can_write_to() checks for the NULL pointer, thus that's not
> > needed.
> >
> > Or were you referring to what happens in netbeans_file_killed() ?
> >
> >
> > Yes. I am referring to the netbeans_file_killed() function. If the netbeans
> connection is not open, then this function will not cleanup the buffer
> reference. Later when the netbeans connection is closed using nbclose,
> it will try to access the dangling buffer pointer.
>
> We can modify this function to always cleanup the buffer reference even
> if the netbeans connection is closed.

So that we then only need to check for NULL? That would rely on always
calling netbeans_file_killed() when a buffer is deleted. Perhaps that
is true already.

I notice the "bufp" is cleared, but no memory is freed. "displayname"
and "signmap" could probably be cleared, and signmaplen set to zero.

--
hundred-and-one symptoms of being an internet addict:
210. When you get a divorce, you don't care about who gets the children,
but discuss endlessly who can use the email address.

/// 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 ///