[PATCH] Fix PR80721

[PATCH] Fix PR80721

[Richard Biener]

It was pointed out by Markus that the EH emergency pool is not
kept sorted and fully merged properly for the cases of freeing
an entry before the first free entry and for the cases where
merging with the immediate successor and for the case with
merging with both successor and predecessor is possible.

The following patch attempts to fix this.

Bootstrap and regtest running on x86_64-unknown-linux-gnu.

Ok for trunk?  (given low / close to no testing coverage
extra close eyes wanted!)

Reporter says maybe it can't happen in real-life as it requires
EH deallocation order not be the reverse of allocation order.
I don't know enough here for a quick guess but "in C++ everything
is possible" ;)

Thanks,
Richard.

2017-05-12  Richard Biener  <rguenther@suse.de>
	Markus Eisenmann  <meisenmann.lba@fh-salzburg.ac.at>

	PR libstdc++/80721
	* libsupc++/eh_alloc.cc (pool::free): Keep list properly
	sorted and add missing freelist item merging cases.

Index: libstdc++-v3/libsupc++/eh_alloc.cc
===================================================================
--- libstdc++-v3/libsupc++/eh_alloc.cc	(revision 247951)
+++ libstdc++-v3/libsupc++/eh_alloc.cc	(working copy)
@@ -194,13 +194,17 @@ namespace
       allocated_entry *e = reinterpret_cast <allocated_entry *>
 	(reinterpret_cast <char *> (data) - offsetof (allocated_entry, data));
       std::size_t sz = e->size;
-      if (!first_free_entry)
+      if (!first_free_entry
+	  || (reinterpret_cast <char *> (e) + sz
+	      < reinterpret_cast <char *> (first_free_entry)))
 	{
-	  // If the free list is empty just put the entry there.
+	  // If the free list is empty or the entry is before the
+	  // first element and cannot be merged with it add it as
+	  // the first free entry.
 	  free_entry *f = reinterpret_cast <free_entry *> (e);
 	  new (f) free_entry;
 	  f->size = sz;
-	  f->next = NULL;
+	  f->next = first_free_entry;
 	  first_free_entry = f;
 	}
       else if (reinterpret_cast <char *> (e) + sz
@@ -224,9 +228,17 @@ namespace
 		   > reinterpret_cast <char *> (e) + sz);
 	       fe = &(*fe)->next)
 	    ;
+	  // If we can merge the next block into us do so and continue
+	  // with the cases below.
+	  if (reinterpret_cast <char *> (e) + sz
+	      == reinterpret_cast <char *> ((*fe)->next))
+	    {
+	      sz += (*fe)->next->size;
+	      (*fe)->next = (*fe)->next->next;
+	    }
 	  if (reinterpret_cast <char *> (*fe) + (*fe)->size
 	      == reinterpret_cast <char *> (e))
-	    /* Merge with the freelist entry.  */
+	    // Merge with the freelist entry.
 	    (*fe)->size += sz;
 	  else
 	    {

Re: [PATCH] Fix PR80721

[Jonathan Wakely]

On 12/05/17 12:10 +0200, Richard Biener wrote:
>
>It was pointed out by Markus that the EH emergency pool is not
>kept sorted and fully merged properly for the cases of freeing
>an entry before the first free entry and for the cases where
>merging with the immediate successor and for the case with
>merging with both successor and predecessor is possible.
>
>The following patch attempts to fix this.
>
>Bootstrap and regtest running on x86_64-unknown-linux-gnu.
>
>Ok for trunk?  (given low / close to no testing coverage
>extra close eyes wanted!)
>
>Reporter says maybe it can't happen in real-life as it requires
>EH deallocation order not be the reverse of allocation order.
>I don't know enough here for a quick guess but "in C++ everything
>is possible" ;)

It's definitely possible.

std::exception_ptr p1, p2;
try {
  throw 1;
} catch (...) {
  p1 = std::current_exception();
}
try {
  throw 2;
} catch (...) {
  p2 = std::current_exception();
}

Both exceptions are still active, and their lifetimes are now tied to
value objects which can be reset, swapped, copied etc. so their
lifetime is now arbitrary.

Re: [PATCH] Fix PR80721

[Jonathan Wakely]

On 12/05/17 12:10 +0200, Richard Biener wrote:
>
>It was pointed out by Markus that the EH emergency pool is not
>kept sorted and fully merged properly for the cases of freeing
>an entry before the first free entry and for the cases where
>merging with the immediate successor and for the case with
>merging with both successor and predecessor is possible.
>
>The following patch attempts to fix this.
>
>Bootstrap and regtest running on x86_64-unknown-linux-gnu.
>
>Ok for trunk?  (given low / close to no testing coverage
>extra close eyes wanted!)
>
>Reporter says maybe it can't happen in real-life as it requires
>EH deallocation order not be the reverse of allocation order.
>I don't know enough here for a quick guess but "in C++ everything
>is possible" ;)

I think it's possible with this testcase:

#include <exception>

int main()
{
  std::exception_ptr p[3];
  for (auto& e : p)
    try {
      throw 1;
    } catch (...) {
      e = std::current_exception();
    }

  p[1] = nullptr;
  p[0] = nullptr;
  p[2] = nullptr;
}

But to test it I had to hack the __cxa_allocate_exception function to
never use malloc and always use the pool, so it's not suitable for the
testsuite.

With current trunk we create three exception objects with addresses,
X, Y, and Z, with the first_free_entry pointing immediately after Z.
The p[1] = nullptr statement frees Y, but after that we still have
first_free_entry pointing after Z, and then its next pointer points to
Y. (So if we threw again at that point, rather than using Y we would
chop off the start of the free block, fragmenting the pool).

When we free p[0] we add X to the list in between the first block and
Y, but don't merge X and Y.

The patch fixes the case above so we end up with a single block again
after all the deallocations.

OK for trunk, although please change s/Slit/Split/ in the comment on
line 165.