Loading ...
Sorry, an error occurred while loading the content.

Re: [patch] fixed access to free memory when using some autocommands

Expand Messages
  • Dominique Pelle
    ... ...snip... I applied your patch to Vim-7.2.128. I still observe something wrong after patching (something which I did not think of testing earlier, so the
    Message 1 of 7 , Mar 1, 2009
    • 0 Attachment
      Bram Moolenaar wrote:

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

      ...snip...

      I applied your patch to Vim-7.2.128.

      I still observe something wrong after patching (something
      which I did not think of testing earlier, so the problem
      I see in your patch was also in my patch).

      The bug I see only happens when opening a file in Vim
      which does not exist but for which there is a swap file.

      Steps to reproduce it:

      # Open a file foobar (which does not exist) in vim to create
      # swap file .foobar.swp
      $ rm -f ~/foobar
      $ vim ~/foobar

      # In another terminal...
      $ vim -u NONE
      :au! SwapExists * cd /tmp
      :e ~/foobar

      The following prompt shows up:

      Swap file "~/.foobar.swp" already exists!
      [O]pen Read-Only, (E)dit anyway, (R)ecover, (Q)uit, (A)bort:

      Press e.

      Valgrind then outputs the following error

      ==17742== Invalid read of size 1
      ==17742== at 0x810B4DA: get_past_head (misc1.c:4472)
      ==17742== by 0x810B428: gettail_sep (misc1.c:4419)
      ==17742== by 0x810B610: dir_of_file_exists (misc1.c:4582)
      ==17742== by 0x80C13CD: readfile (fileio.c:606)
      ==17742== by 0x8052D8E: open_buffer (buffer.c:130)
      ==17742== by 0x8097AC3: do_ecmd (ex_cmds.c:3655)
      ==17742== by 0x80ADE20: do_exedit (ex_docmd.c:7557)
      ==17742== by 0x80ADA97: ex_edit (ex_docmd.c:7452)
      ==17742== by 0x80A6517: do_one_cmd (ex_docmd.c:2622)
      ==17742== by 0x80A3D97: do_cmdline (ex_docmd.c:1096)
      ==17742== by 0x8129422: nv_colon (normal.c:5218)
      ==17742== by 0x8122A86: normal_cmd (normal.c:1189)
      ==17742== by 0x80E5F8D: main_loop (main.c:1180)
      ==17742== by 0x80E5ADA: main (main.c:939)
      ==17742== Address 0x4d34be0 is 0 bytes inside a block of size 15 free'd
      ==17742== at 0x4024E5A: free (vg_replace_malloc.c:323)
      ==17742== by 0x8113C72: vim_free (misc2.c:1631)
      ==17742== by 0x80C8E58: shorten_fnames (fileio.c:5743)
      ==17742== by 0x80AE6AA: ex_cd (ex_docmd.c:7939)
      ==17742== by 0x80A6517: do_one_cmd (ex_docmd.c:2622)
      ==17742== by 0x80A3D97: do_cmdline (ex_docmd.c:1096)
      ==17742== by 0x80CC9A5: apply_autocmds_group (fileio.c:8883)
      ==17742== by 0x80CC3A8: apply_autocmds (fileio.c:8494)
      ==17742== by 0x80F9592: do_swapexists (memline.c:3770)
      ==17742== by 0x80F9EAD: findswapname (memline.c:4130)
      ==17742== by 0x80F3FF8: ml_open_file (memline.c:553)
      ==17742== by 0x80F411E: check_need_swap (memline.c:605)
      ==17742== by 0x80C13BF: readfile (fileio.c:605)
      ==17742== by 0x8052D8E: open_buffer (buffer.c:130)
      ==17742== by 0x8097AC3: do_ecmd (ex_cmds.c:3655)
      ==17742== by 0x80ADE20: do_exedit (ex_docmd.c:7557)
      ==17742== by 0x80ADA97: ex_edit (ex_docmd.c:7452)
      ==17742== by 0x80A6517: do_one_cmd (ex_docmd.c:2622)
      ==17742== by 0x80A3D97: do_cmdline (ex_docmd.c:1096)
      ==17742== by 0x8129422: nv_colon (normal.c:5218)
      ==17742== by 0x8122A86: normal_cmd (normal.c:1189)
      ==17742== by 0x80E5F8D: main_loop (main.c:1180)
      ==17742== by 0x80E5ADA: main (main.c:939)
      (and several other errors follow)

      If I enter ":messages" I also do not see the error
      message E812 (I would expect to see this message).

      I do not see this issue when the file foobar already exists.

      This remaining issue happens because call to
      check_need_swap() in fileio.c:605 (or line 592 in
      pristine vim-7.2.128) may trigger an autocommand
      which can also invalidate fname.

      Attached patch (on top of your patch) fixes this
      remaining issue (and a typo). I will continue to test
      with this patch for a while but so far it looks OK.
      I can't see any further bugs so far with autocommands.

      Regards
      Dominique

      --~--~---------~--~----~------------~-------~--~----~
      You received this message from the "vim_dev" maillist.
      For more information, visit http://www.vim.org/maillist.php
      -~----------~----~----~----~------~----~------~--~---
    • Bram Moolenaar
      ... Thanks, I ll include this as well. When I test the steps you gave here I now get E811 for the :cd command. And other nastyness should be caught by the
      Message 2 of 7 , Mar 1, 2009
      • 0 Attachment
        Dominique Pelle wrote:

        > Bram Moolenaar wrote:
        >
        > > 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.
        >
        > ...snip...
        >
        > I applied your patch to Vim-7.2.128.
        >
        > I still observe something wrong after patching (something
        > which I did not think of testing earlier, so the problem
        > I see in your patch was also in my patch).
        >
        > The bug I see only happens when opening a file in Vim
        > which does not exist but for which there is a swap file.
        >
        > Steps to reproduce it:
        >
        > # Open a file foobar (which does not exist) in vim to create
        > # swap file .foobar.swp
        > $ rm -f ~/foobar
        > $ vim ~/foobar
        >
        > # In another terminal...
        > $ vim -u NONE
        > :au! SwapExists * cd /tmp
        > :e ~/foobar
        >
        > The following prompt shows up:
        >
        > Swap file "~/.foobar.swp" already exists!
        > [O]pen Read-Only, (E)dit anyway, (R)ecover, (Q)uit, (A)bort:
        >
        > Press e.
        >
        > Valgrind then outputs the following error
        >
        > ==17742== Invalid read of size 1
        > ==17742== at 0x810B4DA: get_past_head (misc1.c:4472)
        > ==17742== by 0x810B428: gettail_sep (misc1.c:4419)
        > ==17742== by 0x810B610: dir_of_file_exists (misc1.c:4582)
        > ==17742== by 0x80C13CD: readfile (fileio.c:606)
        > ==17742== by 0x8052D8E: open_buffer (buffer.c:130)
        > ==17742== by 0x8097AC3: do_ecmd (ex_cmds.c:3655)
        > ==17742== by 0x80ADE20: do_exedit (ex_docmd.c:7557)
        > ==17742== by 0x80ADA97: ex_edit (ex_docmd.c:7452)
        > ==17742== by 0x80A6517: do_one_cmd (ex_docmd.c:2622)
        > ==17742== by 0x80A3D97: do_cmdline (ex_docmd.c:1096)
        > ==17742== by 0x8129422: nv_colon (normal.c:5218)
        > ==17742== by 0x8122A86: normal_cmd (normal.c:1189)
        > ==17742== by 0x80E5F8D: main_loop (main.c:1180)
        > ==17742== by 0x80E5ADA: main (main.c:939)
        > ==17742== Address 0x4d34be0 is 0 bytes inside a block of size 15 free'd
        > ==17742== at 0x4024E5A: free (vg_replace_malloc.c:323)
        > ==17742== by 0x8113C72: vim_free (misc2.c:1631)
        > ==17742== by 0x80C8E58: shorten_fnames (fileio.c:5743)
        > ==17742== by 0x80AE6AA: ex_cd (ex_docmd.c:7939)
        > ==17742== by 0x80A6517: do_one_cmd (ex_docmd.c:2622)
        > ==17742== by 0x80A3D97: do_cmdline (ex_docmd.c:1096)
        > ==17742== by 0x80CC9A5: apply_autocmds_group (fileio.c:8883)
        > ==17742== by 0x80CC3A8: apply_autocmds (fileio.c:8494)
        > ==17742== by 0x80F9592: do_swapexists (memline.c:3770)
        > ==17742== by 0x80F9EAD: findswapname (memline.c:4130)
        > ==17742== by 0x80F3FF8: ml_open_file (memline.c:553)
        > ==17742== by 0x80F411E: check_need_swap (memline.c:605)
        > ==17742== by 0x80C13BF: readfile (fileio.c:605)
        > ==17742== by 0x8052D8E: open_buffer (buffer.c:130)
        > ==17742== by 0x8097AC3: do_ecmd (ex_cmds.c:3655)
        > ==17742== by 0x80ADE20: do_exedit (ex_docmd.c:7557)
        > ==17742== by 0x80ADA97: ex_edit (ex_docmd.c:7452)
        > ==17742== by 0x80A6517: do_one_cmd (ex_docmd.c:2622)
        > ==17742== by 0x80A3D97: do_cmdline (ex_docmd.c:1096)
        > ==17742== by 0x8129422: nv_colon (normal.c:5218)
        > ==17742== by 0x8122A86: normal_cmd (normal.c:1189)
        > ==17742== by 0x80E5F8D: main_loop (main.c:1180)
        > ==17742== by 0x80E5ADA: main (main.c:939)
        > (and several other errors follow)
        >
        > If I enter ":messages" I also do not see the error
        > message E812 (I would expect to see this message).
        >
        > I do not see this issue when the file foobar already exists.
        >
        > This remaining issue happens because call to
        > check_need_swap() in fileio.c:605 (or line 592 in
        > pristine vim-7.2.128) may trigger an autocommand
        > which can also invalidate fname.
        >
        > Attached patch (on top of your patch) fixes this
        > remaining issue (and a typo). I will continue to test
        > with this patch for a while but so far it looks OK.
        > I can't see any further bugs so far with autocommands.

        Thanks, I'll include this as well.

        When I test the steps you gave here I now get E811 for the ":cd"
        command. And other nastyness should be caught by the check added by
        this patch. So that's good.

        --
        Fingers not found - Pound head on keyboard to continue.

        /// Bram Moolenaar -- Bram@... -- 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
        -~----------~----~----~----~------~----~------~--~---
      Your message has been successfully submitted and would be delivered to recipients shortly.