Running "echoconsole" terminates vim-without-gui

[mss...@gmail.com]

Vim developers,

I am building vim without the gui feature on windows 10 using mingw.   I am seeing many swapfiles left behind when I run the Vim test suite. 

This is apparently caused by the "echoconsole" command that is in the file runtest.vim (the call was introduced in patch 8.2.2638).  I believe this causes the problem because if I bracket that command with a test for "gui_running" the swapfiles are cleaned up as they should be and the tests succeed. 

In addition, after echoconsole displays its arguments on stdout, vim.exe appears to terminate because I then see the command prompt from cmd.exe.  I don't see any indication of an error condition from Vim.  

Adding the if-test prevents the undesired behavior but I'm uncertain if that's the fix you would prefer so I haven't provided a patch and am limiting this note to reporting the problem.

-mike

[Bram Moolenaar]

Strange, I would not expect writing to the console have any effect on
swapfiles. Does this mean Vim crashes? If you are using MinGW, perhaps
you can use gdb to see what happens.

A workaround would be that when not running the GUI make :echoconsole
behave like :echomsg. In eval.c, ex_execute.c, change:

if (eap->cmdidx == CMD_echomsg)

into:

if (eap->cmdidx == CMD_echomsg || (eap->cmdidx == CMD_echoconsole && !gui.in_use))

You may have to add an #ifdef.


--
hundred-and-one symptoms of being an internet addict:
123. You ask the car dealer to install an extra cigarette lighter
on your new car to power your notebook.

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[mss...@gmail.com]

On Monday, April 19, 2021 at 7:53:14 AM UTC-4 Bram Moolenaar wrote:

> I am building vim without the gui feature on windows 10 using mingw. I am
> seeing many swapfiles left behind when I run the Vim test suite.
>
> This is apparently caused by the "echoconsole" command that is in the file
> runtest.vim (the call was introduced in patch 8.2.2638). I believe this
> causes the problem because if I bracket that command with a test for
> "gui_running" the swapfiles are cleaned up as they should be and the tests
> succeed.
>
> In addition, after echoconsole displays its arguments on stdout, vim.exe
> appears to terminate because I then see the command prompt from cmd.exe. I
> don't see any indication of an error condition from Vim.
>
> Adding the if-test prevents the undesired behavior but I'm uncertain if
> that's the fix you would prefer so I haven't provided a patch and am
> limiting this note to reporting the problem.

Strange, I would not expect writing to the console have any effect on
swapfiles. Does this mean Vim crashes? If you are using MinGW, perhaps
you can use gdb to see what happens.

If vim is crashing I see no evidence of it.  Except for the left-behind swapfiles, it appears that vim exiting without error.

 I will investigate further with gdb as you've suggested.

I neglected to mention that I'm building with the "normal" feature set and that I see the same behavior if I build with the MSYS2 distribution.

A workaround would be that when not running the GUI make :echoconsole
behave like :echomsg. In eval.c, ex_execute.c, change:

if (eap->cmdidx == CMD_echomsg)

into:

if (eap->cmdidx == CMD_echomsg || (eap->cmdidx == CMD_echoconsole && !gui.in_use))

You may have to add an #ifdef.

I can't speak to the change you've suggested above but I think it is the right approach as opposed to just "fixing" the test which is why I did not propose a patch.  It seems inappropriate to me to have vim-without-gui printing to stdout. 

[mss...@gmail.com]

Using gdb it turns out that vim is aborting with a segmentation fault.  The traceback is:

   os_win32.c, line 6414:  s[len] = NUL;

   ui.c, line 52:  mch_write(s, len);

   eval.c, line 6161:  ui_write((char_u *)"\r\n", 2, TRUE);

So it appears that mch_write() is attempting to modify a string literal so I'm guessing that the literal is in read-only memory.  To test this, I created a local variable, initialized it to "]r]n"  and passed this as the first argument.  After rebuilding, the problem no longer occurs.

So, I propose the following patch (forgive the poor GoogleGroups formatting):

diff --git a/src/eval.c b/src/eval.c
index 4dbbc4096..abf9406c9 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -6157,8 +6157,9 @@ ex_execute(exarg_T *eap)
        }
        else if (eap->cmdidx == CMD_echoconsole)
        {
+           char_u crlf[] = "\r\n";
            ui_write(ga.ga_data, (int)STRLEN(ga.ga_data), TRUE);
-           ui_write((char_u *)"\r\n", 2, TRUE);
+           ui_write(crlf, 2, TRUE);
        }
        else if (eap->cmdidx == CMD_echoerr)
        {

-mike

[Bram Moolenaar]

> Using gdb it turns out that vim is aborting with a segmentation fault. The
> traceback is:
>
> os_win32.c, line 6414: s[len] = NUL;
> ui.c, line 52: mch_write(s, len);
> eval.c, line 6161: ui_write((char_u *)"\r\n", 2, TRUE);
>
> So it appears that mch_write() is attempting to modify a string literal so
> I'm guessing that the literal is in read-only memory. To test this, I
> created a local variable, initialized it to "]r]n" and passed this as the
> first argument. After rebuilding, the problem no longer occurs.

Thank you for figuring that out!

> So, I propose the following patch (forgive the poor GoogleGroups
> formatting):
>
> diff --git a/src/eval.c b/src/eval.c
> index 4dbbc4096..abf9406c9 100644
> --- a/src/eval.c
> +++ b/src/eval.c
> @@ -6157,8 +6157,9 @@ ex_execute(exarg_T *eap)
> }
> else if (eap->cmdidx == CMD_echoconsole)
> {
> + char_u crlf[] = "\r\n";
> ui_write(ga.ga_data, (int)STRLEN(ga.ga_data), TRUE);
> - ui_write((char_u *)"\r\n", 2, TRUE);
> + ui_write(crlf, 2, TRUE);
> }
> else if (eap->cmdidx == CMD_echoerr)
> {

Not writing the NUL when it's already there should catch more cases.
It's too easy to add another ui_write() with a string literal. I'll
make a patch that solves it this way, please check.

--
Statistics say that you can have a baby per month on average:
Just get nine women pregnant.

[mss...@gmail.com]

I agree with you that not writing the NUL would be better but then why the "len" argument?  Its presence suggests to me that the first argument need not be NUL-byte terminated.

[Bram Moolenaar]

> > Not writing the NUL when it's already there should catch more cases.
> > It's too easy to add another ui_write() with a string literal. I'll
> > make a patch that solves it this way, please check.
>
> I agree with you that not writing the NUL would be better but then why the
> "len" argument? Its presence suggests to me that the first argument need
> not be NUL-byte terminated.

It does not have to be NUL terminated. But when passing a literal
string it will be, and then writing the NUL causes problems.
It would still fail when passing a literal string with a "len" that is
less than the actual length. But that's unlikely to happen.

--
hundred-and-one symptoms of being an internet addict:

129. You cancel your newspaper subscription.