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

PATCH In review Steffan Karger 5 participants 15 messages Dec 29, 2017 – Jan 11, 2018

Share & tools

Steffan Karger

author

2017-12-29 09:47 UTC #0 lore raw

PATCH 1/1 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.

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

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

.travis.yml +2 -2

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"

Signature · 1 line

2.7.4

Steffan Karger

author

2017-12-29 10:00 UTC #1 lore raw

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

-Steffan

Antonio Quartulli

2017-12-29 10:32 UTC #2 lore raw

Hi,

Quoted 11 lines · Steffan Karger

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\

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.

Cheers,

Signature · 1 line

Antonio Quartulli

Steffan Karger

author

2017-12-29 10:58 UTC #3 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Quoted 15 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\

Quoted 5 lines

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.

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

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

- -Steffan

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJaRh/bAAoJEEEwndWOY1w5kFQH/iJuCx02eIhNexq9o24aYJa4 rFSuOYwM4Qf+AE+ufa6RmMWZd94Ppud3mpoJ/Gpy0wWe3MivmtWWRIBzgQzGZpqG tpcnJLwBWtxQGL6ZUFyrjNy1QxGRMPFM/dk8fqFW2mwWlmbRzuRxiRpDZf6DYg3d Lr508W+kSJEDmzk4PCFwyocMNSOSTMbRuOwhd9Urf9y4noptD7PD9aLz9Xr70bOh pTIymTT3aUKIlGKPdgk5zyFPJJuM+MBgHLnt6o6kyDC5BDWtUFkuitVPxAj5yiMG W5sQJheXoDKLXuHwDFFBV3XB2p+JPm1NcH96RVbh9EAsHrAwyJba9kWtNFO5+X4= =v9s+
-----END PGP SIGNATURE-----

Antonio Quartulli

2017-12-29 11:00 UTC #4 lore raw

Hi,

Quoted 18 lines · Steffan Karger

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 8 lines

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.

woops, I didn't realize it was only that one :)
Even better!

Cheers,

Signature · 1 line

Antonio Quartulli

Илья Шипицин

2017-12-29 10:11 UTC #5 lore raw

In reply to Steffan Karger

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 ?

2017-12-29 14:47 GMT+05:00 Steffan Karger <[email protected]...>:

Quoted 47 lines

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

Steffan Karger

author

2017-12-29 10:23 UTC #6 lore raw

Re: [Openvpn-devel] --with-mem-check, what to do with it?

Hi,

Quoted 10 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
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).

-Steffan

Antonio Quartulli

2017-12-29 11:02 UTC #7 lore raw

Re: [Openvpn-devel] --with-mem-check, what to do with it?

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,

Signature · 1 line

Antonio Quartulli

Илья Шипицин

2017-12-29 11:16 UTC #8 lore raw

Re: [Openvpn-devel] --with-mem-check, what to do with it?

2017-12-29 16:02 GMT+05:00 Antonio Quartulli <[email protected]...>:

Quoted 22 lines

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).

Quoted 7 lines

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).

I played with those sanitizers, they are nice. But memory sanitizers are noisy on openssl.
seem to need some work (either resolving issues on openssl side or muting them)

Quoted 7 lines

Cheers,
--
Antonio Quartulli

David Sommerseth

2017-12-29 21:14 UTC #9 lore raw

Re: [Openvpn-devel] --with-mem-check, what to do with it?

In reply to Antonio Quartulli

Quoted 22 lines · 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).

Quoted 7 lines

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).

When compiling with --with-mem-check=valgrind ... a lot of the noise from OpenSSL (and other external issues) vanishes when running OpenVPN via valgrind. So I find that feature very useful. It's really handy to use this approach when checking for memory leaks in plug-ins.

There might be better checks, granted. But not sure how well they will play with plug-ins. The valgrind type works fine. Not sure how well the other --with-mem-check alternatives (dmalloc, ssl) works, so can't speak for them. If they don't build or work, then we should consider to remove what doesn't work.

Signature · 4 lines

kind regards,

David Sommerseth
OpenVPN Inc

Gert Doering

2018-01-09 14:31 UTC #10 lore raw

In reply to Steffan Karger

ACK, makes sense.

Your patch has been applied to the master branch.

commit 7b11915ddfe97d8c28f998db54c40384a4eafb93

Author: Steffan Karger
Date:   Fri Dec 29 10:47:37 2017 +0100

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

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg16102.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Steffan Karger

author

2018-01-11 13:55 UTC #11 lore raw

Hi,

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.

-Steffan

Gert Doering

2018-01-11 14:01 UTC #12 lore raw

Hi,

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?

gert

Signature · 3 lines

now what should I write here...

Gert Doering - Munich, Germany                             [email protected]...

Steffan Karger

author

2018-01-11 14:42 UTC #13 lore raw

Quoted 15 lines · Gert Doering

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

Quoted 8 lines

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 ;-)

-Steffan

Gert Doering

2018-01-11 16:13 UTC #14 lore raw

Hi,

Quoted 1 line · Steffan Karger

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

ok, so here's 2.4 updates...

commit b968f43f9142f832483ae91c8b4e44a5c9efed57 (HEAD -> release/2.4)

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

travis-ci: add brew cache, remove ccache
(cherry picked from commit 0c1b9864184e609b68c2bb317ee57317ae12a026)

commit a5dc64653e261a233cc9df0b5adc46767b544645 (HEAD -> release/2.4)

Author: Steffan Karger <[email protected]...>
Date:   Fri Dec 29 10:47:37 2017 +0100

travis: use clang's -fsanitize=address to catch more bugs (cherry picked from commit 7b11915ddfe97d8c28f998db54c40384a4eafb93)

(cherry-picked from master, no code changes)

gert

Signature · 3 lines

now what should I write here...

Gert Doering - Munich, Germany                             [email protected]...