Sanitizer builds #5272
Sanitizer builds #5272
Conversation
wolfsage
force-pushed
the
sanitizer-builds
branch
from
2e9a43c to
b9412f7
Compare
There was a problem hiding this comment.
The Perl changes seem correct to me. I'm doing a practical test now and might have questions. The C changes should be reviewed by someone else. (I see Ken has approved.)
Of special interest is the change to build-with-cyruslibs, which adds some more CFLAGS. I believe that @elliefm recently said that we should move from using the environment to adding options to ./configure. It would be good to get her take here. But mostly: 👍🏽
For flags that we think should be the default for all builds, we want to put them in AM_CFLAGS et al. But we don't want these flags for all builds. We could add a I think we'd usually add a detail like this later though, once we've driven it in anger a bit and have some sense of the ergonomics we want. |
There was a problem hiding this comment.
I think this is very important work for this project, thanks for making it happen.
I haven't approved nor requested changes, because I would first like to understand if the couple commits that address specific ubsan/asan issues are required to be part of this PR or just a byproduct of getting the test infrastructure in place. E.g. I'm not sure I agree with adding the aligned (2) attributes, but I don't want to hold back shipping this PR with these nits.
Thanks, this was my plan too. I'd like to get this info into the build using a configure opt or make opt, and have cyr_info reflect it so cassandane can inspect it and do the right thing, but I did not want to fight with automake at this point |
wolfsage
force-pushed
the
sanitizer-builds
branch
2 times, most recently
from
bf41889 to
e8003b7
Compare
|
@elliefm I'd like to merge this if you're okay with it, and we can fix up other things down the line |
There was a problem hiding this comment.
Looks good
I've included a patch in my comment that removes the valgrind suppression for the leak that your wrap_int_parser_nofree change fixes, since we won't need that suppression anymore with the leak fixed. Use it if you want, doesn't need re-review if you do.
If you don't use it, I'll eventually clean up the stale suppression myself anyway next time I think about it. But the history would be nicer if the suppression is removed in the same commit as the leak is fixed :)
CU_syslogMatchEnd stores .re in a pointer for the caller to use in error messages. We can't free that string and then hand it back, so instead switch slmatch to using a buffer and copy strings into it as necessary.
Due to the way search_expr.c is linked in with cunit tests, this variable causes an odr violation that's detected by asan.
You can use this by doing:
export LSAN_OPTIONS="suppressions=leaksanitizer.suppress"
And then running make check.
For AddressSanitizer, you need to compile cyrus with:
-fsanitize=address -static-libasan
in both LDFLAGS and CFLAGS
When testing fatal conditions, wrap_int_parser mallocs prot and b, but the related free() calls never happen because we longjmp away. Split wrap_int_parser into two methods, one which expects to complete fully and can handle cleaning up memory itself, and one where the caller must clean up memory. Then use the caller-version for fatal tests. This keeps LeakSanitizer happy.
Unlike --valgrind, asan and ubsan are baked into the running programs. If they happen to be compiled in, they'll write to the provided log directories, and we can pull out any errors they generate automatically. Note that unfortunately there are some buggy (still?) verisons of clang and/or gcc where the UBSAN_OPTIONS log_path overrides the ASAN_OPTIONS log_path. The effect of this is that tests run with ASan will misreport errors as ubsan errors, but I think we can live with that.
wolfsage
force-pushed
the
sanitizer-builds
branch
from
e8003b7 to
5b6c40c
Compare
Now that parse.testc doesn't leak on fatal errors we don't need to suppress valgrind reports for it!
wolfsage
force-pushed
the
sanitizer-builds
branch
from
40dd6f1 to
93b4734
Compare
This is mostly intended to be used in tandem with https://github.com/cyrusimap/cyrus-docker/pull/29/files but can stand on its own.
If you compile cyrus with
-fsanitize=addressand then runLSAN_OPTIONS=suppressions=leaksanitizer.suppress make checkall tests pass minus those few skipped.If you compile cyrus with
-fsanitize=undefined,make checkwill pass but throw some warnings. To get it to fail, compile with-fsanitize=undefined -fsanitize-undefined-trap-on-erroror usehalt_on_error=1 make check.When running cassandane with such a build, LSAN_OPTIONS and UBSAN_OPTIONS will be configured to send log files somewhere cassandane can see them, and cass will detect/report any detected log entries as failures (similar to how testrunner --valgrind works).
There's lots of work to go to get asan/ubsan tests passing, but this gets the infrastructure in place.
With this, we can add asan/ubsan builds to all CI if we want. Draft here: #5273
Note that ubsan will complain until a lot of things are fixed... we could start with just asan builds and work our way up to getting everything passing with both.