Message ID | 1452888274-46515-2-git-send-email-dmalcolm@redhat.com |
---|---|
State | New |
Headers | show |
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®rtesting 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
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 block->m_next = instance.m_blocks; instance.m_blocks = block; } diff --git a/libiberty/obstack.c b/libiberty/obstack.c index 6d8d672..8df5517 100644 --- a/libiberty/obstack.c +++ b/libiberty/obstack.c @@ -292,6 +292,11 @@ _obstack_free (struct obstack *h, void *obj) else if (obj != 0) /* obj is not in any of the chunks! */ abort (); + + /* If OBJ is zero, the obstack will require reinitialization; poison it. + TODO: make this conditional on being a debug build. */ + if (obj == 0) + memset (h, 0xdd, sizeof (struct obstack)); } _OBSTACK_SIZE_T