Re: [PATCH] nscd

On Monday, September 29, 2014 11:40:52 PM David Shane Holden wrote:
> So, I've noticed nscd hasn't worked right for awhile now.  Since I
> upgraded to 10.0 it never seemed to cache properly but I never bothered
> to really dig into it until recently and here's what I've found.  In my
> environment I have nsswitch set to use caching and LDAP as such:
> 
> group: files cache ldap
> passwd: files cache ldap
> 
> The LDAP part works fine, but caching didn't on 10.0 for some reason.
> On my 9.2 machines it works as expected though.  What I've found is in
> usr.sbin/nscd/query.c
> 
> struct query_state *
> init_query_state(int sockfd, size_t kevent_watermark, uid_t euid, gid_t
> egid)
> {
>    ...
> 	memcpy(&retval->timeout, &s_configuration->query_timeout,
> 		sizeof(struct timeval));
>    ...
> }
> 
> s_configuration->query_timeout is an 'int' which is being memcpy'd into
> a 'struct timeval' causing it to grab other parts of the s_configuration
> struct along with the query_timeout value and polluting retval->timeout.
> In this case it appears to be grabbing s_configuration->threads_num and
> shoving that into timeout.tv_sec along with the query_timeout. This ends
> up confusing nscd later on (instead of being 8 it ends up being set to
> 34359738376) and breaks it's ability to cache.  I've attached a patch to
> set the retval->timeout properly and gets nscd working again.  I'm
> guessing gcc was handling this differently from clang which is why it
> wasn't a problem before 10.0.

Cute.  This codebase likes to use memcpy() way too much for simple assignments
Which can lead to bugs like this.  It also likes to do this:

	size = strlen(name) + 1;
	foo = calloc(size, 1);
	assert(foo != NULL);
	strlcpy(foo, name, size); // or memcpy()

instead of the simpler:

	foo = strdup(name);
	assert(foo != NULL);

I have a patch to remove various memcpy's trying to copy structures as well as 
un-expand several of the strdup() calls if you'd care to test it.  These 
changes didn't cover any other memcpy() misuse.

Index: cachelib.c
===================================================================
--- cachelib.c	(revision 272657)
+++ cachelib.c	(working copy)
_at__at_ -485,7 +485,7 _at__at_
 	assert(retval != NULL);
 
 	assert(params != NULL);
-	memcpy(&retval->params, params, sizeof(struct cache_params));
+	retval->params = *params;
 
 	retval->entries = calloc(1,
 		sizeof(*retval->entries) * INITIAL_ENTRIES_CAPACITY);
_at__at_ -522,7 +522,6 _at__at_
 	struct cache_entry_params const *params)
 {
 	int policies_size;
-	size_t entry_name_size;
 	struct cache_common_entry_	*new_common_entry;
 	struct cache_mp_entry_		*new_mp_entry;
 
_at__at_ -552,7 +551,6 _at__at_
 		the_cache->entries = new_entries;
 	}
 
-	entry_name_size = strlen(params->entry_name) + 1;
 	switch (params->entry_type)
 	{
 	case CET_COMMON:
_at__at_ -560,16 +558,12 _at__at_
 			sizeof(*new_common_entry));
 		assert(new_common_entry != NULL);
 
-		memcpy(&new_common_entry->common_params, params,
-			sizeof(struct common_cache_entry_params));
-		new_common_entry->params =
-		  (struct cache_entry_params *)&new_common_entry->common_params;
+		new_common_entry->common_params.cep = *params;
+		new_common_entry->params = &new_common_entry->common_params.cep;
 
-		new_common_entry->common_params.cep.entry_name = calloc(1,
-			entry_name_size);
+		new_common_entry->common_params.cep.entry_name =
+		    strdup(params->entry_name);
 		assert(new_common_entry->common_params.cep.entry_name != NULL);
-		strlcpy(new_common_entry->common_params.cep.entry_name,
-			params->entry_name, entry_name_size);
 		new_common_entry->name =
 			new_common_entry->common_params.cep.entry_name;
 
_at__at_ -614,16 +608,12 _at__at_
 			sizeof(*new_mp_entry));
 		assert(new_mp_entry != NULL);
 
-		memcpy(&new_mp_entry->mp_params, params,
-			sizeof(struct mp_cache_entry_params));
-		new_mp_entry->params =
-			(struct cache_entry_params *)&new_mp_entry->mp_params;
+		new_mp_entry->mp_params.cep = *params;
+		new_mp_entry->params = &new_mp_entry->mp_params.cep;
 
