[PATCH] Don't override operator new if GDB is built with -fsanitize=address

[PATCH] Don't override operator new if GDB is built with -fsanitize=address

[Yao Qi]

Nowadays, if we build GDB with -fsanitize=address, we can get the asan
error below,

(gdb) quit
=================================================================
==9723==ERROR: AddressSanitizer: alloc-dealloc-mismatch (malloc vs operator delete) on 0x60200003bf70
    #0 0x7f88f3837527 in operator delete(void*) (/usr/lib/x86_64-linux-gnu/libasan.so.1+0x55527)
    #1 0xac8e13 in __gnu_cxx::new_allocator<void (*)()>::deallocate(void (**)(), unsigned long) /usr/include/c++/4.9/ext/new_allocator.h:110
    #2 0xac8cc2 in __gnu_cxx::__alloc_traits<std::allocator<void (*)()> >::deallocate(std::allocator<void (*)()>&, void (**)(), unsigned long) /usr/include/c++/4.9/ext/alloc_traits.h:185
....
0x60200003bf70 is located 0 bytes inside of 8-byte region [0x60200003bf70,0x60200003bf78)
allocated by thread T0 here:
    #0 0x7f88f38367ef in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.1+0x547ef)
    #1 0xbd2762 in operator new(unsigned long) /home/yao/SourceCode/gnu/gdb/git/gdb/common/new-op.c:42
    #2 0xac8edc in __gnu_cxx::new_allocator<void (*)()>::allocate(unsigned long, void const*) /usr/include/c++/4.9/ext/new_allocator.h:104
    #3 0xac8d81 in __gnu_cxx::__alloc_traits<std::allocator<void (*)()> >::allocate(std::allocator<void (*)()>&, unsigned long) /usr/include/c++/4.9/ext/alloc_traits.h:182

The reason for this is that we override operator new but don't override
operator delete.  This patch does the override only if the code is NOT
compiled with asan.

gdb:

2016-10-25  Yao Qi  <yao.qi@linaro.org>

	PR gdb/20716
	* common/new-op.c (__has_feature): New macro.
	Don't override operator new if asan is used.
---
 gdb/common/new-op.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/gdb/common/new-op.c b/gdb/common/new-op.c
index 5ba4d6e..f04c5cb 100644
--- a/gdb/common/new-op.c
+++ b/gdb/common/new-op.c
@@ -17,6 +17,12 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
+/* GCC does not understand __has_feature.  */
+#if !defined(__has_feature)
+# define __has_feature(x) 0
+#endif
+
+#if !__has_feature(address_sanitizer) && !defined(__SANITIZE_ADDRESS__)
 #include "common-defs.h"
 #include "host-defs.h"
 #include <new>
@@ -83,3 +89,4 @@ operator new[] (std::size_t sz, const std::nothrow_t&)
 {
   return ::operator new (sz, std::nothrow);
 }
+#endif
-- 
1.9.1

Re: [PATCH] Don't override operator new if GDB is built with -fsanitize=address

[Pedro Alves]

On 10/25/2016 10:21 AM, Yao Qi wrote:
> Nowadays, if we build GDB with -fsanitize=address, we can get the asan
> error below,
> 
> (gdb) quit
> =================================================================
> ==9723==ERROR: AddressSanitizer: alloc-dealloc-mismatch (malloc vs operator delete) on 0x60200003bf70
>     #0 0x7f88f3837527 in operator delete(void*) (/usr/lib/x86_64-linux-gnu/libasan.so.1+0x55527)
>     #1 0xac8e13 in __gnu_cxx::new_allocator<void (*)()>::deallocate(void (**)(), unsigned long) /usr/include/c++/4.9/ext/new_allocator.h:110
>     #2 0xac8cc2 in __gnu_cxx::__alloc_traits<std::allocator<void (*)()> >::deallocate(std::allocator<void (*)()>&, void (**)(), unsigned long) /usr/include/c++/4.9/ext/alloc_traits.h:185
> ....
> 0x60200003bf70 is located 0 bytes inside of 8-byte region [0x60200003bf70,0x60200003bf78)
> allocated by thread T0 here:
>     #0 0x7f88f38367ef in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.1+0x547ef)
>     #1 0xbd2762 in operator new(unsigned long) /home/yao/SourceCode/gnu/gdb/git/gdb/common/new-op.c:42
>     #2 0xac8edc in __gnu_cxx::new_allocator<void (*)()>::allocate(unsigned long, void const*) /usr/include/c++/4.9/ext/new_allocator.h:104
>     #3 0xac8d81 in __gnu_cxx::__alloc_traits<std::allocator<void (*)()> >::allocate(std::allocator<void (*)()>&, unsigned long) /usr/include/c++/4.9/ext/alloc_traits.h:182
> 
> The reason for this is that we override operator new but don't override
> operator delete.  This patch does the override only if the code is NOT
> compiled with asan.
> 
> gdb:
> 
> 2016-10-25  Yao Qi  <yao.qi@linaro.org>
> 
> 	PR gdb/20716
> 	* common/new-op.c (__has_feature): New macro.
> 	Don't override operator new if asan is used.

