Date: Mon, 06 Oct 2014 14:20:42 -0400 From: John Baldwin <jhb@freebsd.org> To: freebsd-current@freebsd.org Cc: David Shane Holden <dpejesh@yahoo.com> Subject: Re: [PATCH] nscd Message-ID: <5187301.oT50I74Vz0@ralph.baldwin.cx> In-Reply-To: <m0d8o4$qa3$1@ger.gmane.org> References: <m0d8o4$qa3$1@ger.gmane.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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)
@@ -485,7 +485,7 @@
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);
@@ -522,7 +522,6 @@
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;
@@ -552,7 +551,6 @@
the_cache->entries = new_entries;
}
- entry_name_size = strlen(params->entry_name) + 1;
switch (params->entry_type)
{
case CET_COMMON:
@@ -560,16 +558,12 @@
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;
@@ -614,16 +608,12 @@
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);
@@ -781,9 +771,8 @@
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;
@@ -873,9 +862,8 @@
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)
@@ -116,7 +116,6 @@
struct mp_cache_entry_params const *mp_params)
{
struct configuration_entry *retval;
- size_t size;
int res;
TRACE_IN(create_configuration_entry);
@@ -159,22 +158,15 @@
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);
@@ -212,8 +204,7 @@
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;
@@ -544,15 +535,12 @@
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)
@@ -307,9 +307,7 @@
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)
@@ -241,9 +241,7 @@
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)
@@ -138,10 +138,8 @@
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);
}
@@ -161,8 +159,7 @@
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)
@@ -451,9 +451,8 @@
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;
@@ -532,9 +531,8 @@
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;
@@ -806,9 +804,8 @@
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5187301.oT50I74Vz0>