-		new_mp_entry->mp_params.cep.entry_name = calloc(1,
-			entry_name_size);
+		new_mp_entry->mp_params.cep.entry_name =
+		    strdup(params->entry_name);
 		assert(new_mp_entry->mp_params.cep.entry_name != NULL);
-		strlcpy(new_mp_entry->mp_params.cep.entry_name, params->entry_name,
-			entry_name_size);
 		new_mp_entry->name = new_mp_entry->mp_params.cep.entry_name;
 
 		TAILQ_INIT(&new_mp_entry->ws_head);
_at__at_ -781,9 +771,8 _at__at_
 
 	if (find_res->fifo_policy_item->connected_item != NULL) {
 		connected_item = find_res->fifo_policy_item->connected_item;
-		memcpy(&connected_item->last_request_time,
-			&find_res->fifo_policy_item->last_request_time,
-			sizeof(struct timeval));
+		connected_item->last_request_time = 
+			find_res->fifo_policy_item->last_request_time;
 		connected_item->request_count =
 			find_res->fifo_policy_item->request_count;
 
_at__at_ -873,9 +862,8 _at__at_
 	if (common_entry->policies_size > 1) {
 		connected_policy_item =
 			common_entry->policies[1]->create_item_func();
-		memcpy(&connected_policy_item->creation_time,
-			&policy_item->creation_time,
-			sizeof(struct timeval));
+		connected_policy_item->creation_time =
+			policy_item->creation_time;
 		connected_policy_item->key = policy_item->key;
 		connected_policy_item->key_size = policy_item->key_size;
 
Index: config.c
===================================================================
--- config.c	(revision 272657)
+++ config.c	(working copy)
_at__at_ -116,7 +116,6 _at__at_
 	struct mp_cache_entry_params const *mp_params)
 {
 	struct configuration_entry *retval;
-	size_t	size;
 	int res;
 
 	TRACE_IN(create_configuration_entry);
_at__at_ -159,22 +158,15 _at__at_
 		return (NULL);
 	}
 
-	memcpy(&retval->positive_cache_params, positive_params,
-		sizeof(struct common_cache_entry_params));
-	memcpy(&retval->negative_cache_params, negative_params,
-		sizeof(struct common_cache_entry_params));
-	memcpy(&retval->mp_cache_params, mp_params,
-		sizeof(struct mp_cache_entry_params));
+	retval->positive_cache_params = *positive_params;
+	retval->negative_cache_params = *negative_params;
+	retval->mp_cache_params = *mp_params;
 
-	size = strlen(name);
-	retval->name = calloc(1, size + 1);
+	retval->name = strdup(name);
 	assert(retval->name != NULL);
-	memcpy(retval->name, name, size);
 
-	memcpy(&retval->common_query_timeout, common_timeout,
-		sizeof(struct timeval));
-	memcpy(&retval->mp_query_timeout, mp_timeout,
-		sizeof(struct timeval));
+	retval->common_query_timeout = *common_timeout;
+	retval->mp_query_timeout = *mp_timeout;
 
 	asprintf(&retval->positive_cache_params.cep.entry_name, "%s+", name);
 	assert(retval->positive_cache_params.cep.entry_name != NULL);
_at__at_ -212,8 +204,7 _at__at_
 	positive_params.confidence_threshold = DEFAULT_POSITIVE_CONF_THRESH;
 	positive_params.policy = CPT_LRU;
 
-	memcpy(&negative_params, &positive_params,
-		sizeof(struct common_cache_entry_params));
+	negative_params = positive_params;
 	negative_params.max_elemsize = DEFAULT_NEGATIVE_ELEMENTS_SIZE;
 	negative_params.satisf_elemsize = DEFAULT_NEGATIVE_ELEMENTS_SIZE / 2;
 	negative_params.max_lifetime.tv_sec = DEFAULT_NEGATIVE_LIFETIME;
_at__at_ -544,15 +535,12 _at__at_
 	if (config->socket_path != NULL)
 		free(config->socket_path);
 
-	len = strlen(DEFAULT_SOCKET_PATH);
-	config->socket_path = calloc(1, len + 1);
+	config->socket_path = strdup(DEFAULT_SOCKET_PATH);
 	assert(config->socket_path != NULL);
-	memcpy(config->socket_path, DEFAULT_SOCKET_PATH, len);
 
-	len = strlen(DEFAULT_PIDFILE_PATH);
+	config->pidfile_path = strdup(DEFAULT_PIDFILE_PATH);
 	config->pidfile_path = calloc(1, len + 1);
 	assert(config->pidfile_path != NULL);
-	memcpy(config->pidfile_path, DEFAULT_PIDFILE_PATH, len);
 
 	config->socket_mode =  S_IFSOCK | S_IRUSR | S_IWUSR |
 		S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH;
Index: mp_rs_query.c
===================================================================
--- mp_rs_query.c	(revision 272657)
+++ mp_rs_query.c	(working copy)
_at__at_ -307,9 +307,7 _at__at_
 			CELT_MULTIPART);
 		    if ((qstate->config_entry->mp_query_timeout.tv_sec != 0) ||
 		    (qstate->config_entry->mp_query_timeout.tv_usec != 0))
