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

Philip Martin <philip.martin_at_wandisco.com> writes:

> 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.

If enforce a runtime limit on key size, both fixed and variable, making
it a positive number less than MAX_ITEM_SIZE then even in 32 bits we can
detect overflow with something like this:

    apr_size_t item_size, key_len.
    svn_boolean_t too_big = item_size > MAX_ITEM_SIZE - key_len;

So a patch something like:

Index: subversion/libsvn_subr/cache-membuffer.c
===================================================================
--- subversion/libsvn_subr/cache-membuffer.c (revision 1679506)
+++ subversion/libsvn_subr/cache-membuffer.c (working copy)
@@ -197,7 +197,7 @@ typedef struct entry_key_t
 
   /* Length of the full key. This value is aligned to ITEM_ALIGNMENT to
    * make sure the subsequent item content is properly aligned. */
- apr_uint32_t key_len;
+ apr_size_t key_len;
 } entry_key_t;
 
 /* A full key, i.e. the combination of the cache's key prefix with some
@@ -2013,14 +2013,19 @@ membuffer_cache_set_internal(svn_membuffer_t *cach
                              apr_pool_t *scratch_pool)
 {
   cache_level_t *level;
- apr_size_t size = item_size + to_find->entry_key.key_len;
+ apr_size_t size;
+ svn_boolean_t too_big
+ = item_size > MAX_ITEM_SIZE - to_find->entry_key.key_len;
 
   /* first, look for a previous entry for the given key */
   entry_t *entry = find_entry(cache, group_index, to_find, FALSE);
 
+ if (!too_big)
+ size = item_size + to_find->entry_key.key_len;
+
   /* if there is an old version of that entry and the new data fits into
    * the old spot, just re-use that space. */
