Hi!
I want to report two bugs in Subversion 1.8.5 libsvn_subr
subversion/libsvn_subr/cache-membuffer.c, a more important one and a
less important one. I've fixed them, and patches are attached - please
apply them.
First bug (important):
There is an incorrect integer addition to cache->data_used which makes
32-bit threaded svnserve sometimes hang and eat 100% CPU. When it hangs,
one thread is doing an infinite loop in ensure_data_insertable() with
write lock acquired, and others just wait for it to release the lock. So
it looks like 100% usage of a single CPU core and non-working svn://
access.
The problem is in line 1377: "cache->data_used += size - entry->size;"
is incorrect because cache->data_used is always apr_uint64_t, and size
and entry->size are size_t which is unsigned 32-bit integer type on
32-bit platforms. And size is always [almost always] less than
entry->size at that line, so cache->data_used becomes > 0x100000000 at
once which makes ensure_data_insertable() to loop infinitely because all
entries in cache "become" less than 1/8 * average entry size.
The patch is in the attachment 'fix-membuffer-u32-overflow'
Second bug (less important):
There is also another bug in libsvn_subr membuffer cache implementation:
cache->hit_count and entry->hit_count are incremented without proper
mutual exclusion (only with read lock acquired), so some increments get
lost in multithreaded svnserve; as it's unlikely two threads access the
same entry at once, usually cache->hit_count (not entry->hit_count)
increments are lost, which leads to cache->hit_count being less than
sum(entry->hit_count).
So cache->hit_count may overflow during subtraction of entry->hit_count
from it and lead to removing of extra values from the cache in
ensure_data_insertable(), because the retain threshold may be very big;
also it may overflow again, become less than cache->entries_used and
also lead to removing of extra items.
Instead of killing the performance by mutual exclusion I think it's
possible to just check for integer overflow.
This doesn't affect general svnserve usability, so severity is minor.
The patch is in the attachment 'fix-membuffer-hit_count-overflow'
Hope you'll apply these patches soon, or fix the bugs some another way
:)
--
With best regards,
Vitaliy Filippov
See the comment.
--- a/subversion/libsvn_subr/cache-membuffer.c 2014-02-12 21:53:58.547230675
+0000
+++ a/subversion/libsvn_subr/cache-membuffer.c 2014-02-12 22:02:11.039246322
+0000
@@ -639,9 +639,16 @@ drop_entry(svn_membuffer_t *cache, entry
assert(idx <= last_in_group);
/* update global cache usage counters
+ *
+ * cache->hit_count and entry->hit_count are incremented without proper
mutual
+ * exclusion (only with read lock), so some increments get lost in
multithreaded svnserve;
+ * as it's unlikely two threads access the same entry at once usually it
leads to
+ * cache->hit_count being less than sum(entry->hit_count). So it may overflow
+ * during subtraction and lead to emptying the cache in
ensure_data_insertable().
+ * instead of killing the performance by mutual exclusion we just check for
possible overflow.
*/
cache->used_entries--;
- cache->hit_count -= entry->hit_count;
+ cache->hit_count = cache->hit_count < entry->hit_count ? 0 :
cache->hit_count-entry->hit_count;
cache->data_used -= entry->size;
/* extend the insertion window, if the entry happens to border it
@@ -802,7 +809,7 @@ let_entry_age(svn_membuffer_t *cache, en
{
apr_uint32_t hits_removed = (entry->hit_count + 1) >> 1;
- cache->hit_count -= hits_removed;
+ cache->hit_count = cache->hit_count < hits_removed ? 0 :
cache->hit_count-hits_removed;
entry->hit_count -= hits_removed;
}
This patch fixes the bug which makes 32-bit svnserve hang after some period of
time
doing an infinite loop inside ensure_data_insertable() because cache->data_used
becomes
a very big value, and ensure_data_insertable() never removes entries smaller
than
cache->data_used / cache->used_entries / 8.
--- a/subversion/libsvn_subr/cache-membuffer.c 2014-02-12 21:42:12.483208244
+0000
+++ b/subversion/libsvn_subr/cache-membuffer.c 2014-02-12 21:45:54.275215290
+0000
@@ -1374,7 +1374,9 @@ membuffer_cache_set_internal(svn_membuff
* the old spot, just re-use that space. */
if (entry && ALIGN_VALUE(entry->size) >= size && buffer)
{
- cache->data_used += size - entry->size;
+ /* not "+=" because (size - entry_size) is almost always a big 32-bit
+ unsigned representation of a negative value on 32-bit platforms */
+ cache->data_used = cache->data_used + size - entry->size;
entry->size = size;
#ifdef SVN_DEBUG_CACHE_MEMBUFFER
