Checking for dead memcache server buggy?

[rpluem-vf]

In

mod_auth_openidc/src/cache/memcache.c

Line 229 in 28557c1

rc = rc && (context->cache_memcache->live_servers[0]->status != APR_MC_SERVER_DEAD);

doesn't it need to be

rc = rc && (context->cache_memcache->live_servers[i]->status != APR_MC_SERVER_DEAD); 

because otherwise I don't see how we iterate over all servers as the loop body in

mod_auth_openidc/src/cache/memcache.c

Lines 228 to 229 in 28557c1

	for (i = 0; rc && i < context->cache_memcache->ntotal; i++)
	rc = rc && (context->cache_memcache->live_servers[0]->status != APR_MC_SERVER_DEAD);

is invariant as neither i is used nor the array pointer of live_servers gets incremented.

Furthermore, what should be tested? That all servers are up? In this case the check would be correct, but I guess we would like to check that a least one server is up and hence the check would need to be:

	int rc = FALSE;
	int i;
	for (i = 0; !rc && i < context->cache_memcache->ntotal; i++)
		rc = rc || (context->cache_memcache->live_servers[i]->status != APR_MC_SERVER_DEAD);
	return rc;

or

	int i;
	for (i = 0;  i < context->cache_memcache->ntotal; i++) {
		if (context->cache_memcache->live_servers[i]->status != APR_MC_SERVER_DEAD)
		    return TRUE;
        }
	return FALSE;

[zandbelt]

I believe you're right, thanks for spotting this! The error has been there since the very first commit of that code in 2017 and is fixed now in 30d9a55.