[2/2] RFE: poisoning of invalid memory blocks and obstacks

It was difficult to track down the memory corruption bug fixed by the
previous patch (PR jit/68446).  The following patch attempts to make
it easier to find that kind of thing by adding "poisoning" code:

(A) when memory blocks are returned to the memory_block_pool's free
    list (e.g. by an obstack), fill the content with a garbage value.

(B) When calling
      obstack_free (obstack, NULL);
    which leaves the obstack requiring reinitialization, fill
    the obstack's fields with a garbage value.

in both cases to try fail faster for use-after-free errors.

This patch isn't ready as-is:
- I couldn't see an equivalent of CHECKING_P for libiberty, so
  case (B) would do it even in a production build.

- this interracts badly with Valgrind; the latter emits messages
  about "Invalid write of size 8"
	"16 bytes inside a block of size 65,536 alloc'd"
  I think that it merely needs some extra uses of the valgrind
  annotation macros to fix.

- the garbage/poison values I picked were rather arbitrary

That said, it's survived bootstrap&regrtesting on x86_64-pc-linux-gnu
(in conjunction with the previous patch).

Thoughts?

gcc/ChangeLog:
	* memory-block.h (memory_block_pool::release): If CHECKING_P,
	fill the released block with a poison value.

libiberty/ChangeLog:
	* obstack.c (_obstack_free): If OBJ is zero, poison the
	obstack to highlight the need for reinitialization.
---
 gcc/memory-block.h  | 3 +++
 libiberty/obstack.c | 5 +++++
 2 files changed, 8 insertions(+)

On 01/15/2016 01:04 PM, David Malcolm wrote:
> It was difficult to track down the memory corruption bug fixed by the
> previous patch (PR jit/68446).  The following patch attempts to make
> it easier to find that kind of thing by adding "poisoning" code:
>
> (A) when memory blocks are returned to the memory_block_pool's free
>      list (e.g. by an obstack), fill the content with a garbage value.
>
> (B) When calling
>        obstack_free (obstack, NULL);
>      which leaves the obstack requiring reinitialization, fill
>      the obstack's fields with a garbage value.
>
> in both cases to try fail faster for use-after-free errors.
>
> This patch isn't ready as-is:
> - I couldn't see an equivalent of CHECKING_P for libiberty, so
>    case (B) would do it even in a production build.
>
> - this interracts badly with Valgrind; the latter emits messages
>    about "Invalid write of size 8"
> 	"16 bytes inside a block of size 65,536 alloc'd"
>    I think that it merely needs some extra uses of the valgrind
>    annotation macros to fix.
>
> - the garbage/poison values I picked were rather arbitrary
>
> That said, it's survived bootstrap&regrtesting on x86_64-pc-linux-gnu
> (in conjunction with the previous patch).
>
> Thoughts?
>
> gcc/ChangeLog:
> 	* memory-block.h (memory_block_pool::release): If CHECKING_P,
> 	fill the released block with a poison value.
>
> libiberty/ChangeLog:
> 	* obstack.c (_obstack_free): If OBJ is zero, poison the
> 	obstack to highlight the need for reinitialization.
> ---
>   gcc/memory-block.h  | 3 +++
>   libiberty/obstack.c | 5 +++++
>   2 files changed, 8 insertions(+)
>
> diff --git a/gcc/memory-block.h b/gcc/memory-block.h
> index d7b96a3..52c17f9 100644
> --- a/gcc/memory-block.h
> +++ b/gcc/memory-block.h
> @@ -66,6 +66,9 @@ inline void
>   memory_block_pool::release (void *uncast_block)
>   {
>     block_list *block = new (uncast_block) block_list;
> +#if CHECKING_P
> +  memset (block, 0xde, block_size);
> +#endif
Is there some reason this isn't if (flag_checking) instead of a #if?

As you note, we don't currently have the concept of checking mode for 
libiberty.  If obstacks weren't opaque, we could wrap obstack_free 
inside GCC and handle poising there.

We'll definitely want the valgrind annotations.

This feels like something we should add during gcc7's cycle.  Note that 
we may not be the canonical sources for obstacks -- I'm really not sure 
on that one.

jeff