- if (entry && ALIGN_VALUE(entry->size) >= size && buffer)
+ if (entry && !too_big && ALIGN_VALUE(entry->size) >= size && buffer)
     {
       /* Careful! We need to cast SIZE to the full width of CACHE->DATA_USED
        * lest we run into trouble with 32 bit underflow *not* treated as a
@@ -2052,8 +2057,9 @@ membuffer_cache_set_internal(svn_membuffer_t *cach
 
   /* if necessary, enlarge the insertion window.
    */
- level = buffer ? select_level(cache, size, priority) : NULL;
- if (level)
+ if (!too_big)
+ level = buffer ? select_level(cache, size, priority) : NULL;
+ if (!too_big && level)
     {
       /* Remove old data for this key, if that exists.
        * Get an unused entry for the key and and initialize it with
@@ -2477,10 +2483,12 @@ membuffer_cache_set_partial_internal(svn_membuffer
            */
           if (item_data != orig_data)
             {
+ svn_boolean_t too_big = item_size > MAX_ITEM_SIZE - key_len;
+
               /* Remove the old entry and try to make space for the new one.
                */
               drop_entry(cache, entry);
- if ( (cache->max_entry_size >= item_size + key_len)
+ if (!too_big
                   && ensure_data_insertable_l1(cache, item_size + key_len))
                 {
                   /* Write the new entry.
@@ -2598,12 +2606,18 @@ typedef struct svn_membuffer_cache_t
   svn_mutex__t *mutex;
 } svn_membuffer_cache_t;
 
+/* Largest fixed-size key and longest APR_HASH_KEY_STRING key allowed,
+ * must be less than MAX_ITEM_SIZE. An arbitrary number much higher
+ * than the expected key sizes of couple of hundred bytes at most.
+ */
+#define MAX_KEY_SIZE 50000
+
 /* Basically calculate a hash value for KEY of length KEY_LEN, combine it
  * with the CACHE->PREFIX and write the result in CACHE->COMBINED_KEY.
  * This could replace combine_key() entirely but we actually use it only
  * when the quick path failed.
  */
-static void
+static svn_error_t *
 combine_long_key(svn_membuffer_cache_t *cache,
                  const void *key,
                  apr_ssize_t key_len)
@@ -2615,7 +2629,12 @@ combine_long_key(svn_membuffer_cache_t *cache,
 
   /* handle variable-length keys */
   if (key_len == APR_HASH_KEY_STRING)
- key_len = strlen((const char *) key);
+ {
+ key_len = strlen((const char *) key);
+ if (key_len > MAX_KEY_SIZE)
+ return svn_error_create(SVN_ERR_UNSUPPORTED_FEATURE, NULL,
+ _("Key too big for membuffer-based cache"));
+ }
 
   /* Combine keys. */
   aligned_key_len = ALIGN_VALUE(key_len);
@@ -2636,12 +2655,14 @@ combine_long_key(svn_membuffer_cache_t *cache,
     ^= cache->prefix.entry_key.fingerprint[0];
   cache->combined_key.entry_key.fingerprint[1]
     ^= cache->prefix.entry_key.fingerprint[1];
+
+ return SVN_NO_ERROR;
 }
 
 /* Basically calculate a hash value for KEY of length KEY_LEN, combine it
  * with the CACHE->PREFIX and write the result in CACHE->COMBINED_KEY.
  */
-static void
+static svn_error_t *
 combine_key(svn_membuffer_cache_t *cache,
             const void *key,
             apr_ssize_t key_len)
@@ -2678,8 +2699,13 @@ combine_key(svn_membuffer_cache_t *cache,
   else
     {
       /* longer or variably sized keys */
- combine_long_key(cache, key, key_len);
+ SVN_ERR(combine_long_key(cache, key, key_len));
     }
+
+ cache->combined_key.entry_key.fingerprint[0] = 0x01010183;
+ cache->combined_key.entry_key.fingerprint[1] = 0x0302030F;
+
+ return SVN_NO_ERROR;
 }
 
 /* Implement svn_cache__vtable_t.get (not thread-safe)
@@ -2707,7 +2733,7 @@ svn_membuffer_cache_get(void **value_p,
   /* construct the full, i.e. globally unique, key by adding
    * this cache instances' prefix
    */
- combine_key(cache, key, cache->key_len);
+ SVN_ERR(combine_key(cache, key, cache->key_len));
 
   /* Look the item up. */
   SVN_ERR(membuffer_cache_get(cache->membuffer,
@@ -2744,7 +2770,7 @@ svn_membuffer_cache_has_key(svn_boolean_t *found,
   /* construct the full, i.e. globally unique, key by adding
    * this cache instances' prefix
    */
- combine_key(cache, key, cache->key_len);
+ SVN_ERR(combine_key(cache, key, cache->key_len));
 
   /* Look the item up. */
   SVN_ERR(membuffer_cache_has_key(cache->membuffer,
@@ -2774,7 +2800,7 @@ svn_membuffer_cache_set(void *cache_void,
   /* construct the full, i.e. globally unique, key by adding
    * this cache instances' prefix
    */
- combine_key(cache, key, cache->key_len);
+ SVN_ERR(combine_key(cache, key, cache->key_len));
 
   /* (probably) add the item to the cache. But there is no real guarantee
    * that the item will actually be cached afterwards.
@@ -2824,7 +2850,7 @@ svn_membuffer_cache_get_partial(void **value_p,
       return SVN_NO_ERROR;
     }
 
- combine_key(cache, key, cache->key_len);
+ SVN_ERR(combine_key(cache, key, cache->key_len));
   SVN_ERR(membuffer_cache_get_partial(cache->membuffer,
                                       &cache->combined_key,
                                       value_p,
@@ -2852,7 +2878,7 @@ svn_membuffer_cache_set_partial(void *cache_void,
 
   if (key != NULL)
     {
- combine_key(cache, key, cache->key_len);
+ SVN_ERR(combine_key(cache, key, cache->key_len));
       SVN_ERR(membuffer_cache_set_partial(cache->membuffer,
                                           &cache->combined_key,
                                           func,
@@ -3126,6 +3152,10 @@ svn_cache__create_membuffer_cache(svn_cache__t **c
   svn_cache__t *wrapper = apr_pcalloc(result_pool, sizeof(*wrapper));
   svn_membuffer_cache_t *cache = apr_pcalloc(result_pool, sizeof(*cache));
 
+ if (klen != APR_HASH_KEY_STRING && klen > MAX_KEY_SIZE)
+ return svn_error_create(SVN_ERR_UNSUPPORTED_FEATURE, NULL,
+ _("Key too big for membuffer-based cache"));
+
   /* initialize our internal cache header
    */
   cache->membuffer = membuffer;

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