Re: [VOTE] Merging 1.9-cache-improvements to /trunk

Branko Čibej <brane_at_wandisco.com> writes:

> -0 because:
>
> $ make
> .../subversion/libsvn_subr/cache-membuffer.c:2626:59: warning: implicit conversion loses integer
> precision: 'unsigned long' to 'apr_uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
> cache->combined_key.entry_key.key_len = aligned_key_len + prefix_len;
> ~ ~~~~~~~~~~~~~~~~^~~~~~~~~~~~
> .../subversion/libsvn_subr/cache-membuffer.c:2664:58: warning: implicit conversion loses integer
> precision: 'unsigned long' to 'apr_uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
> cache->combined_key.entry_key.key_len = prefix_len + 16;
> ~ ~~~~~~~~~~~^~~~
> .../subversion/libsvn_subr/cache-membuffer.c:3161:37: warning: implicit conversion loses integer
> precision: 'apr_size_t' (aka 'unsigned long') to 'apr_uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
> cache->prefix.entry_key.key_len = prefix_len;
> ~ ^~~~~~~~~~
> 3 warnings generated.
>
> +1 if these warnings get fixed before or as part of the merge without
> adding casts.

I'd like to see this code on trunk but I am reluctant to vote because I
don't think a change like this should require a vote.

Prompted by the warnings I think there are some issues to fix. For
APR_HASH_KEY_STRING keys there is no protection against abnormally long
keys. combined_long_key() will allocate strlen() memory even if it is
many GB. The item will not get cached if key+data length is more than
4GB but the memory for the key, which could be more than 4GB, will be
permanently allocated in the cache. There is also a problem with
overflow in membuffer_cache_set_internal() when calculating key+data
length, although in practice a key large enough to trigger this will
probably fail memory allocation first.

In practice keys are short, a 6GB key probably indicates that a bug has
already occurred and that memory is corrupt. However, the code should be
more defensive.

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*