This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]