Improve ASAN CI testing #1425
Closed
Improve ASAN CI testing #1425
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dpelle
reviewed
Jan 29, 2017
.travis.yml
Outdated
| @@ -19,8 +19,8 @@ env: | |||
| - BUILD=yes TEST=test COVERAGE=no FEATURES=huge SHADOWOPT= SRCDIR=./src CHECK_AUTOCONF=no | |||
| "CONFOPT='--enable-perlinterp --enable-pythoninterp --enable-rubyinterp --enable-luainterp'" | |||
| # ASAN build | |||
| - BUILD=yes TEST=test SANITIZER_CFLAGS="-g -O1 -fsanitize=address -fno-omit-frame-pointer" FEATURES=huge SRCDIR=./src CHECK_AUTOCONF=no | |||
| "CONFOPT='--enable-perlinterp --enable-pythoninterp --enable-rubyinterp --enable-luainterp'" | |||
| - BUILD=yes TEST=test SANITIZER_CFLAGS="-g -O1 -DEXITFREE -fsanitize=address -fno-omit-frame-pointer" FEATURES=huge SRCDIR=./src CHECK_AUTOCONF=no | |||
There was a problem hiding this comment.
We could also consider compiling with -DABORT_ON_INTERNAL_ERROR in CI to make internal errors more obvious.
There was a problem hiding this comment.
Good suggestion. Added.
|
This is going to fail due to a buffer overrun introduced in v8.0.0250 |
|
James McCoy wrote:
This is going to fail due to a buffer overrun introduced in [v8.0.0250]
Don't see how that patch could introduce this problem. Perhaps this was
so far hidden because the column was zero?
You can perhaps try this in getvcol(), it's not a solution but would
explain why it didn't fail before:
#ifdef FEAT_MBYTE
if (has_mbyte && pos->col > 0)
/* always start on the first byte */
posptr -= (*mb_head_off)(line, posptr);
#endif
Valgrind doesn't report this problem.
How can we see what test caused the problem?
…--
Q: What's orange and sounds like a parrot?
A: A carrot
/// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
|
This is not needed. A simpler solution to detect the asan failure in CI is to remove It also has other advantages:
Without this change (removal of dash), all tests can succeed despite I'm going to create a PR with the dash removal to check that works as intended |
|
Dominique Pellé wrote:
> Configure ASAN to print the stack traces and write the logs out to files
This is not needed. A simpler solution to detect the asan failure in CI is to remove
the dash in src/testdir/Makefile as follows:
```
diff --git a/src/testdir/Makefile b/src/testdir/Makefile
index 08f11cd..bb0249a 100644
--- a/src/testdir/Makefile
+++ b/src/testdir/Makefile
@@ -78,7 +78,7 @@ test1.out: test1.in
# 200 msec is sufficient, but only modern sleep supports a fraction of
# a second, fall back to a second if it fails.
@-/bin/sh -c "sleep .2 > /dev/null 2>&1 || sleep 1"
- -$(RUN_VIM) $*.in
+ $(RUN_VIM) $*.in
# For flaky tests retry one time. No tests at the moment.
#@/bin/sh -c "if test -f test.out -a $* = test61; then \
@@ -108,7 +108,7 @@ bench_re_freeze.out: bench_re_freeze.vim
# 200 msec is sufficient, but only modern sleep supports a fraction of
# a second, fall back to a second if it fails.
@-/bin/sh -c "sleep .2 > /dev/null 2>&1 || sleep 1"
- -$(RUN_VIM) $*.in
+ $(RUN_VIM) $*.in
@/bin/sh -c "if test -f benchmark.out; then cat benchmark.out; fi"
nolog:
```
It also has other advantages:
- If asan fails, it prints the error immediately so we know which test fails
- it also makes CI fail if a regular (non-asan) build was to crash when
vim exits (as a result of bug with -DEXITFREE or other reasons).
Without this change (removal of dash), all tests can succeed despite
vim crashing on exit, which is bad.
I'm going to create a PR with the dash removal to check that works as intended
on all plaforms.
Doesn't this mean the first test that fails causes an abort, no further
tests are executed? That's bad, if several tests fail we need to know.
…--
Michael: There is no such thing as a dump question.
Bernard: Sure there is. For example "what is a core dump?"
/// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
|
If a test fails, VIMRUN still return 0 normally. So all tests will run. But if VIMRUN returns something else than 0 (as a result of of a crash for In any cases, it's better than currently saying "all tests passed" despite the |
|
I think you meant to close #1427 with 8.0.0272, not this PR. |
|
Sorry, Dominique's comment was in this thread. |
jamessan
force-pushed
the
use-EXITFREE-with-ASAN
branch
from
d5a9af9
to
89a9002
Compare
|
Rebased to include Dominique's changes and that shows the ASAN failure is happening when running test7.in. The suggested change to check |
|
I see the same global-buffer-overflow reported by @jamessan, when building Code in memline.c: I don't understand yet what's causing the overflow. valgrind won't detect this kind of bugs, as only asan detects overflows https://github.com/google/sanitizers/wiki/AddressSanitizerComparisonOfMemoryTools Full error reported by asan in test7, with vim-8.0.303: I also see that overflow in 2 other tests:
|
|
Adding some printf, I sort of see what's going on with the asan error, Function ml_get_buf() is called and returns an empty constant That ml_get_buf() calls came from getvcol() at charset.c:1294: So 'line' pointer at charset.c:1294 points to a global Then at charset.c:1999, we add pos->col, which happens to The error from asan is thus correct, but a misleading because it |
|
Dominique Pellé wrote:
I see the same global-buffer-overflow reported by @jamessan, when building
vim-8.0.303 with -DEXITFREE and asan. My asan output is a bit better than
the one posted earlier, as I see line number for globals:
```
0x000000d21d36 is located 42 bytes to the left of global variable '<string literal>' defined in 'memline.c:1153:6' (0xd21d60) of size 3
'<string literal>' is ascii string '.s'
0x000000d21d36 is located 21 bytes to the right of global variable '<string literal>' defined in 'memline.c:1147:20' (0xd21d20) of size 1
'<string literal>' is ascii string ''
```
Code in memline.c:
```
1141 /*
1142 * If the file name ends in ".s[uvw][a-z]" we assume this is the swap file.
1143 * Otherwise a search is done to find the swap file(s).
1144 */
1145 fname = curbuf->b_fname;
1146 if (fname == NULL) /* When there is no file name */
!!1147 fname = (char_u *)"";
1148 len = (int)STRLEN(fname);
1149 if (len >= 4 &&
1150 #if defined(VMS)
1151 STRNICMP(fname + len - 4, "_s" , 2)
1152 #else
!!1153 STRNICMP(fname + len - 4, ".s" , 2)
1154 #endif
```
I don't understand yet what's causing the overflow.
I first thought the code in memline.c is unrelated, that we just end up
with a pointer somewhere in a random position. However, all three cases
you found point around the same area in memory. That's a hint.
Looking around, I now relalize that we first clear all buffers, before
calling free_all_mem() and closing the windows. I think that we hit
this line in ml_get_buf():
if (buf->b_ml.ml_mfp == NULL) /* there are no lines */
return (char_u *)"";
So we get an empty string, always.
Ah, now I also read your other message, you come to the same conclusion.
valgrind won't detect this kind of bugs, as only asan detects overflows
in global variables as described in this comparison page:
https://github.com/google/sanitizers/wiki/AddressSanitizerComparisonOfMemoryTools
Full error reported by asan in test7, with vim-8.0.303:
```
VIMRUNTIME=../../runtime; export VIMRUNTIME; ../vim -f -u unix.vim -U NONE --noplugin --not-a-term -s dotest.in test7.in
=================================================================
==5875==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000000d21d36 at pc 0x000000890a51 bp 0x7ffe71d46860 sp 0x7ffe71d46858
READ of size 1 at 0x000000d21d36 thread T0
#0 0x890a50 in utf_head_off /home/pel/sb/vim/src/mbyte.c:3746
#1 0xc7818a in getvcol /home/pel/sb/vim/src/charset.c:1303
#2 0xc7a082 in getvvcol /home/pel/sb/vim/src/charset.c:1477
#3 0x87a65a in curs_columns /home/pel/sb/vim/src/move.c:964
#4 0xc33915 in scroll_to_fraction /home/pel/sb/vim/src/window.c:5827
#5 0xc1e20b in win_new_height /home/pel/sb/vim/src/window.c:5711
#6 0xc1f625 in frame_new_height /home/pel/sb/vim/src/window.c:2862
#7 0xc2842d in winframe_remove /home/pel/sb/vim/src/window.c:2673
Since getvcol calls ml_get_buf() without a problem, wp->w_buffer and
pos->lnum are probably valid. However, we probably get the empty string
here. If the cursor column is non-zero then in this line we get an
invalid pointer:
posptr = ptr + pos->col;
If you want to try, check the text length first:
if (pos->col > STRLEN(ptr))
wrong!
To avoid even getting there, I think that when exiting we can skip
scroll_to_fraction(). I'll make a patch for that.
There can still be more places where the empty string causes problems,
please look out for that.
…--
A programmer's wife asks him: "Please run to the store and pick up a loaf of
bread. If they have eggs, get a dozen". The programmer comes home with 12
loafs of bread.
/// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
|
jamessan
force-pushed
the
use-EXITFREE-with-ASAN
branch
from
89a9002
to
eb4796c
Compare
|
Rebased on 8.0.0413. The ASAN failures that were detected before have been fixed, so this should be good to merge now. |
chrisbra
pushed a commit
to chrisbra/vim
that referenced
this pull request
Mar 25, 2017
Problem: Crash on exit is not detected when running tests. Solution: Remove the dash before the command. (Dominique Pelle, closes vim#1425)
chrisbra
pushed a commit
to chrisbra/vim
that referenced
this pull request
Mar 25, 2017
Problem: ASAN logs are disabled and don't cause a failure.
Solution: Enable ASAN logs and fail if not empty. (James McCoy,
closes vim#1425)
desvp
pushed a commit
to desvp/vim
that referenced
this pull request
May 30, 2017
Problem: Crash on exit is not detected when running tests. Solution: Remove the dash before the command. (Dominique Pelle, closes vim#1425)
desvp
pushed a commit
to desvp/vim
that referenced
this pull request
May 30, 2017
Problem: ASAN logs are disabled and don't cause a failure.
Solution: Enable ASAN logs and fail if not empty. (James McCoy,
closes vim#1425)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
-DEXITFREEon the CI builds which perform ASAN testing