Re: [HACKERS] LLVM Address Sanitizer (ASAN) and valgrind support

2016-10-23 Thread Noah Misch
On Thu, Oct 20, 2016 at 06:14:28PM +0100, Greg Stark wrote:
> On Oct 20, 2016 5:27 PM, "Noah Misch"  wrote:
> > On Wed, Oct 19, 2016 at 11:08:39AM +0100, Greg Stark wrote:
> > > The MEMPOOL_FREE doesn't take any size argument and mcxt.c doesn't
> > > have convenient access to a size argument. It could call the
> > > GetChunkSpace method but that will include the allocation overhead and
> >
> > That is indeed a problem for making VALGRIND_MEMPOOL_FREE() an alias of
> > VALGRIND_MAKE_MEM_NOACCESS() under ASAN as I suggested.  Calling
> > GetMemoryChunkSpace() in the macro would cause memdebug.h to embed an
> > assumption of mcxt.c, which is messy.  Including the allocation overhead is
> > fine, though.
> 
> I think the way out is to simply have aset.c make the memory
> undefined/noaccess even if it's redundant under valgrind. It's a bit
> unfortunate that the macros would have different semantics under the two.
> If it's too confusing or we're worried about the performance overhead we
> could make a MAKE_MEM_{UNDEFINED,NOACCESS}_IF_NO_MEMPOOL() but I don't
> think it's worth it myself.

I don't expect much performance overhead.  When I last benchmarked Valgrind
Memcheck of "make installcheck", a !USE_VALGRIND build (no client requests at
all) saved about 5% of runtime.  A single new client request should be cheap
enough.  (Marking otherwise-redundant calls may still be good, though.)

> There are a couple build oddities both with gcc and clang before I can
> commit anything though. And I can't test valgrind to be sure the redundant
> calls aren't causing a problem.

When you submit your patch to a CommitFest, mention that you're blocked on
having a reviewer who can test Valgrind.  Many reviewers can help.

Thanks,
nm


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] LLVM Address Sanitizer (ASAN) and valgrind support

2016-10-20 Thread Greg Stark
On Oct 20, 2016 5:27 PM, "Noah Misch"  wrote:
>
> On Wed, Oct 19, 2016 at 11:08:39AM +0100, Greg Stark wrote:
>
> > The MEMPOOL_FREE doesn't take any size argument and mcxt.c doesn't
> > have convenient access to a size argument. It could call the
> > GetChunkSpace method but that will include the allocation overhead and
>
> That is indeed a problem for making VALGRIND_MEMPOOL_FREE() an alias of
> VALGRIND_MAKE_MEM_NOACCESS() under ASAN as I suggested.  Calling
> GetMemoryChunkSpace() in the macro would cause memdebug.h to embed an
> assumption of mcxt.c, which is messy.  Including the allocation overhead
is
> fine, though.

I think the way out is to simply have aset.c make the memory
undefined/noaccess even if it's redundant under valgrind. It's a bit
unfortunate that the macros would have different semantics under the two.
If it's too confusing or we're worried about the performance overhead we
could make a MAKE_MEM_{UNDEFINED,NOACCESS}_IF_NO_MEMPOOL() but I don't
think it's worth it myself.

> > in any case isn't this memory already marked noaccess by aset.c?
>
> Only sometimes, when AllocSetFree() happens to call free() or wipe_mem().

I think if I did further surgery here I would rename wipe_mem and
randomise_mem and make them responsible for making the memory undefined and
noaccess as well. They would always be defined so that would get rid of all
the ifdefs except within those functions.

I have a patch working under asan on both gcc and clang. That handles
noaccess but not undefined memory reads. I need to try msan to make sure
the macro definitions work well for that API too.

There are a couple build oddities both with gcc and clang before I can
commit anything though. And I can't test valgrind to be sure the redundant
calls aren't causing a problem.


Re: [HACKERS] LLVM Address Sanitizer (ASAN) and valgrind support

2016-10-20 Thread Noah Misch
On Wed, Oct 19, 2016 at 11:08:39AM +0100, Greg Stark wrote:
> On Sat, Feb 6, 2016 at 4:52 AM, Noah Misch  wrote:
> > aset.c relies on the fact that VALGRIND_MEMPOOL_ALLOC() has an implicit
> > VALGRIND_MAKE_MEM_UNDEFINED() and VALGRIND_MEMPOOL_FREE() has an implicit
> > VALGRIND_MAKE_MEM_NOACCESS().  #define those two accordingly.  If ASAN has 
> > no
> 
> Actually this is confusing.
> 
> aset.c doesn't actually use the MEMPOOL_ALLOC macro at all, it just
> calls UNDEFINED, DEFINED, and NOACCESS. mcxt.c does however do the
> MEMPOOL_ALLOC/FREE.

