code: address SonarQube warnings in src/cache/*.c

Signed-off-by: Hans Zandbelt <[email protected]>
OpenIDC · Dec 17, 2024 · 127e170 · 127e170
1 parent ef58a94
commit 127e170
Show file tree

Hide file tree

Showing 7 changed files with 61 additions and 49 deletions.
diff --git a/ChangeLog b/ChangeLog
@@ -1,11 +1,15 @@
+12/17/2024
+- code: address SonarQube warnings in src/cache/*.c
+
 12/16/2024
 - http: report errors when curl_easy_setopt fails and improve macro usage
-- code: declare enum members as int so they can be set to OIDC_CONFIG_POS_INT_UNSET without warning
-- code: declare memcache members as int so they can be set to OIDC_CONFIG_POS_INT_UNSET without warning
-- code: declare introspection_endpoint_method member as int so it can be set to OIDC_CONFIG_POS_INT_UNSET without warning
-- code: check return value of oidc_get_provider_from_session and oidc_refresh_token_grant in logout.c
-- code: avoid potential crash on non-conformant literal IPv6 adresses in oidc_util_current_url_host
-- code: apply boundary checks on oidc_metrics_shm_size and use a global static for performance reasons
+- address warnings from static code analysis tool Coverity
+  - code: declare enum members as int so they can be set to OIDC_CONFIG_POS_INT_UNSET without warning
+  - code: declare memcache members as int so they can be set to OIDC_CONFIG_POS_INT_UNSET without warning
+  - code: declare introspection_endpoint_method member as int so it can be set to OIDC_CONFIG_POS_INT_UNSET without warning
+  - code: check return value of oidc_get_provider_from_session and oidc_refresh_token_grant in logout.c
+  - code: avoid potential crash on non-conformant literal IPv6 adresses in oidc_util_current_url_host
+  - code: apply boundary checks on oidc_metrics_shm_size and use a global static for performance reasons
 
 12/15/2024
 - add Coverity Github action
@@ -22,16 +26,18 @@
   - code: correct check for *static_template_content in oidc_util_html_send_in_template in util.c
 
 12/11/2024
-- code: loop over authz arrays with index instead of pointer
-- code: avoid embedding defines in macro arguments
-- code: avoid cast warnings
-- code: add comment to empty functions
-- code: remove any side effects from right hand operands of logical && operator
+- address warnings from static code analysis tool SonarQube
+  - code: loop over authz arrays with index instead of pointer
+  - code: avoid embedding defines in macro arguments
+  - code: avoid cast warnings
+  - code: add comment to empty functions
+  - code: remove any side effects from right hand operands of logical && operator
 
 12/10/2024
 - github: add SonarQube analysis to Github workflows
-- code: use snprintf instead of sprintf
-- code: move _snprintf define to const.h
+- address warnings from static code analysis tool SonarQube
+  - code: use snprintf instead of sprintf
+  - code: move _snprintf define to const.h
 - bump to 2.4.16.7dev
 
 12/09/2024

diff --git a/src/cache/common.c b/src/cache/common.c
@@ -218,7 +218,7 @@ apr_byte_t oidc_cache_mutex_destroy(server_rec *s, oidc_cache_mutex_t *m) {
 
 	oidc_slog(s, APLOG_TRACE1, "init: %pp (m=%pp,s=%pp, p=%d)", m, m->gmutex ? m->gmutex : 0, s, m->is_parent);
 
-	if ((m) && (m->is_parent == TRUE)) {
+	if (m && (m->is_parent == TRUE)) {
 		if ((m->is_global) && (m->gmutex)) {
 			rv = apr_global_mutex_destroy(m->gmutex);
 			m->gmutex = NULL;
@@ -293,7 +293,8 @@ apr_byte_t oidc_cache_get(request_rec *r, const char *section, const char *key,
 	apr_byte_t rc = FALSE;
 	char *msg = NULL;
 	const char *s_key = NULL;
-	char *cache_value = NULL, *s_secret = NULL;
+	char *cache_value = NULL;
+	char *s_secret = NULL;
 
 	oidc_debug(r, "enter: %s (section=%s, decrypt=%d, type=%s)", key, section, encrypted, cfg->cache.impl->name);
 

diff --git a/src/cache/file.c b/src/cache/file.c
@@ -307,8 +307,8 @@ static apr_status_t oidc_cache_file_clean(request_rec *r) {
 			/* skip non-cache entries, cq. the ".", ".." and the metadata file */
 			if ((fi.name[0] == OIDC_CHAR_DOT) ||
 			    (_oidc_strstr(fi.name, OIDC_CACHE_FILE_PREFIX) != fi.name) ||
-			    ((_oidc_strcmp(fi.name,
-					   oidc_cache_file_name(r, "cache-file", OIDC_CACHE_FILE_LAST_CLEANED)) == 0)))
+			    (_oidc_strcmp(fi.name,
+					  oidc_cache_file_name(r, "cache-file", OIDC_CACHE_FILE_LAST_CLEANED)) == 0))
 				continue;
 
 			/* get the fully qualified path to the cache file and open it */

