Running "browse tabnew" command in TUI MacVim crashes Vim

[yegappan]

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

[ychin]

Just reproduced this. Thanks for the report.

[yegappan]

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?

[ichizok]

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

[ichizok]

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

[ychin]

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" 

[ychin]

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.

[yegappan]

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
        {

[yegappan]

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;
 }

[ichizok]

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
        {

[ychin]

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?

[ychin]

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.

[yegappan]

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.

[yegappan]

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

[ychin]

Thanks. Will be merged from upstream.