Skip to content

Running "browse tabnew" command in TUI MacVim crashes Vim #1107

Closed
Closed
Running "browse tabnew" command in TUI MacVim crashes Vim#1107
Milestone
snapshot-167

Description

@yegappan
yegappan
opened on Oct 11, 2020

Describe the bug
When running the TUI MacVim, executing the command 'browse tabnew' crashes Vim.

To Reproduce
Detailed steps to reproduce the behavior:

$ cd macvim/src
$ MacVim/build/Release/MacVim.app/Contents/MacOS/Vim
:browse tabnew
Vim: Caught deadly signal BUS
Vim: Finished.
Bus error: 10

I don't have any Vim plugins. But I couldn't reproduce this problem with --clean.

Expected behavior
The command should fail with the E338 error.

Environment (please complete the following information):

  • Vim version 8.2.1805
  • OS: macOS 10.15.7
  • Terminal: MacOS Terminal app

Activity

  1. ychin commented on Oct 11, 2020

    @ychin
    ychin
    on Oct 11, 2020
    Member

    Just reproduced this. Thanks for the report.

  2. ychin
    added this to the snapshot-167 milestone on Oct 11, 2020

  3. yegappan commented on Oct 11, 2020

    @yegappan
    yegappan
    on Oct 11, 2020
    ContributorAuthor

    The following commands also crash Vim: "browse enew", "browse split",
    "browse edit", "browse view", "browse vsplit", "browse tabedit", "browse diffsplit",
    "browse open", "browse visual", "browse vsplit".

    Most probably the fix is the same for all of these commands.

    The backtrace for one of the crashes is:

    * thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=2, address=0x100290fc8)
  * frame #0: 0x00007fff6848b9c6 libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell + 198
    frame #1: 0x00007fff6848c074 libsystem_platform.dylib`_platform_strcpy + 84
    frame #2: 0x000000010011e3f0 Vim`fname_case + 384
    frame #3: 0x000000010006c3a5 Vim`do_ecmd + 453
    frame #4: 0x000000010007b467 Vim`do_exedit + 503
    frame #5: 0x0000000100074e8a Vim`do_cmdline + 11450
    frame #6: 0x00000001000fc92e Vim`nv_colon + 126
    frame #7: 0x00000001000f861c Vim`normal_cmd + 6220
    frame #8: 0x0000000100277111 Vim`main_loop + 1841
    frame #9: 0x00000001002765c2 Vim`vim_main2 + 3666
    frame #10: 0x0000000100274f1e Vim`main + 6846
    frame #11: 0x00007fff68295cc9 libdyld.dylib`start + 1

    I am not able to build MacVim with debug symbols enabled and load it in lldb.
    I set CFLAGS to "-g -O0" in Makefile and built Vim. Still the generated Vim
    binary under the MacVim/build/Release/MacVim.app/Contents/MacOS
    directory didn't have the debug symbols.

    I am able to build the regular Vim with debug symbols enabled on MacOS, Linux
    and MS-Windows. How do I build MacVim with debug symbols enabled, load it
    in lldb and step through the code?

  4. ichizok commented on Oct 12, 2020

    @ichizok
    ichizok
    on Oct 12, 2020
    Member

    It can reproduce in Linux and other OS by defining USE_FNAME_CASE on build (with GUI), thus it appears an upstream problem.

  5. ichizok commented on Oct 12, 2020

    @ichizok
    ichizok
    on Oct 12, 2020
    Member

    Conditions that causes the problem:

    • has +browse feature (e.g. built with GUI)
    • augroup FileExplorer is defined (e.g. loaded netrw plugin)
    • USE_FNAME_CASE macro is defined (e.g. on macOS, Windows)
    • run on terminal

  6. ychin commented on Oct 12, 2020

    @ychin
    ychin
    on Oct 12, 2020
    Member

    Regarding building for debugging, @yegappan , see https://github.com/macvim-dev/macvim/wiki/Building. In particular, this should work:

    make "CFLAGS=-O0 -g" XCODEFLAGS="-configuration Debug"

  7. ychin commented on Oct 12, 2020

    @ychin
    ychin
    on Oct 12, 2020
    Member

    Just looking at older builds, seems like most of them suffer the same issue. I think this is a pretty old bug. At least 1 year old.

  8. yegappan commented on Oct 12, 2020

    @yegappan
    yegappan
    on Oct 12, 2020
    ContributorAuthor

    This issue is caused by fname_case() trying to copy a string to
    a read-only string. The relevant lines are 2514 in ex_cmds.c and
    6139 in ex_docmd.c.

    If I use vim_strsave() to create a copy of the "." string, then the
    crash is not seen. But there is a memory leak now. I need to
    come up with a fix to free this memory.

    The patch is below:

    diff --git a/src/ex_cmds.c b/src/ex_cmds.c