diff --git a/src/cache/memcache.c b/src/cache/memcache.c
@@ -78,13 +78,19 @@ static int oidc_cache_memcache_post_config(server_rec *s) {
 	cfg->cache.cfg = context;
 
 	apr_status_t rv = APR_SUCCESS;
-	int nservers = 0;
+	apr_uint16_t nservers = 0;
 	char *split;
 	char *tok;
 	apr_pool_t *p = s->process->pool;
 	APR_OPTIONAL_FN_TYPE(http2_get_num_workers) * get_h2_num_workers;
-	int max_threads, minw, maxw;
-	apr_uint32_t min, smax, hmax, ttl;
+	int max_threads = 0;
+	int minw = 0;
+	int maxw = 0;
+	apr_uint32_t min = 0;
+	apr_uint32_t smax = 0;
+	apr_uint32_t hmax = 0;
+	apr_uint32_t ttl = 0;
+	;
 
 	if (oidc_cfg_cache_memcache_servers_get(cfg) == NULL) {
 		oidc_serror(s, "cache type is set to \"memcache\", but no valid " OIDCMemCacheServers
@@ -222,9 +228,8 @@ static char *oidc_cache_memcache_get_key(apr_pool_t *pool, const char *section,
 /*
  * check dead/alive status for all servers
  */
-static apr_byte_t oidc_cache_memcache_status(request_rec *r, oidc_cache_cfg_memcache_t *context) {
-	int i = 0;
-	for (i = 0; i < context->cache_memcache->ntotal; i++) {
+static apr_byte_t oidc_cache_memcache_status(const oidc_cache_cfg_memcache_t *context) {
+	for (int i = 0; i < context->cache_memcache->ntotal; i++) {
 		if (context->cache_memcache->live_servers[i]->status != APR_MC_SERVER_DEAD)
 			return TRUE;
 	}
@@ -250,7 +255,7 @@ static apr_byte_t oidc_cache_memcache_get(request_rec *r, const char *section, c
 		/*
 		 * NB: workaround the fact that the apr_memcache returns APR_NOTFOUND if a server has been marked dead
 		 */
-		if (oidc_cache_memcache_status(r, context) == FALSE) {
+		if (oidc_cache_memcache_status(context) == FALSE) {
 
 			oidc_cache_memcache_log_status_error(r, "apr_memcache_getp", rv);
 
@@ -309,7 +314,7 @@ static apr_byte_t oidc_cache_memcache_set(request_rec *r, const char *section, c
 	} else {
 
 		/* calculate the timeout as a Unix timestamp which allows values > 30 days */
-		apr_uint32_t timeout = apr_time_sec(expiry);
+		apr_uint32_t timeout = (apr_uint32_t)apr_time_sec(expiry);
 
 		/* store it */
 		rv = apr_memcache_set(context->cache_memcache, oidc_cache_memcache_get_key(r->pool, section, key),

diff --git a/src/cache/redis.c b/src/cache/redis.c
@@ -112,11 +112,9 @@ static apr_status_t oidc_cache_redis_connect(request_rec *r, oidc_cache_cfg_redi
  * free resources allocated for the per-process Redis connection context
  */
 apr_status_t oidc_cache_redis_disconnect(oidc_cache_cfg_redis_t *context) {
-	if (context != NULL) {
-		if (context->rctx != NULL) {
-			redisFree(context->rctx);
-			context->rctx = NULL;
-		}
+	if ((context != NULL) && (context->rctx != NULL)) {
+		redisFree(context->rctx);
+		context->rctx = NULL;
 	}
 	return APR_SUCCESS;
 }
@@ -367,10 +365,11 @@ static int oidc_cache_redis_env2int(request_rec *r, const char *env_var_name, co
 #define OIDC_REDIS_RETRY_INTERVAL_DEFAULT 300
 
 #define OIDC_REDIS_WARN_OR_ERROR(cond, r, ...)                                                                         \
-	if (cond)                                                                                                      \
+	if (cond) {                                                                                                    \
 		oidc_warn(r, ##__VA_ARGS__);                                                                           \
-	else                                                                                                           \
-		oidc_error(r, ##__VA_ARGS__);
+	} else {                                                                                                       \
+		oidc_error(r, ##__VA_ARGS__);                                                                          \
+	}
 
 /*
  * execute Redis command and deal with return value
@@ -379,14 +378,13 @@ static redisReply *oidc_cache_redis_exec(request_rec *r, oidc_cache_cfg_redis_t
 
 	redisReply *reply = NULL;
 	char *errstr = NULL;
-	int i = 0;
 	va_list ap;
 	int retries = oidc_cache_redis_env2int(r, OIDC_REDIS_MAX_TRIES_ENV_VAR, OIDC_REDIS_MAX_TRIES_DEFAULT);
 	apr_time_t interval = apr_time_from_msec(
 	    oidc_cache_redis_env2int(r, OIDC_REDIS_RETRY_INTERVAL_ENV_VAR, OIDC_REDIS_RETRY_INTERVAL_DEFAULT));
 
 	/* try to execute a command at max n times while reconnecting */
-	for (i = 1; i <= retries; i++) {
+	for (int i = 1; i <= retries; i++) {
 
 		/* connect */
 		if (context->connect(r, context) != APR_SUCCESS) {
@@ -507,7 +505,7 @@ apr_byte_t oidc_cache_redis_set(request_rec *r, const char *section, const char
 	} else {
 
 		/* calculate the timeout from now */
-		timeout = apr_time_sec(expiry - apr_time_now());
+		timeout = (apr_uint32_t)apr_time_sec(expiry - apr_time_now());
 
 		/* store it */
 		reply = oidc_cache_redis_exec(r, context, "SETEX %s %d %s",

diff --git a/src/cache/shm.c b/src/cache/shm.c
@@ -103,9 +103,8 @@ int oidc_cache_shm_post_config(server_rec *s) {
 	}
 
 	/* initialize the whole segment to '/0' */
-	int i;
 	oidc_cache_shm_entry_t *t = apr_shm_baseaddr_get(context->shm);
-	for (i = 0; i < cfg->cache.shm_size_max; i++, OIDC_CACHE_SHM_ADD_OFFSET(t, cfg->cache.shm_entry_size_max)) {
+	for (int i = 0; i < cfg->cache.shm_size_max; i++, OIDC_CACHE_SHM_ADD_OFFSET(t, cfg->cache.shm_entry_size_max)) {
 		t->section_key[0] = '\0';
 		t->access = 0;
 	}
@@ -220,11 +219,12 @@ static apr_byte_t oidc_cache_shm_set(request_rec *r, const char *section, const
 	oidc_cfg_t *cfg = ap_get_module_config(r->server->module_config, &auth_openidc_module);
 	oidc_cache_cfg_shm_t *context = (oidc_cache_cfg_shm_t *)cfg->cache.cfg;
 
-	oidc_cache_shm_entry_t *match, *free, *lru;
-	oidc_cache_shm_entry_t *t;
-	apr_time_t current_time;
-	int i;
-	apr_time_t age;
+	oidc_cache_shm_entry_t *match = NULL;
+	oidc_cache_shm_entry_t *free = NULL;
+	oidc_cache_shm_entry_t *lru = NULL;
+	oidc_cache_shm_entry_t *t = NULL;
+	apr_time_t current_time = 0;
+	apr_time_t age = 0;
 
 	const char *section_key = oidc_cache_shm_get_key(r, section, key);
 	if (section_key == NULL)
@@ -255,7 +255,7 @@ static apr_byte_t oidc_cache_shm_set(request_rec *r, const char *section, const
 	match = NULL;
 	free = NULL;
 	lru = t;
-	for (i = 0; i < cfg->cache.shm_size_max; i++, OIDC_CACHE_SHM_ADD_OFFSET(t, cfg->cache.shm_entry_size_max)) {
+	for (int i = 0; i < cfg->cache.shm_size_max; i++, OIDC_CACHE_SHM_ADD_OFFSET(t, cfg->cache.shm_entry_size_max)) {
 
 		/* see if this slot is free */
 		if (t->section_key[0] == '\0') {
@@ -327,15 +327,15 @@ static int oidc_cache_shm_destroy(server_rec *s) {
 	oidc_slog(s, APLOG_TRACE1, "destroy: %pp (shm=%pp,s=%pp, p=%d)", context, context ? context->shm : 0, s,
 		  context ? context->is_parent : -1);
 
-	if ((context) && (context->is_parent == TRUE) && (context->shm) && (context->mutex)) {
+	if (context && (context->is_parent == TRUE) && (context->shm) && (context->mutex)) {
 		oidc_cache_mutex_lock(s->process->pool, s, context->mutex);
 		rv = apr_shm_destroy(context->shm);
 		oidc_sdebug(s, "apr_shm_destroy returned: %d", rv);
 		context->shm = NULL;
 		oidc_cache_mutex_unlock(s->process->pool, s, context->mutex);
 	}
 
-	if ((context) && (context->mutex)) {
+	if (context && (context->mutex)) {
 		if (oidc_cache_mutex_destroy(s, context->mutex) != TRUE)
 			rv = APR_EGENERAL;
 		context->mutex = NULL;

diff --git a/src/metrics.h b/src/metrics.h
@@ -197,10 +197,12 @@ extern const oidc_metrics_counter_info_t _oidc_metrics_counters_info[];
 void oidc_metrics_counter_inc(request_rec *r, oidc_metrics_counter_type_t type, const char *spec);
 
 #define OIDC_METRICS_COUNTER_INC_SPEC(r, cfg, type, spec)                                                              \
-	if (oidc_cfg_metrics_hook_data_get(cfg) != NULL)                                                               \
+	if (oidc_cfg_metrics_hook_data_get(cfg) != NULL) {                                                             \
 		if (apr_hash_get(oidc_cfg_metrics_hook_data_get(cfg), _oidc_metrics_counters_info[type].class_name,    \
-				 APR_HASH_KEY_STRING) != NULL)                                                         \
-			oidc_metrics_counter_inc(r, type, spec);
+				 APR_HASH_KEY_STRING) != NULL) {                                                       \
+			oidc_metrics_counter_inc(r, type, spec);                                                       \
+		}                                                                                                      \
+	}
 
 #define OIDC_METRICS_COUNTER_INC(r, cfg, type) OIDC_METRICS_COUNTER_INC_SPEC(r, cfg, type, NULL);