Branko Čibej <br...@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*
