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.
Thanks for the patch. I thought of catching the problem at the cause,
by disallowing the autocommands to do something that will cause trouble.
However, it's very difficult to catch all situations.
So I think we can do both: disallow autocommands to do things that are
clearly wrong, and give an error if we notice side effects. That should
be safe.
Please have a look at the patch below. There might still be other
autocommands that cause trouble. If you see one, please report.
*** ../vim-7.2.127/src/ex_getln.c Fri Nov 28 10:59:57 2008
--- src/ex_getln.c Sun Mar 1 00:13:15 2009
***************
*** 2000,2007 ****
#if defined(FEAT_AUTOCMD) || defined(PROTO)
/*
! * Check if "curbuf_lock" is set and return TRUE when it is and give an error
! * message.
*/
int
curbuf_locked()
--- 2000,2007 ----
#if defined(FEAT_AUTOCMD) || defined(PROTO)
/*
! * Check if "curbuf_lock" or "allbuf_lock" is set and return TRUE when it is
! * and give an error message.
*/
int
curbuf_locked()
***************
*** 2011,2016 ****
--- 2011,2031 ----
EMSG(_("E788: Not allowed to edit another buffer now"));
return TRUE;
}
+ return allbuf_locked();
+ }
+
+ /*
+ * Check if "allbuf_lock" is set and return TRUE when it is and give an error
+ * message.
+ */
+ int
+ allbuf_locked()
+ {
+ if (allbuf_lock > 0)
+ {
+ EMSG(_("E811: Not allowed to change buffer information now"));
+ return TRUE;
+ }
return FALSE;
}
#endif
*** ../vim-7.2.127/src/globals.h Tue Jan 6 16:13:42 2009
--- src/globals.h Sun Mar 1 00:02:42 2009
***************
*** 616,621 ****
--- 616,626 ----
EXTERN int curbuf_lock INIT(= 0);
/* non-zero when the current buffer can't be
* changed. Used for FileChangedRO. */
+ EXTERN int allbuf_lock INIT(= 0);
+ /* non-zero when no buffer name can be
+ * changed, no buffer can be deleted and
+ * current directory can't be changed.
+ * Used for SwapExists et al. */
#endif
#ifdef FEAT_EVAL
# define HAVE_SANDBOX
*** ../vim-7.2.127/src/fileio.c Wed Dec 31 16:20:54 2008
--- src/fileio.c Sun Mar 1 02:11:09 2009
***************
*** 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,313 ----
int conv_restlen = 0; /* nr of bytes in conv_rest[] */
#endif
+ #ifdef FEAT_AUTOCMD
+ /* Remember the initial values of curbuf, curbuf->b_ffname and
+ * curbuf->b_fname to detect whether they are altered as a result of
+ * executing nasaty autocommands. Also check if "fname" and "sfname"
+ * point to one of these values. */
+ buf_T *old_curbuf = curbuf;
+ char_u *old_b_ffname = curbuf->b_ffname;
+ char_u *old_b_fname = curbuf->b_fname;
+ int using_b_ffname = (fname == curbuf->b_ffname)
+ || (sfname == curbuf->b_ffname);
+ int using_b_fname = (fname == curbuf->b_fname)
+ || (sfname == curbuf->b_fname);
+ #endif
write_no_eol_lnum = 0; /* in case it was set by the previous read */
/*
***************
*** 668,673 ****
--- 681,697 ----
#endif
{
check_need_swap(newfile);
+ #ifdef FEAT_AUTOCMD
+ if (!read_stdin && (curbuf != old_curbuf
+ || (using_b_ffname && (old_b_ffname != curbuf->b_ffname))
+ || (using_b_fname && (old_b_fname != curbuf->b_fname))))
+ {
+ EMSG(_("E812: Autocommands changed buffer or buffer name"));
+ if (!read_buffer)
+ close(fd);
+ return FAIL;
+ }
+ #endif
#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
--- 722,727 ----
***************
*** 740,747 ****
--- 763,775 ----
/*
* 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
+ || (using_b_ffname && (old_b_ffname != curbuf->b_ffname))
+ || (using_b_fname && (old_b_fname != curbuf->b_fname))
|| (fd = mch_open((char *)fname, O_RDONLY | O_EXTRA, 0)) < 0))
{
--no_wait_return;
***************
*** 6320,6326 ****
if (!stuff_empty() || global_busy || !typebuf_typed()
#ifdef FEAT_AUTOCMD
! || autocmd_busy || curbuf_lock > 0
#endif
)
need_check_timestamps = TRUE; /* check later */
--- 6348,6354 ----
if (!stuff_empty() || global_busy || !typebuf_typed()
#ifdef FEAT_AUTOCMD
! || autocmd_busy || curbuf_lock > 0 || allbuf_lock > 0
#endif
)
need_check_timestamps = TRUE; /* check later */
***************
*** 6522,6529 ****
--- 6550,6559 ----
set_vim_var_string(VV_FCS_REASON, (char_u *)reason, -1);
set_vim_var_string(VV_FCS_CHOICE, (char_u *)"", -1);
# endif
+ ++allbuf_lock;
n = apply_autocmds(EVENT_FILECHANGEDSHELL,
buf->b_fname, buf->b_fname, FALSE, buf);
+ --allbuf_lock;
busy = FALSE;
if (n)
{
*** ../vim-7.2.127/src/proto/ex_getln.pro Fri Nov 28 10:59:57 2008
--- src/proto/ex_getln.pro Sun Mar 1 00:27:12 2009
***************
*** 4,9 ****
--- 4,10 ----
int text_locked __ARGS((void));
void text_locked_msg __ARGS((void));
int curbuf_locked __ARGS((void));
+ int allbuf_locked __ARGS((void));
char_u *getexline __ARGS((int c, void *dummy, int indent));
char_u *getexmodeline __ARGS((int promptc, void *dummy, int indent));
int cmdline_overstrike __ARGS((void));
--
Amazing but true: If all the salmon caught in Canada in one year were laid
end to end across the Sahara Desert, the smell would be absolutely awful.
/// Bram Moolenaar -- [email protected] -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ download, build and distribute -- http://www.A-A-P.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
--~--~---------~--~----~------------~-------~--~----~
You received this message from the "vim_dev" maillist.
For more information, visit http://www.vim.org/maillist.php
-~----------~----~----~----~------~----~------~--~---
