Message ID | alpine.LSU.2.20.1705121203560.20726@zhemvz.fhfr.qr |
---|---|
State | New |
Headers | show |
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.
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.
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 {