Correct.

> So both layers are calling these macros for
> overlapping memory areas which I find very confusing and I'm not sure
> what the net effect is.

The net effect looks like this, at the instant an mcxt.c function returns:

NOACCESS:
- requested_size field of AllocChunkData
- Trailing padding, if any, of AllocChunkData
- Any chunk bytes after the last byte of the most recent requested size

DEFINED:
- palloc0() return value, up to requested size

UNDEFINED:
- palloc() return value, up to requested size
- repalloc() new portion, after size increase (with MEMORY_CONTEXT_CHECKING
  disabled, memory unfortunately becomes DEFINED instead)

> The MEMPOOL_FREE doesn't take any size argument and mcxt.c doesn't
> have convenient access to a size argument. It could call the
> GetChunkSpace method but that will include the allocation overhead and

That is indeed a problem for making VALGRIND_MEMPOOL_FREE() an alias of
VALGRIND_MAKE_MEM_NOACCESS() under ASAN as I suggested.  Calling
GetMemoryChunkSpace() in the macro would cause memdebug.h to embed an
assumption of mcxt.c, which is messy.  Including the allocation overhead is
fine, though.

> in any case isn't this memory already marked noaccess by aset.c?

Only sometimes, when AllocSetFree() happens to call free() or wipe_mem().


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] LLVM Address Sanitizer (ASAN) and valgrind support

2016-10-19 Thread Greg Stark
On Sat, Feb 6, 2016 at 4:52 AM, Noah Misch  wrote:
> aset.c relies on the fact that VALGRIND_MEMPOOL_ALLOC() has an implicit
> VALGRIND_MAKE_MEM_UNDEFINED() and VALGRIND_MEMPOOL_FREE() has an implicit
> VALGRIND_MAKE_MEM_NOACCESS().  #define those two accordingly.  If ASAN has no

Actually this is confusing.

aset.c doesn't actually use the MEMPOOL_ALLOC macro at all, it just
calls UNDEFINED, DEFINED, and NOACCESS. mcxt.c does however do the
MEMPOOL_ALLOC/FREE. So both layers are calling these macros for
overlapping memory areas which I find very confusing and I'm not sure
what the net effect is.

The MEMPOOL_FREE doesn't take any size argument and mcxt.c doesn't
have convenient access to a size argument. It could call the
GetChunkSpace method but that will include the allocation overhead and
in any case isn't this memory already marked noaccess by aset.c?

-- 
greg


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] LLVM Address Sanitizer (ASAN) and valgrind support

2016-09-28 Thread Greg Stark
On Wed, Sep 28, 2016 at 7:40 AM, Piotr Stefaniak
 wrote:
> Not remembering the context, I was initially confused about what exactly
> supposedly needs to be done in order to have ASan support, especially
> since I've been using it for a couple of years without any kind of
> modifications. Having read the whole thread now, I assume the discussion
> is now about getting MSan support, since apparently it's been already
> concluded that not much is needed for getting ASan support:


The differnce between msan and asan is only related to whether
uninitialized reads are tracked. All other memory errors such as
reading past the end of an allocation or reading after a free are
tracked by both.

Without asan support in Postgres's memdebug.h asan will just track
whether you're using memory that is outside the memory that malloc has
handed Postgres. It doesn't know anything about whether that memory
has been returned by palloc or has since been pfree'd. Even the bounds
checking is not great since you could be reading from palloc's header
or from the bytes in the next palloc block that happened to be
returned by the same malloc (or another malloc if you're unlucky).

The support I added to memdebug.h called macros which call llvm
intrinsics to mark the memory malloc'd by Postgres as unusuable until
it's returned by palloc. Once it's returned by palloc it's marked
usable except for a "guard" byte at the end. Then pfree marks the
memory unusable again. This basically mimics the behaviour you would
get from asan if you were using malloc directly.

I added support for msan as well which is basically just one more
macro to make the distinction between usable but uninitialized memory
and usable and initialized memory. But I was unable to test it because
msan didn't work for me at all. This seems to be the way of things
with llvm. It's great stuff but there's always 10% that is broken
because there's some cool new thing that's better.

-- 
greg


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] LLVM Address Sanitizer (ASAN) and valgrind support

2016-09-28 Thread Piotr Stefaniak
On 2016-09-28 00:02, Andres Freund wrote:
> On 2015-09-07 17:05:10 +0100, Greg Stark wrote:
>> I feel like I remember hearing about this before but I can't find any
>> mention of it in my mail archives. It seems pretty simple to add
>> support for LLVM's Address Sanitizer (asan) by using the hooks we
>> already have for valgrind.
>
> Any plans to pick this up again?