LGTM.

Thanks,
Pedro Alves

Re: [PATCH] Don't override operator new if GDB is built with -fsanitize=address

[Pedro Alves]

[Re-adding the list.]

On 10/25/2016 11:15 AM, Yao Qi wrote:
> On Tue, Oct 25, 2016 at 10:39 AM, Pedro Alves <palves@redhat.com> wrote:
>>
>> LGTM.
>>
> 
> Patch is pushed in.
> 

How about this follow up?

From e58eb16a5f27b1ee39c45642a80da7364763a07b Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 25 Oct 2016 11:20:03 +0100
Subject: [PATCH] new-op.c: Add comment about -fsanitize=address

gdb/ChangeLog:
2016-10-25  Pedro Alves  <palves@redhat.com>

	* common/new-op.c: Add comment about -fsanitize=address.
---
 gdb/common/new-op.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gdb/common/new-op.c b/gdb/common/new-op.c
index f04c5cb..1eb4f94 100644
--- a/gdb/common/new-op.c
+++ b/gdb/common/new-op.c
@@ -33,6 +33,12 @@
    new-handler function instead (std::set_new_handler) because we want
    to catch allocation errors from within global constructors too.
 +   Skip overriding if building with -fsanitize=address though.
+   Address sanitizer wants to override operator new/delete too in
+   order to detect malloc+delete and new+free mismatches.  Our
+   versions would mask out ASan's, with the result of losing that
+   useful mismatch detection.
+
    Note that C++ implementations could either have their throw
    versions call the nothrow versions (libstdc++), or the other way
    around (clang/libc++).  For that reason, we replace both throw and
-- 
2.5.5

Re: [PATCH] Don't override operator new if GDB is built with -fsanitize=address

[Pedro Alves]

On 10/25/2016 11:38 AM, Pedro Alves wrote:
> How about this follow up?

On 10/25/2016 11:38 AM, Pedro Alves wrote:

> +++ b/gdb/common/new-op.c
> @@ -33,6 +33,12 @@
>     new-handler function instead (std::set_new_handler) because we want
>     to catch allocation errors from within global constructors too.
>  +   Skip overriding if building with -fsanitize=address though.
> +   Address sanitizer wants to override operator new/delete too in

Bah, looks like "edit as new" with Thunderbird messed
up the patch...  Here it is again...

From e58eb16a5f27b1ee39c45642a80da7364763a07b Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 25 Oct 2016 11:20:03 +0100
Subject: [PATCH] new-op.c: Add comment about -fsanitize=address

gdb/ChangeLog:
2016-10-25  Pedro Alves  <palves@redhat.com>

	* common/new-op.c: Add comment about -fsanitize=address.
---
 gdb/common/new-op.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gdb/common/new-op.c b/gdb/common/new-op.c
index f04c5cb..1eb4f94 100644
--- a/gdb/common/new-op.c
+++ b/gdb/common/new-op.c
@@ -33,6 +33,12 @@
    new-handler function instead (std::set_new_handler) because we want
    to catch allocation errors from within global constructors too.
 
+   Skip overriding if building with -fsanitize=address though.
+   Address sanitizer wants to override operator new/delete too in
+   order to detect malloc+delete and new+free mismatches.  Our
+   versions would mask out ASan's, with the result of losing that
+   useful mismatch detection.
+
    Note that C++ implementations could either have their throw
    versions call the nothrow versions (libstdc++), or the other way
    around (clang/libc++).  For that reason, we replace both throw and
-- 
2.5.5

Re: [PATCH] Don't override operator new if GDB is built with -fsanitize=address

[Yao Qi]

On Tue, Oct 25, 2016 at 11:40 AM, Pedro Alves <palves@redhat.com> wrote:
>
> gdb/ChangeLog:
> 2016-10-25  Pedro Alves  <palves@redhat.com>
>
>         * common/new-op.c: Add comment about -fsanitize=address.

Patch is good to me.

-- 
Yao (齐尧)

Re: [PATCH] Don't override operator new if GDB is built with -fsanitize=address

[Pedro Alves]

On 10/25/2016 12:39 PM, Yao Qi wrote:
> On Tue, Oct 25, 2016 at 11:40 AM, Pedro Alves <palves@redhat.com> wrote:
>>
>> gdb/ChangeLog:
>> 2016-10-25  Pedro Alves  <palves@redhat.com>
>>
>>         * common/new-op.c: Add comment about -fsanitize=address.
> 
> Patch is good to me.
> 

Pushed.

Thanks,
Pedro Alves