From 845ceb7cc84f983d681e3487293c1c0368cd327b Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 13 May 2021 07:05:07 +0100 Subject: [PATCH] Improve readability and efficiency of SSL cache reference impl The reference session cache implementation may end up storing multiple sessions associated to the same session ID if the set()-call for the second session finds an outdated cache entry prior to noticing the entry with the matching session ID. While this logically overwrites the existing entry since we always search the cache in order, this is at least a waste of resources. This commit fixes this by always checking first whether the given ID is already present in the cache. It also restructures the code for easier readability. Fixes #4509. Signed-off-by: Hanno Becker --- library/ssl_cache.c | 168 ++++++++++++++++++++++---------------------- 1 file changed, 83 insertions(+), 85 deletions(-) diff --git a/library/ssl_cache.c b/library/ssl_cache.c index 5790fc6bf..1bbcc957b 100644 --- a/library/ssl_cache.c +++ b/library/ssl_cache.c @@ -129,7 +129,6 @@ static int ssl_cache_pick_writing_slot( mbedtls_ssl_cache_context *cache, size_t session_id_len, mbedtls_ssl_cache_entry **dst ) { - int ret = 1; #if defined(MBEDTLS_HAVE_TIME) mbedtls_time_t t = mbedtls_time( NULL ), oldest = 0; mbedtls_ssl_cache_entry *old = NULL; @@ -140,117 +139,116 @@ static int ssl_cache_pick_writing_slot( mbedtls_ssl_cache_context *cache, cur = cache->chain; prv = NULL; + /* Check 1: Is there already an entry with the given session ID? + * + * If yes, overwrite it. + * + * If not, `count` will hold the size of the session cache + * at the end of this loop, and `prv` will point to the last + * entry, both of which will be used later. */ + while( cur != NULL ) { count++; -#if defined(MBEDTLS_HAVE_TIME) - if( cache->timeout != 0 && - (int) ( t - cur->timestamp ) > cache->timeout ) - { - cur->timestamp = t; - break; /* expired, reuse this slot, update timestamp */ - } -#endif - if( session_id_len == cur->session_id_len && memcmp( session_id, cur->session_id, cur->session_id_len ) == 0 ) { - break; /* client reconnected, keep timestamp for session id */ + goto found; } -#if defined(MBEDTLS_HAVE_TIME) - if( oldest == 0 || cur->timestamp < oldest ) - { - oldest = cur->timestamp; - old = cur; - } -#endif - prv = cur; cur = cur->next; } - if( cur == NULL ) - { + /* Check 2: Is there an outdated entry in the cache? + * + * If so, overwrite it. + * + * If not, remember the oldest entry in `old` for later. + */ + #if defined(MBEDTLS_HAVE_TIME) - /* - * Reuse oldest entry if max_entries reached - */ - if( count >= cache->max_entries ) + while( cur != NULL ) + { + if( cache->timeout != 0 && + (int) ( t - cur->timestamp ) > cache->timeout ) { - if( old == NULL ) - { - ret = 1; - goto exit; - } - - cur = old; + goto found; } -#else /* MBEDTLS_HAVE_TIME */ - /* - * Reuse first entry in chain if max_entries reached, - * but move to last place - */ - if( count >= cache->max_entries ) + + if( oldest == 0 || cur->timestamp < oldest ) { - if( cache->chain == NULL ) - { - ret = 1; - goto exit; - } - - cur = cache->chain; - cache->chain = cur->next; - cur->next = NULL; - prv->next = cur; + oldest = cur->timestamp; + old = cur; } + + prv = cur; + cur = cur->next; + } #endif /* MBEDTLS_HAVE_TIME */ - else - { - /* - * max_entries not reached, create new entry - */ - cur = mbedtls_calloc( 1, sizeof(mbedtls_ssl_cache_entry) ); - if( cur == NULL ) - { - ret = 1; - goto exit; - } - if( prv == NULL ) - cache->chain = cur; - else - prv->next = cur; - } + /* Check 3: Is there free space in the cache? */ + + if( count < cache->max_entries ) + { + /* Create new entry */ + cur = mbedtls_calloc( 1, sizeof(mbedtls_ssl_cache_entry) ); + if( cur == NULL ) + return( 1 ); + + /* Append to the end of the linked list. */ + if( prv == NULL ) + cache->chain = cur; + else + prv->next = cur; + + goto found; + } + + /* Last resort: The cache is full and doesn't contain any outdated + * elements. In this case, we evict the oldest one, judged by timestamp + * (if present) or cache-order. */ #if defined(MBEDTLS_HAVE_TIME) - cur->timestamp = t; -#endif - } - - if( cur != NULL ) + if( old == NULL ) { - *dst = cur; + /* This should only happen on an ill-configured cache + * with max_entries == 0. */ + return( 1 ); + } +#else /* MBEDTLS_HAVE_TIME */ + /* Reuse first entry in chain, but move to last place. */ + if( cache->chain == NULL ) + return( 1 ); - /* - * If we're reusing an entry, free it first. - */ - if( cur->session != NULL ) - { - mbedtls_free( cur->session ); - cur->session = NULL; - cur->session_len = 0; - memset( cur->session_id, 0, sizeof( cur->session_id ) ); - cur->session_id_len = 0; - } + old = cache->chain; + cache->chain = old->next; + cur->next = NULL; + prv->next = old; +#endif /* MBEDTLS_HAVE_TIME */ - ret = 0; + /* Now `old` points to the oldest entry to be overwritten. */ + cur = old; + +found: + +#if defined(MBEDTLS_HAVE_TIME) + cur->timestamp = t; +#endif + + /* If we're reusing an entry, free it first. */ + if( cur->session != NULL ) + { + mbedtls_free( cur->session ); + cur->session = NULL; + cur->session_len = 0; + memset( cur->session_id, 0, sizeof( cur->session_id ) ); + cur->session_id_len = 0; } -exit: - - return( ret ); + *dst = cur; + return( 0 ); } int mbedtls_ssl_cache_set( void *data,