Not remembering the context, I was initially confused about what exactly 
supposedly needs to be done in order to have ASan support, especially 
since I've been using it for a couple of years without any kind of 
modifications. Having read the whole thread now, I assume the discussion 
is now about getting MSan support, since apparently it's been already 
concluded that not much is needed for getting ASan support:

>> I don't even see any need offhand for a configure flag or autoconf
>> test. We could have a configure flag just to be consistent with
>> valgrind but it seems pointless. If you're compiling with asan I don't
>> see any reason to not use it. I'm building this to see if it works
>> now.
>
> I agree.  A flag guards Valgrind client requests, because we'd otherwise have
> no idea whether the user plans to run the binary under Valgrind.  For ASAN,
> all relevant decisions happen at build time.

Please correct me if I'm wrong.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] LLVM Address Sanitizer (ASAN) and valgrind support

2016-09-27 Thread Andres Freund
On 2016-09-27 19:31:57 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2016-09-28 00:23:11 +0100, Greg Stark wrote:
> >> I would love to remove all the #ifdef's and have the
> >> macros just be no-ops if they're compiled out for example...
> 
> > Don't we pretty much have that?
> 
> I think "((void) 0)" is a more common spelling of a dummy statement.
> Though there's little wrong with it as it is.

That's already committed code, I didn't propose to add it ;)


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] LLVM Address Sanitizer (ASAN) and valgrind support

2016-09-27 Thread Tom Lane
Andres Freund  writes:
> On 2016-09-28 00:23:11 +0100, Greg Stark wrote:
>> I would love to remove all the #ifdef's and have the
>> macros just be no-ops if they're compiled out for example...

> Don't we pretty much have that?

I think "((void) 0)" is a more common spelling of a dummy statement.
Though there's little wrong with it as it is.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] LLVM Address Sanitizer (ASAN) and valgrind support

2016-09-27 Thread Andres Freund
On 2016-09-28 00:23:11 +0100, Greg Stark wrote:
> On Tue, Sep 27, 2016 at 11:02 PM, Andres Freund  wrote:
> > Any plans to pick this up again?
> 
> Yeah, I was just thinking I should pick this up again.
> 
> > I vote for renaming the VALGRIND names etc. to something more tool-neutral. 
> > I think it's going to be too confusing otherwise.
> 
> Hm, the danger there is once I start refactoring things I could get
> bogged down...

Meh, adding a neutral name seems pretty harmless.

> I would love to remove all the #ifdef's and have the
> macros just be no-ops if they're compiled out for example...

Don't we pretty much have that?

#ifdef USE_VALGRIND
#include 
#else
#define VALGRIND_CHECK_MEM_IS_DEFINED(addr, size)   do {} 
while (0)
#define VALGRIND_CREATE_MEMPOOL(context, redzones, zeroed)  do {} while (0)
#define VALGRIND_DESTROY_MEMPOOL(context)   
do {} while (0)
#define VALGRIND_MAKE_MEM_DEFINED(addr, size)   do {} 
while (0)
#define VALGRIND_MAKE_MEM_NOACCESS(addr, size)  do {} 
while (0)
#define VALGRIND_MAKE_MEM_UNDEFINED(addr, size) do {} 
while (0)
#define VALGRIND_MEMPOOL_ALLOC(context, addr, size) do {} 
while (0)
#define VALGRIND_MEMPOOL_FREE(context, addr)do {} 
while (0)
#define VALGRIND_MEMPOOL_CHANGE(context, optr, nptr, size)  do {} while (0)
#endif

?


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] LLVM Address Sanitizer (ASAN) and valgrind support

2016-09-27 Thread Greg Stark
On Tue, Sep 27, 2016 at 11:02 PM, Andres Freund  wrote:
> Any plans to pick this up again?

Yeah, I was just thinking I should pick this up again.

> I vote for renaming the VALGRIND names etc. to something more tool-neutral. I 
> think it's going to be too confusing otherwise.

Hm, the danger there is once I start refactoring things I could get
bogged down... I would love to remove all the #ifdef's and have the
macros just be no-ops if they're compiled out for example...

-- 
greg


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] LLVM Address Sanitizer (ASAN) and valgrind support

2016-09-27 Thread Andres Freund
Hi,


On 2015-09-07 17:05:10 +0100, Greg Stark wrote:
> I feel like I remember hearing about this before but I can't find any
> mention of it in my mail archives. It seems pretty simple to add
> support for LLVM's Address Sanitizer (asan) by using the hooks we
> already have for valgrind.

