Re: [bug] invalid memory access with normal commands
Christian Brabandt wrote:
> On Di, 14 Feb 2017, Dominique Pellé wrote:
>
> > Hi
> >
> > afl-fuzz found another command that causes access to
> > invalid memory in Vim-8.0.329. It's not a recent regression
> > since bug is present in at least Vim-7.4.52 that comes
> > with ubuntu-14.04.
> >
> > I have not been able to find a fix yet.
> >
> > Step to reproduce:
> >
> > $ valgrind vim -u NONE -c'norm oxx' -c'norm vapo' -c'q!' 2>vg.log
> >
> > And vg.log contains:
> >
> > ==4259== Memcheck, a memory error detector
> > ==4259== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
> > ==4259== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright
> > info
> > ==4259== Command: vim -u NONE -cnorm\ oxx -cnorm\ vapo -cq!
> > ==4259==
> > ==4259== Invalid read of size 1
> > ==4259==at 0x4CF4A0: utf_head_off (mbyte.c:3746)
> > ==4259==by 0x4D094C: mb_adjustpos (mbyte.c:4028)
> > ==4259==by 0x4E11C9: normal_cmd (normal.c:1292)
> > ==4259==by 0x468F25: exec_normal (ex_docmd.c:10418)
> > ==4259==by 0x469076: ex_normal (ex_docmd.c:10310)
> > ==4259==by 0x46B734: do_one_cmd (ex_docmd.c:2981)
> > ==4259==by 0x46B734: do_cmdline (ex_docmd.c:1120)
> > ==4259==by 0x5BFC2B: exe_commands (main.c:2905)
> > ==4259==by 0x5BFC2B: vim_main2 (main.c:781)
> > ==4259==by 0x40C3D3: main (main.c:415)
> > ==4259== Address 0xa95bc50 is 0 bytes after a block of size 4,096 alloc'd
> > ==4259==at 0x4C2ABF5: malloc (vg_replace_malloc.c:299)
> > ==4259==by 0x4C4D0B: lalloc (misc2.c:942)
> > ==4259==by 0x5C0830: mf_alloc_bhdr.isra.3 (memfile.c:907)
> > ==4259==by 0x5C1546: mf_new (memfile.c:381)
> > ==4259==by 0x4AC6FF: ml_new_data (memline.c:3513)
> > ==4259==by 0x4AF0FC: ml_open (memline.c:400)
> > ==4259==by 0x414DD6: open_buffer (buffer.c:163)
> > ==4259==by 0x5BF891: create_windows (main.c:2677)
> > ==4259==by 0x5BF891: vim_main2 (main.c:704)
> > ==4259==by 0x40C3D3: main (main.c:415)
> >
> > Possibly related to this, the following command does not
> > seem to behave correctly, even if it does not trigger invalid
> > memory:
> >
> > $ echo "foo bar" | vim -u NONE -c'norm wvapo' -
> >
> > Then press o multiple times and observe that cursor
> > alternates between the beginning of the visual block
> > and the *middle* of the visual block. I would expect it to
> > alternate between the beginning and the *end* of the
> > visual block.
>
> Interesting. The second issue happens, because current_par does
> internally switch to visual line mode, but does not reset the VIsual
> column correctly. This patch fixes it:
>
> diff --git a/src/search.c b/src/search.c
> index 36410e50f..ffda65837 100644
> --- a/src/search.c
> +++ b/src/search.c
> @@ -4241,6 +4241,7 @@ extend:
> * line, we get stuck there. Trap this here. */
> if (VIsual_mode == 'V' && start_lnum == curwin->w_cursor.lnum)
> goto extend;
> + VIsual.col = 0;
> VIsual.lnum = start_lnum;
> VIsual_mode = 'V';
> redraw_curbuf_later(INVERTED); /* update the inversion */
>
> As a side effect this also disables switching cursor position using 'o'
> which is currently a no-op when only a single line is selected (since it
> effectively switches positions between the start of line and the start
> of line).
I think the column should only be reset when VIsual.lnum actually
changes.
> Interestingly, I never noticed that this does nothing currently if a
> single visual line is selected.
>
> As it stands, I think this also fixes the invalid read mentioned above.
> I just wanted to take a look at that issue, when I noticed that this
> patch does not exhibit this behaviour, so I didn't check very closely
> why it works.
>
> I tried to add a test, to verify that this is fixed, but whatever I do,
> I always get back col 0 for the visual mode. I think this is because in
> getmark_buf_fnum() (mark.c:417) the column is explicitly set to zero for
> visual line mode and also using getpos('v') does not work, since by the
> time I run it, Visual mode is no longer active, so I cannot verify that
> this works in vimscript.
I can at least add a test to verify the memory access is fixed.
--
"Beware of bugs in the above code; I have only proved
it correct, not tried it." -- Donald Knuth
/// Bram Moolenaar -- b...@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///
--
--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php
---
You received this message because you are subscribed to the Google Groups
"vim_dev" group.
To unsubscribe from this group and stop receiving
Re: [bug] invalid memory access with normal commands
On Mi, 15 Feb 2017, Dominique Pellé wrote: > Christian Brabandt wrote: > > On Di, 14 Feb 2017, Dominique Pellé wrote: > Your patch appears to fix the valgrind error. > However, it's not correct. If I do... > > $ echo "foo bar" | vim -u NONE -c'norm wvapo' - > > ... then pressing o multiple times, the cursor remains > at the beginning of the visual selection, instead of > alternating at the beginning and at the end of the > visual selection. That's what I meant with "I did never notice that this is the exact same behaviour if you use V (e.g. only visually select a single line) and then press o" Best, Christian -- Wen das Wort nicht schlägt, den schlägt auch der Stock nicht. -- Sokrates (470-399 v.Chr.) -- -- You received this message from the "vim_dev" maillist. Do not top-post! Type your reply below the text you are replying to. For more information, visit http://www.vim.org/maillist.php --- You received this message because you are subscribed to the Google Groups "vim_dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Re: [bug] invalid memory access with normal commands
Christian Brabandt wrote:
> Hi Dominique!
>
> On Di, 14 Feb 2017, Dominique Pellé wrote:
>
>> Hi
>>
>> afl-fuzz found another command that causes access to
>> invalid memory in Vim-8.0.329. It's not a recent regression
>> since bug is present in at least Vim-7.4.52 that comes
>> with ubuntu-14.04.
>>
>> I have not been able to find a fix yet.
>>
>> Step to reproduce:
>>
>> $ valgrind vim -u NONE -c'norm oxx' -c'norm vapo' -c'q!' 2>vg.log
>>
>> And vg.log contains:
>>
>> ==4259== Memcheck, a memory error detector
>> ==4259== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
>> ==4259== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright
>> info
>> ==4259== Command: vim -u NONE -cnorm\ oxx -cnorm\ vapo -cq!
>> ==4259==
>> ==4259== Invalid read of size 1
>> ==4259==at 0x4CF4A0: utf_head_off (mbyte.c:3746)
>> ==4259==by 0x4D094C: mb_adjustpos (mbyte.c:4028)
>> ==4259==by 0x4E11C9: normal_cmd (normal.c:1292)
>> ==4259==by 0x468F25: exec_normal (ex_docmd.c:10418)
>> ==4259==by 0x469076: ex_normal (ex_docmd.c:10310)
>> ==4259==by 0x46B734: do_one_cmd (ex_docmd.c:2981)
>> ==4259==by 0x46B734: do_cmdline (ex_docmd.c:1120)
>> ==4259==by 0x5BFC2B: exe_commands (main.c:2905)
>> ==4259==by 0x5BFC2B: vim_main2 (main.c:781)
>> ==4259==by 0x40C3D3: main (main.c:415)
>> ==4259== Address 0xa95bc50 is 0 bytes after a block of size 4,096 alloc'd
>> ==4259==at 0x4C2ABF5: malloc (vg_replace_malloc.c:299)
>> ==4259==by 0x4C4D0B: lalloc (misc2.c:942)
>> ==4259==by 0x5C0830: mf_alloc_bhdr.isra.3 (memfile.c:907)
>> ==4259==by 0x5C1546: mf_new (memfile.c:381)
>> ==4259==by 0x4AC6FF: ml_new_data (memline.c:3513)
>> ==4259==by 0x4AF0FC: ml_open (memline.c:400)
>> ==4259==by 0x414DD6: open_buffer (buffer.c:163)
>> ==4259==by 0x5BF891: create_windows (main.c:2677)
>> ==4259==by 0x5BF891: vim_main2 (main.c:704)
>> ==4259==by 0x40C3D3: main (main.c:415)
>>
>> Possibly related to this, the following command does not
>> seem to behave correctly, even if it does not trigger invalid
>> memory:
>>
>> $ echo "foo bar" | vim -u NONE -c'norm wvapo' -
>>
>> Then press o multiple times and observe that cursor
>> alternates between the beginning of the visual block
>> and the *middle* of the visual block. I would expect it to
>> alternate between the beginning and the *end* of the
>> visual block.
>
> Interesting. The second issue happens, because current_par does
> internally switch to visual line mode, but does not reset the VIsual
> column correctly. This patch fixes it:
>
> diff --git a/src/search.c b/src/search.c
> index 36410e50f..ffda65837 100644
> --- a/src/search.c
> +++ b/src/search.c
> @@ -4241,6 +4241,7 @@ extend:
> * line, we get stuck there. Trap this here. */
> if (VIsual_mode == 'V' && start_lnum == curwin->w_cursor.lnum)
> goto extend;
> + VIsual.col = 0;
> VIsual.lnum = start_lnum;
> VIsual_mode = 'V';
> redraw_curbuf_later(INVERTED); /* update the inversion */
>
> As a side effect this also disables switching cursor position using 'o'
> which is currently a no-op when only a single line is selected (since it
> effectively switches positions between the start of line and the start
> of line).
>
> Interestingly, I never noticed that this does nothing currently if a
> single visual line is selected.
>
> As it stands, I think this also fixes the invalid read mentioned above.
> I just wanted to take a look at that issue, when I noticed that this
> patch does not exhibit this behaviour, so I didn't check very closely
> why it works.
>
> I tried to add a test, to verify that this is fixed, but whatever I do,
> I always get back col 0 for the visual mode. I think this is because in
> getmark_buf_fnum() (mark.c:417) the column is explicitly set to zero for
> visual line mode and also using getpos('v') does not work, since by the
> time I run it, Visual mode is no longer active, so I cannot verify that
> this works in vimscript.
Hi Christian
Your patch appears to fix the valgrind error.
However, it's not correct. If I do...
$ echo "foo bar" | vim -u NONE -c'norm wvapo' -
... then pressing o multiple times, the cursor remains
at the beginning of the visual selection, instead of
alternating at the beginning and at the end of the
visual selection.
Regards
Dominique
--
--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php
---
You received this message because you are subscribed to the Google Groups
"vim_dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to vim_dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Re: [bug] invalid memory access with normal commands
Hi Dominique!
On Di, 14 Feb 2017, Dominique Pellé wrote:
> Hi
>
> afl-fuzz found another command that causes access to
> invalid memory in Vim-8.0.329. It's not a recent regression
> since bug is present in at least Vim-7.4.52 that comes
> with ubuntu-14.04.
>
> I have not been able to find a fix yet.
>
> Step to reproduce:
>
> $ valgrind vim -u NONE -c'norm oxx' -c'norm vapo' -c'q!' 2>vg.log
>
> And vg.log contains:
>
> ==4259== Memcheck, a memory error detector
> ==4259== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
> ==4259== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright
> info
> ==4259== Command: vim -u NONE -cnorm\ oxx -cnorm\ vapo -cq!
> ==4259==
> ==4259== Invalid read of size 1
> ==4259==at 0x4CF4A0: utf_head_off (mbyte.c:3746)
> ==4259==by 0x4D094C: mb_adjustpos (mbyte.c:4028)
> ==4259==by 0x4E11C9: normal_cmd (normal.c:1292)
> ==4259==by 0x468F25: exec_normal (ex_docmd.c:10418)
> ==4259==by 0x469076: ex_normal (ex_docmd.c:10310)
> ==4259==by 0x46B734: do_one_cmd (ex_docmd.c:2981)
> ==4259==by 0x46B734: do_cmdline (ex_docmd.c:1120)
> ==4259==by 0x5BFC2B: exe_commands (main.c:2905)
> ==4259==by 0x5BFC2B: vim_main2 (main.c:781)
> ==4259==by 0x40C3D3: main (main.c:415)
> ==4259== Address 0xa95bc50 is 0 bytes after a block of size 4,096 alloc'd
> ==4259==at 0x4C2ABF5: malloc (vg_replace_malloc.c:299)
> ==4259==by 0x4C4D0B: lalloc (misc2.c:942)
> ==4259==by 0x5C0830: mf_alloc_bhdr.isra.3 (memfile.c:907)
> ==4259==by 0x5C1546: mf_new (memfile.c:381)
> ==4259==by 0x4AC6FF: ml_new_data (memline.c:3513)
> ==4259==by 0x4AF0FC: ml_open (memline.c:400)
> ==4259==by 0x414DD6: open_buffer (buffer.c:163)
> ==4259==by 0x5BF891: create_windows (main.c:2677)
> ==4259==by 0x5BF891: vim_main2 (main.c:704)
> ==4259==by 0x40C3D3: main (main.c:415)
>
> Possibly related to this, the following command does not
> seem to behave correctly, even if it does not trigger invalid
> memory:
>
> $ echo "foo bar" | vim -u NONE -c'norm wvapo' -
>
> Then press o multiple times and observe that cursor
> alternates between the beginning of the visual block
> and the *middle* of the visual block. I would expect it to
> alternate between the beginning and the *end* of the
> visual block.
Interesting. The second issue happens, because current_par does
internally switch to visual line mode, but does not reset the VIsual
column correctly. This patch fixes it:
diff --git a/src/search.c b/src/search.c
index 36410e50f..ffda65837 100644
--- a/src/search.c
+++ b/src/search.c
@@ -4241,6 +4241,7 @@ extend:
* line, we get stuck there. Trap this here. */
if (VIsual_mode == 'V' && start_lnum == curwin->w_cursor.lnum)
goto extend;
+ VIsual.col = 0;
VIsual.lnum = start_lnum;
VIsual_mode = 'V';
redraw_curbuf_later(INVERTED); /* update the inversion */
As a side effect this also disables switching cursor position using 'o'
which is currently a no-op when only a single line is selected (since it
effectively switches positions between the start of line and the start
of line).
Interestingly, I never noticed that this does nothing currently if a
single visual line is selected.
As it stands, I think this also fixes the invalid read mentioned above.
I just wanted to take a look at that issue, when I noticed that this
patch does not exhibit this behaviour, so I didn't check very closely
why it works.
I tried to add a test, to verify that this is fixed, but whatever I do,
I always get back col 0 for the visual mode. I think this is because in
getmark_buf_fnum() (mark.c:417) the column is explicitly set to zero for
visual line mode and also using getpos('v') does not work, since by the
time I run it, Visual mode is no longer active, so I cannot verify that
this works in vimscript.
Best,
Christian
--
Mein Freund, ich brauche dich - wie eine Höhe, in der man anders
atmet.
-- Antoine de Saint-Exupéry
--
--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php
---
You received this message because you are subscribed to the Google Groups
"vim_dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to vim_dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
