Bram Moolenaar wrote:
> Dominique Pelle wrote:
>
>> Testing autocommands, I see that Vim-7.2.107 (and older)
>> is using memory already freed when doing silly autocommands
>> such as:
>>
>> $ touch foobar
>> $ valgrind ./vim -u NONE -c 'au! BufReadPre * cd /tmp' \
>> -c 'e foobar' 2> vg.log
>>
>> In vg.log, I then see the following error:
>>
>> ==15058== Syscall param open(filename) points to unaddressable byte(s)
>> ==15058== at 0x40007D2: (within /lib/ld-2.8.90.so)
>> ==15058== by 0x805365E: open_buffer (buffer.c:130)
>> ==15058== by 0x8098548: do_ecmd (ex_cmds.c:3655)
>> ==15058== by 0x80AE8E9: do_exedit (ex_docmd.c:7557)
>> ==15058== by 0x80AE560: ex_edit (ex_docmd.c:7452)
>> ==15058== by 0x80A6FE7: do_one_cmd (ex_docmd.c:2622)
>> ==15058== by 0x80A4867: do_cmdline (ex_docmd.c:1096)
>> ==15058== by 0x80A3F00: do_cmdline_cmd (ex_docmd.c:702)
>> ==15058== by 0x80E8A07: exe_commands (main.c:2693)
>> ==15058== by 0x80E63A7: main (main.c:874)
>> ==15058== Address 0x54312d4 is 4 bytes inside a block of size 11 free'd
>> ==15058== at 0x4024E5A: free (vg_replace_malloc.c:323)
>> ==15058== by 0x8114D5D: vim_free (misc2.c:1631)
>> ==15058== by 0x80C97DF: shorten_fnames (fileio.c:5715)
>> ==15058== by 0x80AF1A9: ex_cd (ex_docmd.c:7942)
>> ==15058== by 0x80A6FE7: do_one_cmd (ex_docmd.c:2622)
>> ==15058== by 0x80A4867: do_cmdline (ex_docmd.c:1096)
>> ==15058== by 0x80CD35A: apply_autocmds_group (fileio.c:8853)
>> ==15058== by 0x80CCD9B: apply_autocmds_exarg (fileio.c:8481)
>> ==15058== by 0x80C2246: readfile (fileio.c:723)
>> ==15058== by 0x805365E: open_buffer (buffer.c:130)
>> ==15058== by 0x8098548: do_ecmd (ex_cmds.c:3655)
>> ==15058== by 0x80AE8E9: do_exedit (ex_docmd.c:7557)
[...snip...]
>
> Thanks. It's in the todo list.
I found another case of an autocommand which also causes
to use freed memory and the proposed patch in my previous
email did not help to fix it. Here is how to reproduce it:
# Open a file foobar in vim to create swap file .foobar.swp
$ vim foobar
# In another terminal...
$ vim -u NONE
:au! SwapExists * cd /tmp
:e foobar
... and valgrind complains about use of freed memory:
==17226== Syscall param open(filename) points to unaddressable byte(s)
==17226== at 0x40007D2: (within /lib/ld-2.8.90.so)
==17226== by 0x805365E: open_buffer (buffer.c:130)
==17226== by 0x8098548: do_ecmd (ex_cmds.c:3655)
==17226== by 0x80AE8E9: do_exedit (ex_docmd.c:7557)
==17226== by 0x80AE560: ex_edit (ex_docmd.c:7452)
==17226== by 0x80A6FE7: do_one_cmd (ex_docmd.c:2622)
==17226== by 0x80A4867: do_cmdline (ex_docmd.c:1096)
==17226== by 0x812A4BC: nv_colon (normal.c:5233)
==17226== by 0x8123B40: normal_cmd (normal.c:1200)
==17226== by 0x80E6969: main_loop (main.c:1180)
==17226== by 0x80E64B6: main (main.c:939)
==17226== Address 0x543644c is 4 bytes inside a block of size 11 free'd
==17226== at 0x4024E5A: free (vg_replace_malloc.c:323)
==17226== by 0x8114D5D: vim_free (misc2.c:1631)
==17226== by 0x80C97DF: shorten_fnames (fileio.c:5715)
==17226== by 0x80AF1A9: ex_cd (ex_docmd.c:7942)
==17226== by 0x80A6FE7: do_one_cmd (ex_docmd.c:2622)
==17226== by 0x80A4867: do_cmdline (ex_docmd.c:1096)
==17226== by 0x80CD35A: apply_autocmds_group (fileio.c:8853)
==17226== by 0x80CCD5D: apply_autocmds (fileio.c:8464)
==17226== by 0x80FA022: do_swapexists (memline.c:3770)
==17226== by 0x80FA93D: findswapname (memline.c:4130)
==17226== by 0x80F4A88: ml_open_file (memline.c:553)
==17226== by 0x80F4BAE: check_need_swap (memline.c:605)
==17226== by 0x80C206F: readfile (fileio.c:670)
==17226== by 0x805365E: open_buffer (buffer.c:130)
==17226== by 0x8098548: do_ecmd (ex_cmds.c:3655)
==17226== by 0x80AE8E9: do_exedit (ex_docmd.c:7557)
==17226== by 0x80AE560: ex_edit (ex_docmd.c:7452)
==17226== by 0x80A6FE7: do_one_cmd (ex_docmd.c:2622)
==17226== by 0x80A4867: do_cmdline (ex_docmd.c:1096)
==17226== by 0x812A4BC: nv_colon (normal.c:5233)
==17226== by 0x8123B40: normal_cmd (normal.c:1200)
==17226== by 0x80E6969: main_loop (main.c:1180)
==17226== by 0x80E64B6: main (main.c:939)
The problem happens here because readfile() in fileio.c
calls check_need_swap(newfile); at line fileio.c:670 and
this function can trigger an autocommand (SwapExists)
which can potentially free or realloc curbuf->b_fname
or curbuf->b_ffname.
If parameters fname or sfname of readfile() are aliased to
curbuf->b_fname or curbuf->b_ffname, then readfile() may
read free memory after the autocommand has been executed.
The new attached patch fixes this new problem and also fixes
the 2 errors described in previous email. I'm not sure how
clean the fix is. Please review it. At least it should help to
understand what the problem is.
Cheers
-- Dominique
--~--~---------~--~----~------------~-------~--~----~
You received this message from the "vim_dev" maillist.
For more information, visit http://www.vim.org/maillist.php
-~----------~----~----~----~------~----~------~--~---
Index: fileio.c
===================================================================
RCS file: /cvsroot/vim/vim7/src/fileio.c,v
retrieving revision 1.129
diff -c -r1.129 fileio.c
*** fileio.c 31 Dec 2008 15:21:17 -0000 1.129
--- fileio.c 13 Feb 2009 20:53:17 -0000
***************
*** 69,75 ****
static int au_find_group __ARGS((char_u *name));
# define AUGROUP_DEFAULT -1 /* default autocmd group */
! # define AUGROUP_ERROR -2 /* errornouse autocmd group */
# define AUGROUP_ALL -3 /* all autocmd groups */
#endif
--- 69,75 ----
static int au_find_group __ARGS((char_u *name));
# define AUGROUP_DEFAULT -1 /* default autocmd group */
! # define AUGROUP_ERROR -2 /* erroneous autocmd group */
# define AUGROUP_ALL -3 /* all autocmd groups */
#endif
***************
*** 295,300 ****
--- 295,311 ----
int conv_restlen = 0; /* nr of bytes in conv_rest[] */
#endif
+ /*
+ * Remember the initial values of curbuf, curbuf->b_ffname and
+ * curbuf->b_fname to detect whether they are altered as a result
+ * of executing autocommands.
+ */
+ buf_T *old_curbuf = curbuf;
+ char_u *old_curbuf_b_ffname = curbuf->b_ffname;
+ char_u *old_curbuf_b_fname = curbuf->b_fname;
+ int aliased_b_ffname = (fname == curbuf->b_ffname) || (sfname == curbuf->b_ffname);
+ int aliased_b_fname = (fname == curbuf->b_fname) || (sfname == curbuf->b_fname);
+
write_no_eol_lnum = 0; /* in case it was set by the previous read */
/*
***************
*** 668,673 ****
--- 679,693 ----
#endif
{
check_need_swap(newfile);
+ if (!read_stdin && (curbuf != old_curbuf
+ || (aliased_b_ffname && (old_curbuf_b_ffname != curbuf->b_ffname))
+ || (aliased_b_fname && (old_curbuf_b_fname != curbuf->b_fname))))
+ {
+ /* TODO: should give an error message */
+ if (!read_buffer)
+ close(fd);
+ return FAIL;
+ }
#ifdef UNIX
/* Set swap file protection bits after creating it. */
if (swap_mode > 0 && curbuf->b_ml.ml_mfp->mf_fname != NULL)
***************
*** 698,704 ****
{
int m = msg_scroll;
int n = msg_scrolled;
- buf_T *old_curbuf = curbuf;
/*
* The file must be closed again, the autocommands may want to change
--- 718,723 ----
***************
*** 740,747 ****
--- 759,771 ----
/*
* Don't allow the autocommands to change the current buffer.
* Try to re-open the file.
+ *
+ * Don't allow the autocommands to change the buffer name either
+ * (cd for example) if it invalidates fname or sfname.
*/
if (!read_stdin && (curbuf != old_curbuf
+ || (aliased_b_ffname && (old_curbuf_b_ffname != curbuf->b_ffname))
+ || (aliased_b_fname && (old_curbuf_b_fname != curbuf->b_fname))
|| (fd = mch_open((char *)fname, O_RDONLY | O_EXTRA, 0)) < 0))
{
--no_wait_return;
