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

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

[David Malcolm]

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
   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
-- 
1.8.5.3

[PATCH 1/2] fix memory chunk corruption for opts_obstack (PR jit/68446)

[David Malcolm]

There can be multiple gcc_options instances, each with
a call to
  init_options_struct
matched with a call to
  finalize_options_struct
whereas the
  opts_obstack
is a singleton.  Each gcc_options instance can potentially use the
opts_obstack singleton.

r230264 (aka 25faed340686df8d7bb2242dc8d04285976922b6) fixed
a large memory leak (1.2MB) of the opts_obstack, by making
initialization of the opts_obstack be idempotent
(in init_opts_obstack).

This works if we only have one in-process run of the compiler.
Unfortunately this commit broke much of libgccjit's test suite,
which now fails with memory corruption errors.

The root cause of the breakage is that toplev::finalize cleans up the
opts_obstack using:

  obstack_free (opts_obstack, NULL);

Calling obstack_free with NULL leaves an obstack in an uninitialized
state and hence a reinitialization is required;
libiberty/obstacks.texi has:

  Note that if @var{object} is a null pointer, the result is an
  *uninitialized* obstack.  [my emphasis]

Hence opts_obstack reverts to an uninitialized state - but further
calls to initialize it are rejected by the idempotency code in
init_opts_obstack, and we then attempt to allocate from an
uninitialized obstack.

In particular, the obstack's "chunk" field becomes invalid, but isn't
unset. The underlying 64KB chunk(s) are returned to memory_block_pool's
m_blocks linked-list of 64KB free chunks, and they get reused by othe
obstacks e.g. for bitmaps.  However, given that opts_obstack fails
to be reinitialized, opts_obstack.chunks points at a freed chunk.
Hence, on the 2nd iteration of a jit testcase, it gets used to
allocate copies of the options, but this out of a chunk that's being
used by a different memory_block_pool user, so chaos ensues: we have
64KB chunks of memory being erroneously shared between different
memory-pool users.

The following patch removes idempotency from init_opts_obstack, and
replaces the call to init_opts_stack from init_options_struct with
an assert that the singleton opts_stack is already initialized,
adding in the necessary per-compile initialization of opts_stack
(we already have per-compile cleanup).

Or to put it another way, previously, we had this pattern of calls:

  - for each jit compile:
    - toplev:
      - multiple calls to init_options_struct matched with calls to
        finalize_options_struct; the first call to init_options_struct
        idempotently initializes opts_obstack.
      - obstack_free (&opts_obstack, NULL);
(leading to corrupt opts_obstack on the 2nd iteration of jit compilation)

and with this patch we instead have this:

  - for each jit compile:
    - toplev:
      - init_opts_obstack
      - multiple calls to init_options_struct matched with calls to
        finalize_options_struct
      - obstack_free (&opts_obstack, NULL);

(I don't like that opts_obstack is global state, but it seems risky
to try to fix that at this stage).

The patch also adds code to reset save_decoded_options and
save_decoded_options_count when freeing opts_obstack, since these
saved options are allocated from out of opts_obstack and hence
also become invalid when it's freed.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu in
conjunction with the followup patch.

Fixes all of the jit test suite, apart from test-threads.c (which is
broken for a different reason).  I've also verified manually under
Valgrind that this also keeps the fix for the large leak reported in
the PR that motivated r230264).

OK for trunk?  (assuming it bootstraps&regrtests by itself)

gcc/ChangeLog:
	PR jit/68446
	* gcc.c (driver::decode_argv): Add call to
	init_opts_obstack before init_options_struct.
	* opts.c (init_opts_obstack): Remove idempotency.
	(init_options_struct): Replace call to init_opts_obstack
	with a gcc_assert to verify that it has already been called.
	* toplev.c (toplev::main): Add call to init_opts_obstack before
	calls to init_options_struct.
	(toplev::finalize): Move cleanup of opts_obstack next to
	cleanup of save_decoded_options, clearing the latter, and
	save_decoded_options_count.
---
 gcc/gcc.c    |  1 +
 gcc/opts.c   | 14 +++++---------
 gcc/toplev.c |  7 ++++++-
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/gcc/gcc.c b/gcc/gcc.c
index 319a073..c191fde 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -7191,6 +7191,7 @@ driver::decode_argv (int argc, const char **argv)
   global_init_params ();
   finish_params ();
 
+  init_opts_obstack ();
   init_options_struct (&global_options, &global_options_set);
 
   decode_cmdline_options_to_array (argc, argv,
diff --git a/gcc/opts.c b/gcc/opts.c
index 2add158..9437535 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -266,18 +266,12 @@ add_comma_separated_to_vector (void **pvec, const char *arg)
   *pvec = v;
 }
 
-/* Initialize opts_obstack if not initialized.  */
+/* Initialize opts_obstack.  */
 
 void
 init_opts_obstack (void)
 {
-  static bool opts_obstack_initialized = false;
-
-  if (!opts_obstack_initialized)
-    {
-      opts_obstack_initialized = true;
-      gcc_obstack_init (&opts_obstack);
-    }
+  gcc_obstack_init (&opts_obstack);
 }
 
 /* Initialize OPTS and OPTS_SET before using them in parsing options.  */
@@ -287,7 +281,9 @@ init_options_struct (struct gcc_options *opts, struct gcc_options *opts_set)
 {
   size_t num_params = get_num_compiler_params ();
 
-  init_opts_obstack ();
+  /* Ensure that opts_obstack has already been initialized by the time
+     that we initialize any gcc_options instances (PR jit/68446).  */
+  gcc_assert (opts_obstack.chunk_size > 0);
 
   *opts = global_options_init;
 
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 8bab3e5..580c439 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -2050,6 +2050,7 @@ toplev::main (int argc, char **argv)
   /* One-off initialization of options that does not need to be
      repeated when options are added for particular functions.  */
   init_options_once ();
+  init_opts_obstack ();
 
   /* Initialize global options structures; this must be repeated for
      each structure used for parsing options.  */
@@ -2131,11 +2132,15 @@ toplev::finalize (void)
   finalize_options_struct (&global_options);
   finalize_options_struct (&global_options_set);
 
+  /* save_decoded_options uses opts_obstack, so these must
+     be cleaned up together.  */
+  obstack_free (&opts_obstack, NULL);
   XDELETEVEC (save_decoded_options);
+  save_decoded_options = NULL;
+  save_decoded_options_count = 0;
 
   /* Clean up the context (and pass_manager etc). */
   delete g;
   g = NULL;
 
-  obstack_free (&opts_obstack, NULL);
 }
-- 
1.8.5.3

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

[Jeff Law]

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

Re: [PATCH 1/2] fix memory chunk corruption for opts_obstack (PR jit/68446)

[Jakub Jelinek]

On Fri, Jan 15, 2016 at 03:04:33PM -0500, David Malcolm wrote:
> OK for trunk?  (assuming it bootstraps&regrtests by itself)
> 
> gcc/ChangeLog:
> 	PR jit/68446
> 	* gcc.c (driver::decode_argv): Add call to
> 	init_opts_obstack before init_options_struct.
> 	* opts.c (init_opts_obstack): Remove idempotency.
> 	(init_options_struct): Replace call to init_opts_obstack
> 	with a gcc_assert to verify that it has already been called.
> 	* toplev.c (toplev::main): Add call to init_opts_obstack before
> 	calls to init_options_struct.
> 	(toplev::finalize): Move cleanup of opts_obstack next to
> 	cleanup of save_decoded_options, clearing the latter, and
> 	save_decoded_options_count.

Ok.

	Jakub