index c61f57533..e2396c8c5 100644
--- a/src/ex_cmds.c
+++ b/src/ex_cmds.c
@@ -2511,7 +2511,7 @@ do_ecmd(
                // No browsing supported but we do have the file explorer:
                // Edit the directory.
                if (ffname == NULL || !mch_isdir(ffname))
-                   ffname = (char_u *)".";
+                   ffname = vim_strsave((char_u *)".");
            }
            else
            {
diff --git a/src/ex_docmd.c b/src/ex_docmd.c
index bc01b69c7..4ab0b0a6f 100644
--- a/src/ex_docmd.c
+++ b/src/ex_docmd.c
@@ -6117,7 +6117,7 @@ ex_splitview(exarg_T *eap)
            // No browsing supported but we do have the file explorer:
            // Edit the directory.
            if (*eap->arg == NUL || !mch_isdir(eap->arg))
-               eap->arg = (char_u *)".";
+               eap->arg = vim_strsave((char_u *)".");
        }
        else
        {

  9. yegappan commented on Oct 13, 2020

    @yegappan
    yegappan
    on Oct 13, 2020
    ContributorAuthor

    An updated patch that fixes this problem without a memory leak is:

    diff --git a/src/ex_cmds.c b/src/ex_cmds.c
index c61f57533..436614b7a 100644
--- a/src/ex_cmds.c
+++ b/src/ex_cmds.c
@@ -2481,6 +2481,9 @@ do_ecmd(
     int                readfile_flags = 0;
     int                did_inc_redrawing_disabled = FALSE;
     long        *so_ptr = curwin->w_p_so >= 0 ? &curwin->w_p_so : &p_so;
+#ifdef USE_FNAME_CASE
+    int                free_sfname = FALSE;
+#endif
 
 #ifdef FEAT_PROP_POPUP
     if (ERROR_IF_TERM_POPUP_WINDOW)
@@ -2528,7 +2531,12 @@ do_ecmd(
            sfname = ffname;
 #ifdef USE_FNAME_CASE
        if (sfname != NULL)
-           fname_case(sfname, 0);   // set correct case for sfname
+       {
+           sfname = vim_strsave(sfname);
+           free_sfname = TRUE;
+           if (sfname != NULL)
+               fname_case(sfname, 0);   // set correct case for sfname
+       }
 #endif
 
        if ((flags & ECMD_ADDBUF) && (ffname == NULL || *ffname == NUL))
@@ -3147,6 +3155,10 @@ theend:
     vim_free(browse_file);
 #endif
     vim_free(free_fname);
+#ifdef USE_FNAME_CASE
+    if (free_sfname)
+       vim_free(sfname);
+#endif
     return retval;
 }

  10. ichizok commented on Oct 13, 2020

    @ichizok
    ichizok
    on Oct 13, 2020
    Member

    Maybe it can use stack, I think because:
    The return value of do_browse() in the opposite side block is finally freed in the end of the function.
    Thus, also the values of eap->arg and ffname just have to exist only during the function.

    --- a/src/ex_cmds.c
+++ b/src/ex_cmds.c
@@ -2465,6 +2465,7 @@ do_ecmd(
     bufref_T   old_curbuf;
     char_u     *free_fname = NULL;
 #ifdef FEAT_BROWSE
+    char_u     dot_path[] = ".";
     char_u     *browse_file = NULL;
 #endif
     int                retval = FAIL;
@@ -2511,7 +2512,7 @@ do_ecmd(
                // No browsing supported but we do have the file explorer:
                // Edit the directory.
                if (ffname == NULL || !mch_isdir(ffname))
-                   ffname = (char_u *)".";
+                   ffname = dot_path;
            }
            else
            {
--- a/src/ex_docmd.c
+++ b/src/ex_docmd.c
@@ -6084,6 +6084,7 @@ ex_splitview(exarg_T *eap)
     char_u     *fname = NULL;
 #endif
 #ifdef FEAT_BROWSE
+    char_u     dot_path[] = ".";
     int                browse_flag = cmdmod.browse;
 #endif
     int                use_tab = eap->cmdidx == CMD_tabedit
@@ -6136,7 +6137,7 @@ ex_splitview(exarg_T *eap)
            // No browsing supported but we do have the file explorer:
            // Edit the directory.
            if (*eap->arg == NUL || !mch_isdir(eap->arg))
-               eap->arg = (char_u *)".";
+               eap->arg = dot_path;
        }
        else
        {

  11. ychin commented on Oct 13, 2020

    @ychin
    ychin
    on Oct 13, 2020
    Member

    I like @ichizok's version, and it also removes the insidious const cast here (which cast "." from const char_u * to char_u *) which is the root source of the bug.

    Can we move the discussion to https://github.com/vim/vim though?

  12. ychin commented on Oct 13, 2020

    @ychin
    ychin
    on Oct 13, 2020
    Member

    Also, seems like there is a coverage gap here in that no test is exercising :browse. But maybe even with a test this wouldn't have been caught as it relies on certain settings to be set.

  13. yegappan commented on Oct 13, 2020

    @yegappan
    yegappan
    on Oct 13, 2020
    ContributorAuthor

    I agree. We can go with the changes from @ichizok. Can you create a PR in the upstream Vim repository?

    It will be difficult to create a test for this as this needs a GUI Vim running in a Terminal on MS-Windows
    or MacOS. The GUI build for MacOS doesn't work with the upstream Vim source.

  14. yegappan commented on Oct 13, 2020

    @yegappan
    yegappan
    on Oct 13, 2020
    ContributorAuthor

    This issue has now been fixed in the upstream by 8.2.1842.

  15. ychin commented on Oct 13, 2020

    @ychin
    ychin
    on Oct 13, 2020
    Member

    Thanks. Will be merged from upstream.

  16. ychin
    closed this as completedon Oct 13, 2020
  17. ychin
    added 2 commits that reference this issue on Dec 11, 2020

    MacVim Snapshot 167

    ee71f93

    MacVim Snapshot 168

    f539a14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions