Patch 9.0.0561

[Bram Moolenaar]

Patch 9.0.0561
Problem: When a test gets stuck it just hangs forever.
Solution: Set a timeout of 30 seconds.
Files: src/testdir/runtest.vim


*** ../vim-9.0.0560/src/testdir/runtest.vim 2022-09-23 19:42:27.814236815 +0100
--- src/testdir/runtest.vim 2022-09-23 20:22:57.288376414 +0100
***************
*** 186,191 ****
--- 186,202 ----
let g:func_start = reltime()
endif

+ " Invoked when a test takes too much time.
+ func TestTimeout(id)
+ split test.log
+ call append(line('$'), '')
+ call append(line('$'), 'Test timed out: ' .. g:testfunc)
+ write
+ call add(v:errors, 'Test timed out: ' . g:testfunc)
+
+ cquit! 42
+ endfunc
+
func RunTheTest(test)
let prefix = ''
if has('reltime')
***************
*** 194,199 ****
--- 205,216 ----
endif
echoconsole prefix .. 'Executing ' .. a:test

+ if has('timers')
+ " No test should take longer than 30 seconds. If it takes longer we
+ " assume we are stuck and need to break out.
+ let test_timeout_timer = timer_start(30000, 'TestTimeout')
+ endif
+
" Avoid stopping at the "hit enter" prompt
set nomore

***************
*** 259,264 ****
--- 276,285 ----
endtry
endif

+ if has('timers')
+ call timer_stop(test_timeout_timer)
+ endif
+
" Clear any autocommands and put back the catch-all for SwapExists.
au!
au SwapExists * call HandleSwapExists()
*** ../vim-9.0.0560/src/version.c 2022-09-23 19:42:27.814236815 +0100
--- src/version.c 2022-09-23 20:23:45.416301530 +0100
***************
*** 701,702 ****
--- 701,704 ----
{ /* Add new patch number below this line */
+ /**/
+ 561,
/**/

--
hundred-and-one symptoms of being an internet addict:
146. You experience ACTUAL physical withdrawal symptoms when away
from your 'puter and the net.

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

[Bram Moolenaar]

I wrote:

> Patch 9.0.0561
> Problem: When a test gets stuck it just hangs forever.
> Solution: Set a timeout of 30 seconds.
> Files: src/testdir/runtest.vim

This is very weird. After including this patch a cscope test started
failing. The error message is confusing, pointing to line 599 of
testdir/runtest.vim, which does not exist. Running "make test_csope"
gets the same error every time.

Removing the added line makes the test pass again. However,
commenting-out the lines does not! Thus it's not the commands that
cause the problem, but the added text in the function. Deleting some
text, without changing the number of lines, makes it work OK again.
Weird!

I tried a few other things, but could not find out much more. It does
seem related to the fork/exec that cscope does to run cscope, which
fails because the command name is wrong. Perhaps something with signal
handling?

If someone likes a puzzle, please have a look at this.

> --
> --
> You received this message from the "vim_dev" maillist.
> Do not top-post! Type your reply below the text you are replying to.
> For more information, visit http://www.vim.org/maillist.php
>
> ---
> You received this message because you are subscribed to the Google Groups "vim_dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+u...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/vim_dev/20220923192747.826AC1C063B%40moolenaar.net.

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

152. You find yourself falling for someone you've never seen or hardly
know, but, boy can he/she TYPE!!!!!!

[Dominique Pellé]

Bram Moolenaar wrote:

> I wrote:
>
> > Patch 9.0.0561
> > Problem: When a test gets stuck it just hangs forever.
> > Solution: Set a timeout of 30 seconds.
> > Files: src/testdir/runtest.vim
>
> This is very weird. After including this patch a cscope test started
> failing. The error message is confusing, pointing to line 599 of
> testdir/runtest.vim, which does not exist. Running "make test_csope"
> gets the same error every time.
>
> Removing the added line makes the test pass again. However,
> commenting-out the lines does not! Thus it's not the commands that
> cause the problem, but the added text in the function. Deleting some
> text, without changing the number of lines, makes it work OK again.
> Weird!
>
> I tried a few other things, but could not find out much more. It does
> seem related to the fork/exec that cscope does to run cscope, which
> fails because the command name is wrong. Perhaps something with signal
> handling?
>
> If someone likes a puzzle, please have a look at this.

I see that if_cscope.c uses SIGALARM and `alarm(unsigned int seconds)`
function. If might be related to this issue since patch 9.0.0561 sets
a timeout.

Speaking of cscope, I recall noticing 2 issues:

1) cscope was accessing invalid memory. I created patch
for it, which was merged in the master branch of cscope
git repo but there has not been an official release which
includes it yet. See:

https://sourceforge.net/p/cscope/cscope/ci/b3ab5461f1a02aa0a07a6f50bc2fa4da057193d1/

2) I recall that cscope test failed on macOS M1 (unrelated
to above fix). I did not really understand this well enough,
but I could fix it with this patch:

https://sourceforge.net/u/dominikoeo/cscope/ci/bugfix/failing-vim-cscope-test-on-macOS-Monterey-M1/~/

But was never merged into the master branch of cscope
because it was not clear enough why it fixed vim tests
on macOS M1.

Regards

Dominique

[Bram Moolenaar]

Thanks for thinking about the cause.

> > I wrote:
> >
> > > Patch 9.0.0561
> > > Problem: When a test gets stuck it just hangs forever.
> > > Solution: Set a timeout of 30 seconds.
> > > Files: src/testdir/runtest.vim
> >
> > This is very weird. After including this patch a cscope test started
> > failing. The error message is confusing, pointing to line 599 of
> > testdir/runtest.vim, which does not exist. Running "make test_csope"
> > gets the same error every time.
> >
> > Removing the added line makes the test pass again. However,
> > commenting-out the lines does not! Thus it's not the commands that
> > cause the problem, but the added text in the function. Deleting some
> > text, without changing the number of lines, makes it work OK again.
> > Weird!
> >
> > I tried a few other things, but could not find out much more. It does
> > seem related to the fork/exec that cscope does to run cscope, which
> > fails because the command name is wrong. Perhaps something with signal
> > handling?
> >
> > If someone likes a puzzle, please have a look at this.
>
> I see that if_cscope.c uses SIGALARM and `alarm(unsigned int seconds)`
> function. If might be related to this issue since patch 9.0.0561 sets
> a timeout.

I commented out the use of alarm() but it did not make any difference.

The timer used doesn't use SIGALARM but the elapsed time. And
commenting out the timer_start() call also doesn't make a difference.

> Speaking of cscope, I recall noticing 2 issues:
>
> 1) cscope was accessing invalid memory. I created patch
> for it, which was merged in the master branch of cscope
> git repo but there has not been an official release which
> includes it yet. See:
>
> https://sourceforge.net/p/cscope/cscope/ci/b3ab5461f1a02aa0a07a6f50bc2fa4da057193d1/
>
> 2) I recall that cscope test failed on macOS M1 (unrelated
> to above fix). I did not really understand this well enough,
> but I could fix it with this patch:
>
> https://sourceforge.net/u/dominikoeo/cscope/ci/bugfix/failing-vim-cscope-test-on-macOS-Monterey-M1/~/
>
> But was never merged into the master branch of cscope
> because it was not clear enough why it fixed vim tests
> on macOS M1.

Well, the failing test doesn't actually execute cscope. It sets
'cscopeprg' to an invalid command. The for()/exec() then fails.

I double checked again and can reliable reproduce that with these
comments in src/testdir/runtest.vim the problem happens:

"if has('timers')

" " No test should take longer than 30 seconds. If it takes longer we

" " assume we are stuck and need to break out.

" let test_timeout_timer = timer_start(30000, 'TestTimeout')

"endif

And when deleting some text the problem goes away:

"if has('timers')