-			memcpy(&qstate->timeout,
-			    &qstate->config_entry->mp_query_timeout,
-			    sizeof(struct timeval));
+			qstate->timeout = qstate->config_entry->mp_query_timeout;
 		    configuration_unlock_entry(qstate->config_entry,
 			CELT_MULTIPART);
 		}
Index: mp_ws_query.c
===================================================================
--- mp_ws_query.c	(revision 272657)
+++ mp_ws_query.c	(working copy)
_at__at_ -241,9 +241,7 _at__at_
 
 		if ((qstate->config_entry->mp_query_timeout.tv_sec != 0) ||
 		    (qstate->config_entry->mp_query_timeout.tv_usec != 0))
-			memcpy(&qstate->timeout,
-				&qstate->config_entry->mp_query_timeout,
-				sizeof(struct timeval));
+			qstate->timeout = qstate->config_entry->mp_query_timeout;
 	}
 	configuration_unlock_entry(qstate->config_entry, CELT_MULTIPART);
 
Index: parser.c
===================================================================
--- parser.c	(revision 272657)
+++ parser.c	(working copy)
_at__at_ -138,10 +138,8 _at__at_
 	lifetime.tv_sec = ttl;
 
 	entry = find_create_entry(config, entry_name);
-	memcpy(&entry->positive_cache_params.max_lifetime,
-		&lifetime, sizeof(struct timeval));
-	memcpy(&entry->mp_cache_params.max_lifetime,
-		&lifetime, sizeof(struct timeval));
+	entry->positive_cache_params.max_lifetime = lifetime;
+	entry->mp_cache_params.max_lifetime = lifetime;
 
 	TRACE_OUT(set_positive_time_to_live);
 }
_at__at_ -161,8 +159,7 _at__at_
 
 	entry = find_create_entry(config, entry_name);
 	assert(entry != NULL);
-	memcpy(&entry->negative_cache_params.max_lifetime,
-		&lifetime, sizeof(struct timeval));
+	entry->negative_cache_params.max_lifetime = lifetime;
 
 	TRACE_OUT(set_negative_time_to_live);
 }
Index: query.c
===================================================================
--- query.c	(revision 272657)
+++ query.c	(working copy)
_at__at_ -451,9 +451,8 _at__at_
 
 		if ((qstate->config_entry->common_query_timeout.tv_sec != 0) ||
 		    (qstate->config_entry->common_query_timeout.tv_usec != 0))
-			memcpy(&qstate->timeout,
-				&qstate->config_entry->common_query_timeout,
-				sizeof(struct timeval));
+			qstate->timeout =
+			    qstate->config_entry->common_query_timeout;
 
 	} else
 		write_response->error_code = -1;
_at__at_ -532,9 +531,8 _at__at_
 
 		if ((qstate->config_entry->common_query_timeout.tv_sec != 0) ||
 		    (qstate->config_entry->common_query_timeout.tv_usec != 0))
-			memcpy(&qstate->timeout,
-				&qstate->config_entry->common_query_timeout,
-				sizeof(struct timeval));
+			qstate->timeout =
+			    qstate->config_entry->common_query_timeout;
 	} else
 		write_response->error_code = -1;
 
_at__at_ -806,9 +804,8 _at__at_
 
 		if ((qstate->config_entry->common_query_timeout.tv_sec != 0) ||
 		    (qstate->config_entry->common_query_timeout.tv_usec != 0))
-			memcpy(&qstate->timeout,
-				&qstate->config_entry->common_query_timeout,
-				sizeof(struct timeval));
+			qstate->timeout =
+			    qstate->config_entry->common_query_timeout;
 	} else
 		read_response->error_code = -1;
 

-- 
John Baldwin