Any plans to pick this up again?


> In fact I think this would actually be sufficient. I'm not sure what
> the MEMPOOL valgrind stuff is though. I don't think it's relevant to
> address sanitizer which only tracks references to free'd or
> unallocated pointers.

It'd be nice to add msan support, so uninitialized accesses are also
tracked. (oh, you suggest that below)


I vote for renaming the VALGRIND names etc. to something more
tool-neutral. I think it's going to be too confusing otherwise.


Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] LLVM Address Sanitizer (ASAN) and valgrind support

2016-02-05 Thread Noah Misch
On Mon, Sep 07, 2015 at 05:05:10PM +0100, Greg Stark wrote:
> I feel like I remember hearing about this before but I can't find any
> mention of it in my mail archives. It seems pretty simple to add
> support for LLVM's Address Sanitizer (asan) by using the hooks we
> already have for valgrind.

Nice.

> In fact I think this would actually be sufficient. I'm not sure what
> the MEMPOOL valgrind stuff is though. I don't think it's relevant to
> address sanitizer which only tracks references to free'd or
> unallocated pointers.

Documentation: http://valgrind.org/docs/manual/mc-manual.html#mc-manual.mempools

A Valgrind memory pool is the precise analog of a PostgreSQL memory context.
The design of the PostgreSQL Valgrind support gave mcxt.c primary
responsibility for validating and invalidating pointers, and mcxt.c uses
MEMPOOL client requests to do so.  While Valgrind could catch all the same
errors with just VALGRIND_MAKE_MEM_{DEFINED,NOACCESS,UNDEFINED}, the MEMPOOL
client requests improve the attribution of errors.  You get this:

==00:00:14:08.472 471537==  Address 0x8ecd848 is 24 bytes inside a block of 
size 25 client-defined
==00:00:14:08.472 471537==at 0x7717FD: MemoryContextAlloc (mcxt.c:589)

Instead of this:

==00:00:02:29.587 18940==  Address 0x6a1b51b is 83,835 bytes inside a block of 
size 262,144 alloc'd
==00:00:02:29.587 18940==at 0x4C2353F: malloc (vg_replace_malloc.c:236)
==00:00:02:29.587 18940==by 0x7FD12F: AllocSetAlloc (aset.c:792)
==00:00:02:29.587 18940==by 0x7FEE9D: MemoryContextAlloc (mcxt.c:524)

> I don't even see any need offhand for a configure flag or autoconf
> test. We could have a configure flag just to be consistent with
> valgrind but it seems pointless. If you're compiling with asan I don't
> see any reason to not use it. I'm building this to see if it works
> now.

I agree.  A flag guards Valgrind client requests, because we'd otherwise have
no idea whether the user plans to run the binary under Valgrind.  For ASAN,
all relevant decisions happen at build time.

> --- a/src/include/utils/memdebug.h
> +++ b/src/include/utils/memdebug.h
> @@ -19,6 +19,27 @@
> 
>  #ifdef USE_VALGRIND
>  #include 
> +
> +#elif __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__)
> +
> +#include 
> +
> +#define VALGRIND_MAKE_MEM_DEFINED(addr, size) \
> + ASAN_UNPOISON_MEMORY_REGION(addr, size)
> +
> +#define VALGRIND_MAKE_MEM_NOACCESS(addr, size) \
> + ASAN_POISON_MEMORY_REGION(addr, size)
> +
> +#define VALGRIND_MAKE_MEM_UNDEFINED(addr, size) \
> + ASAN_UNPOISON_MEMORY_REGION(addr, size)
> +
> +#define VALGRIND_CHECK_MEM_IS_DEFINED(addr, size) do {} while (0)
> +#define VALGRIND_CREATE_MEMPOOL(context, redzones, zeroed) do {} while (0)
> +#define VALGRIND_DESTROY_MEMPOOL(context) do {} while (0)
> +#define VALGRIND_MEMPOOL_ALLOC(context, addr, size) do {} while (0)
> +#define VALGRIND_MEMPOOL_FREE(context, addr) do {} while (0)
> +#define VALGRIND_MEMPOOL_CHANGE(context, optr, nptr, size) do {} while (0)

aset.c relies on the fact that VALGRIND_MEMPOOL_ALLOC() has an implicit
VALGRIND_MAKE_MEM_UNDEFINED() and VALGRIND_MEMPOOL_FREE() has an implicit
VALGRIND_MAKE_MEM_NOACCESS().  #define those two accordingly.  If ASAN has no
equivalent of MEMPOOL client requests, it's fine to stub out the rest.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers