search

gnustep / libobjc2

Objective-C runtime library intended for use with Clang.
http://www.gnustep.org/
MIT License
426 stars 116 forks source link

Fix autorelease pool emptying when new references are added #218

Closed Graham--M closed 2 years ago

Graham--M commented 2 years ago

An edge case exists where it is possible to leak references whilst emptying the arc autorelease pool for a thread when more object references are added to it (via a dealloc or release method) whilst releasing a reference in the pool. The existing code attempts to cover this scenario but if the reference is located in the same struct arc_autorelease_pool page as the stop address and enough references are added to cause one or more pages to be appended, it will prematurely stop inside of or at the beginning of the last appended page.

If the thread has further pools to pop and the scenario does not occur again when they are emptied, the missed references will then be released as normal. However, if the pool being emptied was the root autorelease pool of the thread (i.e. stop is NULL and was happening as part of cleanupPools()), the pages before the last appended page and the references in them will leak.

This PR changes the logic slightly to check that the page is still the same as the stopPool.

Fixes #217

One complication of this PR which could also occur in the existing code is that if the objects are written to always generate another autoreleased reference when released, the loop will never exit as there will always be more references to release.

Graham--M commented 2 years ago

I've jumped the gun on this. The existing code does actually deal with this properly when the thread exits and cleanupPools() runs, as the first emptying loop will continue until all pool pages are empty.

Therefore the only real improvement that this PR would actually give is to ensure that all references in a pool that is being popped are released at that time, rather than later when the thread exits or an earlier pool is popped.

I'm going to close this PR as I don't think this warrants a change from the existing behaviour.

davidchisnall commented 2 years ago

Sorry, reviewed before I saw the follow-on comment. I think waiting until the thread exits is not the right behaviour, what happens if you wrap the @autoreleasepool in another @autoreleasepool? Does it clean everything up on the outer one? What happens on macOS? We should probably aim to do the same thing that they do. I think I would expect everything autoreleased during a -dealloc to be released at the end of the autorelease pool cleanup.

Graham--M commented 2 years ago

I think waiting until the thread exits is not the right behaviour, what happens if you wrap the @autoreleasepool in another @autoreleasepool? Does it clean everything up on the outer one?

When the wrapping pool pops, the leftover appended pages would be emptied then. I mistakenly thought that this bug could also occur when emptying the root pool and not have any more opportunities to be emptied but the bug only happens when partially emptying a page.

What happens on macOS?

The code in the Apple runtime creates a new separate chain of pages every time a pool is pushed. When popping the pool, it loops until every reference in the pages for the pool are released and anticipates more references/pages being added during the loop.

It would probably be better to merge this then.