travis: use clang's -fsanitize=address to catch more bugs

The clang address sanitizer is able to catch quite a number of memory-related bugs, such add memory leaks and buffer under/overruns. So, enable the address sanitizer for one openssl and one mbedtls build.

This would have caught the buffer list unittest memory leak that <[email protected]...> wants to fix.

Quoted 6 lines · Steffan Karger

The clang address sanitizer is able to catch quite a number of memory-related bugs, such add memory leaks and buffer under/overruns. So, enable the address sanitizer for one openssl and one mbedtls build.

This would have caught the buffer list unittest memory leak that <[email protected]...> wants to fix.

For reference, I've pushed this commit to my own gh fork. This would be the result based on current master:
https://travis-ci.org/syzzer/openvpn/builds/322839893\

Quoted 6 lines · Steffan Karger

Quoted 5 lines · Steffan Karger

The clang address sanitizer is able to catch quite a number of memory-related bugs, such add memory leaks and buffer under/overruns. So, enable the address sanitizer for one openssl and one mbedtls build.

Quoted 7 lines

Quoted 3 lines

This would have caught the buffer list unittest memory leak that <[email protected]...> wants to fix.

For reference, I've pushed this commit to my own gh fork. This would be the result based on current master:
https://travis-ci.org/syzzer/openvpn/builds/322839893\

your travis build is failing and throwing out quite some "errors".

I personally think we should fix them all before adding the new sanitize flag, otherwise travis will just always fail until we fix them all.

Quoted 16 lines · Antonio Quartulli

Quoted 6 lines · Steffan Karger

Quoted 5 lines · Steffan Karger

The clang address sanitizer is able to catch quite a number of memory-related bugs, such add memory leaks and buffer under/overruns. So, enable the address sanitizer for one openssl and one mbedtls build.

Quoted 7 lines

Quoted 3 lines

This would have caught the buffer list unittest memory leak that <[email protected]...> wants to fix.

For reference, I've pushed this commit to my own gh fork. This would be the result based on current master:
https://travis-ci.org/syzzer/openvpn/builds/322839893\

your travis build is failing and throwing out quite some "errors".

Quoted 3 lines

I personally think we should fix them all before adding the new sanitize flag, otherwise travis will just always fail until we fix them all.

You already ACK'ed the fix in <[email protected]...> ;-)

But yeah, it would be nice to apply that fix before this patch.

The clang address sanitizer is able to catch quite a number of memory-related bugs, such add memory leaks and buffer under/overruns. So, enable the address sanitizer for one openssl and one mbedtls build.

This would have caught the buffer list unittest memory leak that <[email protected]...> wants to fix.

Signed-off-by Steffan Karger <[email protected]...>

---
 .travis.yml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

.travis.yml +2 -3

index 1f669b3..99cd5e2 100644
@@ -33,7 +33,7 @@ matrix:
     - env: SSLLIB="openssl" OPENSSL_VERSION="1.1.0f"
       os: linux
       compiler: gcc
-    - env: SSLLIB="openssl"
+    - env: SSLLIB="openssl" CFLAGS="-fsanitize=address"
       os: linux
       compiler: clang
     - env: SSLLIB="openssl" OPENSSL_VERSION="1.1.0f"

@@ -42,7 +42,7 @@ matrix:
     - env: SSLLIB="mbedtls"
       os: linux
       compiler: gcc
-    - env: SSLLIB="mbedtls"
+    - env: SSLLIB="mbedtls" CFLAGS="-fsanitize=address"
       os: linux
       compiler: clang
     - env: SSLLIB="openssl"
--

2.7.4

------------------------------------------------------------
------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openvpn-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

thank for the patch.

not directly related to the patch itself, but openvpn is shipped with several memory debugging options

--with-mem-check=TYPE   build with debug memory checking

should we get rid of them (in favour of sanitizers) ? or, maybe add to tests instead ?

Hi,

Quoted 8 lines · Илья Шипицин

thank for the patch.

not directly related to the patch itself, but openvpn is shipped with several memory debugging options

--with-mem-check=TYPE   build with debug memory checking

Quoted 2 lines

should we get rid of them (in favour of sanitizers) ? or, maybe add to tests instead ?

I've tried to use those in the past, but found them hard to use. I think they're a remnant of the past. I would personally prefer to get rid of them and use the (nowadays much better) fully external memory checking tools instead (sanitizers, valgrind, etc).

Hi,

Quoted 11 lines · Steffan Karger

Hi,

Quoted 8 lines · Илья Шипицин

thank for the patch.

not directly related to the patch itself, but openvpn is shipped with several memory debugging options

--with-mem-check=TYPE build with debug memory checking

Quoted 7 lines

Quoted 2 lines

should we get rid of them (in favour of sanitizers) ? or, maybe add to tests instead ?

I've tried to use those in the past, but found them hard to use. I think they're a remnant of the past. I would personally prefer to get rid of them and use the (nowadays much better) fully external memory checking tools instead (sanitizers, valgrind, etc).

As Selva pointed out, we don't use --with-mem-check in any test, therefore I am not even sure it still works as expected.

I am also in favour in of getting rid of our custom made check and use compilers provided sanitizers instead (also gcc provides interesting sanitizers, thus everybody should be bale to enable them).

Cheers,

--
Antonio Quartulli

Hi,

Quoted 11 lines · Steffan Karger

Hi,

Quoted 8 lines · Илья Шипицин

thank for the patch.

not directly related to the patch itself, but openvpn is shipped with several memory debugging options

--with-mem-check=TYPE   build with debug memory checking

Quoted 7 lines

Quoted 2 lines

should we get rid of them (in favour of sanitizers) ? or, maybe add to tests instead ?

I've tried to use those in the past, but found them hard to use. I think they're a remnant of the past. I would personally prefer to get rid of them and use the (nowadays much better) fully external memory checking tools instead (sanitizers, valgrind, etc).

As Selva pointed out, we don't use --with-mem-check in any test, therefore I am not even sure it still works as expected.

I am also in favour in of getting rid of our custom made check and use compilers provided sanitizers instead (also gcc provides interesting sanitizers, thus everybody should be bale to enable them).

ACK, makes sense.

Your patch has been applied to the master branch.

Quoted 3 lines · Gert Doering

ACK, makes sense.

Your patch has been applied to the master branch.

Thanks. Can you also cherry-pick this one to release/2.4? Because there the clang builds are now screaming bloody murder (and sometimes failing) because of ccache too.

Quoted 8 lines · Steffan Karger

Quoted 3 lines · Gert Doering

ACK, makes sense.

Your patch has been applied to the master branch.

Thanks. Can you also cherry-pick this one to release/2.4? Because there the clang builds are now screaming bloody murder (and sometimes failing) because of ccache too.

Uh. Which one? *this* one is about -fsanitize-address...?

I assume you want this one:

commit 0c1b9864184e609b68c2bb317ee57317ae12a026

Author: Ilya Shipitsin <[email protected]...>
Date:   Fri Jan 5 00:37:10 2018 +0500

travis-ci: add brew cache, remove ccache

right?

Uh, right. Though this one is useful too ;-)