Fix releasing of ref_count in apc_cache_store()#587
Conversation
In apc_cache_store(), the ref_count was decremented even when the entry needed to be deleted. This allowed the entry to be moved by defragmentation (by another process) before deletion. As a result, the pointer no longer pointed to the desired entry during deletion, causing segmentation faults.
|
@nikic This time, I repeatedly went through the code and examined all the places after locks or the ref_count were released. This was the only place I found where the pointer to the entry was still being used after a release. Unfortunately, I seem to have overlooked this in my last fix. |
| apc_cache_entry_release(cache, entry); | ||
| } else { | ||
| /* the entry mustn't be released before it is freed to prevent defragmentation from moving the entry */ | ||
| free_entry(cache, entry); |
There was a problem hiding this comment.
Was there a reason why free_entry() was previously below php_apc_end_try()?
|
@nikic Additionally, I would like to provide a PR as soon as possible that adds size binning to the SMA. This has superior performance in many scenarios and significantly reduces SMA lock contention. It's been passing the stress test and CI for days without any issues. I just want to do more testing and fine-tuning... |
|
@nikic Also, #588 and #589 could significantly improve performance. I've tested both PRs for hours with various stress tests and haven't found any issues... |
2 participants
In apc_cache_store(), the ref_count was decremented even when the entry needed to be deleted. This allowed the entry to be moved by defragmentation (by another process) before deletion. As a result, the pointer no longer pointed to the desired entry during deletion, causing segmentation faults.