"
"
" let test_timeout_timer = timer_start(30000, 'TestTimeout')

"endif

I first thought there was a race condition, but since it reproduces so
reliable I doubt that. It's more likely some kind of memory corruption.
I haven't been able to spot it with valgrind, possibly because the
fork() confuses it. Perhaps you can try with asan.

--
Two sheep in a meadow. One says "baaah". The other says "exactly!".

[Yegappan Lakshmanan]

Hi Bram,

On Sat, Sep 24, 2022 at 2:15 AM Bram Moolenaar <Br...@moolenaar.net> wrote:
>
>
> I wrote:
>
> > Patch 9.0.0561
> > Problem: When a test gets stuck it just hangs forever.
> > Solution: Set a timeout of 30 seconds.
> > Files: src/testdir/runtest.vim
>
> This is very weird. After including this patch a cscope test started
> failing. The error message is confusing, pointing to line 599 of
> testdir/runtest.vim, which does not exist. Running "make test_csope"
> gets the same error every time.
>
> Removing the added line makes the test pass again. However,
> commenting-out the lines does not! Thus it's not the commands that
> cause the problem, but the added text in the function. Deleting some
> text, without changing the number of lines, makes it work OK again.
> Weird!
>
> I tried a few other things, but could not find out much more. It does
> seem related to the fork/exec that cscope does to run cscope, which
> fails because the command name is wrong. Perhaps something with signal
> handling?
>
> If someone likes a puzzle, please have a look at this.
>

I think the changes made in 8.2.4081 (CodeQL reports problem in
if_cscope causing it to fail)
are related to this failure. If I copy the latest runtest.vim file to
the 8.2.4081 version and
run "make test_cscope", I can reproduce the test failure (the test is stuck).

- Yegappan

[Bram Moolenaar]

If you do the same before the patch, then there is no problem?

I believe the main change in this patch is that instead of executing a
shell to run the cscope program it executes cscope directly. It may
change something in signal handling and what happens if the command
fails, as it happens in this test.

This, however, does not explain that making a comment shorter or longer
can make this happen or not...

--
Keep America beautiful. Swallow your beer cans.

[Yegappan Lakshmanan]

Hi Bram,

Yes. Before this patch, test_cscope passess successfully with the
latest runtest.vim.
I also tried adding and removing comments and changing their lengths, it works
properly.

> I believe the main change in this patch is that instead of executing a
> shell to run the cscope program it executes cscope directly. It may
> change something in signal handling and what happens if the command
> fails, as it happens in this test.
>
> This, however, does not explain that making a comment shorter or longer
> can make this happen or not...
>

There are four test functions in that file. Changing the comment
shorter or longer
makes the test exit after running one test. In the other case, it doesn't exit
Vim. So in both cases, the test is not working correctly.

Regards,
Yegappan

[Yegappan Lakshmanan]

Hi Bram,

The following change fixes the test failure:

============================================================
diff --git a/src/if_cscope.c b/src/if_cscope.c
index ccccb518b..96ca8cbf9 100644
--- a/src/if_cscope.c
+++ b/src/if_cscope.c
@@ -959,7 +959,7 @@ err_closing:
}

// run the cscope command
- (void)sprintf(cmd, "%s -dl -f %s", prog, csinfo[i].fname);
+ (void)sprintf(cmd, "/bin/sh -c \"exec %s -dl -f %s\"", prog, csinfo[i].fname);

if (csinfo[i].ppath != NULL)
{
============================================================

Regards,
Yegappan

[Bram Moolenaar]

Yes, that works. But this should probably only be done for Unix.
And we should use vim_snprintf() to prevent any buffer overflow.

I tried to figure out why changing a comment makes it fail or pass, but
could not find a reason. Would require digging deeper...

--
In Joseph Heller's novel "Catch-22", the main character tries to get out of a
war by proving he is crazy. But the mere fact he wants to get out of the war
only shows he isn't crazy -- creating the original "Catch-22".