diff --git a/ChangeLog b/ChangeLog index 32a82268..4dcdc399 100644 --- 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 index 4919bf93..9eb6e7be 100644 --- 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 index 06902da0..3b8784bf 100644 --- 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 index 0d0c6b8e..e7c170a4 100644 --- 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 index 4fda4362..7f197ebc 100644 --- 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 index 5933caea..b3354fa3 100644 --- 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,7 +327,7 @@ 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); @@ -335,7 +335,7 @@ static int oidc_cache_shm_destroy(server_rec *s) { 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 index ad8cff36..cb1c482b 100644 --- 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);