Mercurial > illumos > illumos-gate
changeset 4884:a15ef4eea444
6552729 db handle should be reused instead of opened per-request
6588930 idmapd crashes in processing AD LDAP response
author | jp151216 |
---|---|
date | Fri, 17 Aug 2007 08:49:55 -0700 |
parents | 880ca7e5862c |
children | 225b7936f7ea |
files | usr/src/cmd/idmap/idmapd/adutils.c usr/src/cmd/idmap/idmapd/adutils.h usr/src/cmd/idmap/idmapd/dbutils.c usr/src/cmd/idmap/idmapd/idmapd.c usr/src/cmd/idmap/idmapd/idmapd.h usr/src/cmd/idmap/idmapd/server.c |
diffstat | 6 files changed, 279 insertions(+), 140 deletions(-) [+] |
line wrap: on
line diff
--- a/usr/src/cmd/idmap/idmapd/adutils.c Thu Aug 16 23:36:19 2007 -0700 +++ b/usr/src/cmd/idmap/idmapd/adutils.c Fri Aug 17 08:49:55 2007 -0700 @@ -139,6 +139,8 @@ struct idmap_query_state { idmap_query_state_t *next; int qcount; /* how many queries */ + int ref_cnt; /* reference count */ + pthread_cond_t cv; /* Condition wait variable */ uint32_t qlastsent; uint32_t qinflight; /* how many queries in flight */ uint16_t qdead; /* oops, lost LDAP connection */ @@ -162,6 +164,12 @@ static pthread_t reaperid = 0; static pthread_mutex_t adhostlock = PTHREAD_MUTEX_INITIALIZER; + +static void +idmap_lookup_unlock_batch(idmap_query_state_t **state); + + + /*ARGSUSED*/ static int idmap_saslcallback(LDAP *ld, unsigned flags, void *defaults, void *prompts) { @@ -943,11 +951,13 @@ if (new_state == NULL) return (IDMAP_ERR_MEMORY); + new_state->ref_cnt = 1; new_state->qadh = adh; new_state->qcount = nqueries; new_state->qadh_gen = adh->generation; /* should be -1, but the atomic routines want unsigned */ new_state->qlastsent = 0; + (void) pthread_cond_init(&new_state->cv, NULL); (void) pthread_mutex_lock(&qstatelock); new_state->next = qstatehead; @@ -961,7 +971,9 @@ /* * Find the idmap_query_state_t to which a given LDAP result msgid on a - * given connection belongs + * given connection belongs. This routine increaments the reference count + * so that the object can not be freed. idmap_lookup_unlock_batch() + * must be called to decreament the reference count. */ static int @@ -977,6 +989,7 @@ continue; for (i = 0; i < p->qcount; i++) { if ((p->queries[i]).msgid == msgid) { + p->ref_cnt++; *state = p; *qid = i; (void) pthread_mutex_unlock(&qstatelock); @@ -1215,6 +1228,7 @@ atomic_dec_32(&query_state->qinflight); /* we saw at least one reply */ query_state->queries[qid].got_reply = 1; + idmap_lookup_unlock_batch(&query_state); } (void) ldap_msgfree(res); ret = 0; @@ -1238,6 +1252,7 @@ /* we saw at least one result */ query_state->queries[qid].got_reply = 1; query_state->queries[qid].got_results = 1; + idmap_lookup_unlock_batch(&query_state); } (void) ldap_msgfree(res); ret = 0; @@ -1251,15 +1266,48 @@ return (ret); } +/* + * This routine decreament the reference count of the + * idmap_query_state_t + */ +static void +idmap_lookup_unlock_batch(idmap_query_state_t **state) +{ + /* + * Decrement reference count with qstatelock locked + */ + (void) pthread_mutex_lock(&qstatelock); + (*state)->ref_cnt--; + /* + * If there are no references wakup the allocating thread + */ + if ((*state)->ref_cnt == 0) + (void) pthread_cond_signal(&(*state)->cv); + (void) pthread_mutex_unlock(&qstatelock); + *state = NULL; +} + +/* + * This routine frees the idmap_query_state_t structure + * If the reference count is greater than 1 it waits + * for the other threads to finish using it. + */ void -idmap_lookup_free_batch(idmap_query_state_t **state) +idmap_lookup_release_batch(idmap_query_state_t **state) { idmap_query_state_t **p; - idmap_release_conn((*state)->qadh); + /* + * Decrement reference count with qstatelock locked + * and wait for reference count to get to zero + */ + (void) pthread_mutex_lock(&qstatelock); + (*state)->ref_cnt--; + while ((*state)->ref_cnt > 0) { + (void) pthread_cond_wait(&(*state)->cv, &qstatelock); + } /* Remove this state struct from the list of state structs */ - (void) pthread_mutex_lock(&qstatelock); for (p = &qstatehead; *p != NULL; p = &(*p)->next) { if (*p == (*state)) { *p = (*state)->next; @@ -1268,6 +1316,10 @@ } (void) pthread_mutex_unlock(&qstatelock); + (void) pthread_cond_destroy(&(*state)->cv); + + idmap_release_conn((*state)->qadh); + free(*state); *state = NULL; } @@ -1307,7 +1359,7 @@ } } - idmap_lookup_free_batch(state); + idmap_lookup_release_batch(state); return (retcode); }
--- a/usr/src/cmd/idmap/idmapd/adutils.h Thu Aug 16 23:36:19 2007 -0700 +++ b/usr/src/cmd/idmap/idmapd/adutils.h Fri Aug 17 08:49:55 2007 -0700 @@ -139,7 +139,7 @@ struct timeval *timeout); /* Abandon a batch and release its idmap_query_state_t object */ -void idmap_lookup_free_batch(idmap_query_state_t **state); +void idmap_lookup_release_batch(idmap_query_state_t **state); /* * Add a name->SID lookup
--- a/usr/src/cmd/idmap/idmapd/dbutils.c Thu Aug 16 23:36:19 2007 -0700 +++ b/usr/src/cmd/idmap/idmapd/dbutils.c Fri Aug 17 08:49:55 2007 -0700 @@ -40,12 +40,15 @@ #include <time.h> #include <pwd.h> #include <grp.h> +#include <pthread.h> +#include <assert.h> #include "idmapd.h" #include "adutils.h" #include "string.h" #include "idmap_priv.h" + static idmap_retcode sql_compile_n_step_once(sqlite *, char *, sqlite_vm **, int *, int, const char ***); static idmap_retcode lookup_wksids_name2sid(const char *, char **, @@ -65,12 +68,6 @@ #define LOCALRID_MIN 1000 -#define SLEEP_TIME 20 - -#define NANO_SLEEP(rqtp, nsec)\ - rqtp.tv_sec = 0;\ - rqtp.tv_nsec = nsec * (NANOSEC / MILLISEC);\ - (void) nanosleep(&rqtp, NULL); typedef enum init_db_option { @@ -78,6 +75,87 @@ REMOVE_IF_CORRUPT = 1 } init_db_option_t; +/* + * Thread specfic data to hold the database handles so that the + * databaes are not opened and closed for every request. It also + * contains the sqlite busy handler structure. + */ + +struct idmap_busy { + const char *name; + const int *delays; + int delay_size; + int total; + int sec; +}; + + +typedef struct idmap_tsd { + sqlite *db_db; + sqlite *cache_db; + struct idmap_busy cache_busy; + struct idmap_busy db_busy; +} idmap_tsd_t; + + + +static const int cache_delay_table[] = + { 1, 2, 5, 10, 15, 20, 25, 30, 35, 40, + 50, 50, 60, 70, 80, 90, 100}; + +static const int db_delay_table[] = + { 5, 10, 15, 20, 30, 40, 55, 70, 100}; + + +static pthread_key_t idmap_tsd_key; + +void +idmap_tsd_destroy(void *key) +{ + + idmap_tsd_t *tsd = (idmap_tsd_t *)key; + if (tsd) { + if (tsd->db_db) + (void) sqlite_close(tsd->db_db); + if (tsd->cache_db) + (void) sqlite_close(tsd->cache_db); + free(tsd); + } +} + +int +idmap_init_tsd_key(void) { + + return (pthread_key_create(&idmap_tsd_key, idmap_tsd_destroy)); +} + + + +idmap_tsd_t * +idmap_get_tsd(void) +{ + idmap_tsd_t *tsd; + + if ((tsd = pthread_getspecific(idmap_tsd_key)) == NULL) { + /* No thread specific data so create it */ + if ((tsd = malloc(sizeof (*tsd))) != NULL) { + /* Initialize thread specific data */ + (void) memset(tsd, 0, sizeof (*tsd)); + /* save the trhread specific data */ + if (pthread_setspecific(idmap_tsd_key, tsd) != 0) { + /* Can't store key */ + free(tsd); + tsd = NULL; + } + } else { + tsd = NULL; + } + } + + return (tsd); +} + + /* * Initialize 'dbname' using 'sql' @@ -160,26 +238,79 @@ return (rc); } + +/* + * This is the SQLite database busy handler that retries the SQL + * operation until it is successful. + */ +int +/* LINTED E_FUNC_ARG_UNUSED */ +idmap_sqlite_busy_handler(void *arg, const char *table_name, int count) +{ + struct idmap_busy *busy = arg; + int delay; + struct timespec rqtp; + + if (count == 1) { + busy->total = 0; + busy->sec = 2; + } + if (busy->total > 1000 * busy->sec) { + idmapdlog(LOG_ERR, + "Thread %d waited %d sec for the %s database", + pthread_self(), busy->sec, busy->name); + busy->sec++; + } + + if (count <= busy->delay_size) { + delay = busy->delays[count-1]; + } else { + delay = busy->delays[busy->delay_size - 1]; + } + busy->total += delay; + rqtp.tv_sec = 0; + rqtp.tv_nsec = delay * (NANOSEC / MILLISEC); + (void) nanosleep(&rqtp, NULL); + return (1); +} + + /* * Get the database handle */ idmap_retcode get_db_handle(sqlite **db) { char *errmsg; + idmap_tsd_t *tsd; /* - * TBD RFE: Retrieve the db handle from thread-specific storage + * Retrieve the db handle from thread-specific storage * If none exists, open and store in thread-specific storage. */ - - *db = sqlite_open(IDMAP_DBNAME, 0, &errmsg); - if (*db == NULL) { + if ((tsd = idmap_get_tsd()) == NULL) { idmapdlog(LOG_ERR, - "Error opening database %s (%s)", - IDMAP_DBNAME, CHECK_NULL(errmsg)); - sqlite_freemem(errmsg); - return (IDMAP_ERR_INTERNAL); + "Error getting thread specific data for %s", + IDMAP_DBNAME); + return (IDMAP_ERR_MEMORY); } + + if (tsd->db_db == NULL) { + tsd->db_db = sqlite_open(IDMAP_DBNAME, 0, &errmsg); + if (tsd->db_db == NULL) { + idmapdlog(LOG_ERR, + "Error opening database %s (%s)", + IDMAP_DBNAME, CHECK_NULL(errmsg)); + sqlite_freemem(errmsg); + return (IDMAP_ERR_INTERNAL); + } + tsd->db_busy.name = IDMAP_DBNAME; + tsd->db_busy.delays = db_delay_table; + tsd->db_busy.delay_size = sizeof (db_delay_table) / + sizeof (int); + sqlite_busy_handler(tsd->db_db, idmap_sqlite_busy_handler, + &tsd->db_busy); + } + *db = tsd->db_db; return (IDMAP_SUCCESS); } @@ -187,22 +318,38 @@ * Get the cache handle */ idmap_retcode -get_cache_handle(sqlite **db) { +get_cache_handle(sqlite **cache) { char *errmsg; + idmap_tsd_t *tsd; /* - * TBD RFE: Retrieve the db handle from thread-specific storage + * Retrieve the db handle from thread-specific storage * If none exists, open and store in thread-specific storage. */ - - *db = sqlite_open(IDMAP_CACHENAME, 0, &errmsg); - if (*db == NULL) { + if ((tsd = idmap_get_tsd()) == NULL) { idmapdlog(LOG_ERR, - "Error opening database %s (%s)", - IDMAP_CACHENAME, CHECK_NULL(errmsg)); - sqlite_freemem(errmsg); - return (IDMAP_ERR_INTERNAL); + "Error getting thread specific data for %s", + IDMAP_DBNAME); + return (IDMAP_ERR_MEMORY); } + + if (tsd->cache_db == NULL) { + tsd->cache_db = sqlite_open(IDMAP_CACHENAME, 0, &errmsg); + if (tsd->cache_db == NULL) { + idmapdlog(LOG_ERR, + "Error opening database %s (%s)", + IDMAP_CACHENAME, CHECK_NULL(errmsg)); + sqlite_freemem(errmsg); + return (IDMAP_ERR_INTERNAL); + } + tsd->cache_busy.name = IDMAP_CACHENAME; + tsd->cache_busy.delays = cache_delay_table; + tsd->cache_busy.delay_size = sizeof (cache_delay_table) / + sizeof (int); + sqlite_busy_handler(tsd->cache_db, idmap_sqlite_busy_handler, + &tsd->cache_busy); + } + *cache = tsd->cache_db; return (IDMAP_SUCCESS); } @@ -305,31 +452,16 @@ idmap_retcode sql_exec_no_cb(sqlite *db, char *sql) { char *errmsg = NULL; - int r, i, s; - struct timespec rqtp; + int r; idmap_retcode retcode; - for (i = 0, s = SLEEP_TIME; i < MAX_TRIES; i++, s *= 2) { - if (errmsg != NULL) { - sqlite_freemem(errmsg); - errmsg = NULL; - } - r = sqlite_exec(db, sql, NULL, NULL, &errmsg); - if (r != SQLITE_BUSY) - break; - NANO_SLEEP(rqtp, s); - } + r = sqlite_exec(db, sql, NULL, NULL, &errmsg); + assert(r != SQLITE_LOCKED && r != SQLITE_BUSY); if (r != SQLITE_OK) { idmapdlog(LOG_ERR, "Database error during %s (%s)", sql, CHECK_NULL(errmsg)); - if (r == SQLITE_BUSY) { - retcode = IDMAP_ERR_BUSY; - } else { - retcode = idmap_string2stat(errmsg); - if (retcode == IDMAP_ERR_OTHER) - retcode = idmapd_string2stat(errmsg); - } + retcode = idmapd_string2stat(errmsg); if (errmsg != NULL) sqlite_freemem(errmsg); return (retcode); @@ -382,40 +514,28 @@ list_svc_cb cb, void *result) { list_cb_data_t cb_data; char *errmsg = NULL; - int r, i, s; - struct timespec rqtp; + int r; idmap_retcode retcode = IDMAP_ERR_INTERNAL; (void) memset(&cb_data, 0, sizeof (cb_data)); cb_data.result = result; cb_data.limit = limit; - for (i = 0, s = SLEEP_TIME; i < MAX_TRIES; i++, s *= 2) { - r = sqlite_exec(db, sql, cb, &cb_data, &errmsg); - switch (r) { - case SQLITE_OK: - retcode = IDMAP_SUCCESS; - goto out; - case SQLITE_BUSY: - if (errmsg != NULL) { - sqlite_freemem(errmsg); - errmsg = NULL; - } - retcode = IDMAP_ERR_BUSY; - idmapdlog(LOG_DEBUG, - "Database busy, %d retries remaining", - MAX_TRIES - i - 1); - NANO_SLEEP(rqtp, s); - continue; - default: - retcode = IDMAP_ERR_INTERNAL; - idmapdlog(LOG_ERR, - "Database error during %s (%s)", - sql, CHECK_NULL(errmsg)); - goto out; - }; + + r = sqlite_exec(db, sql, cb, &cb_data, &errmsg); + assert(r != SQLITE_LOCKED && r != SQLITE_BUSY); + switch (r) { + case SQLITE_OK: + retcode = IDMAP_SUCCESS; + break; + + default: + retcode = IDMAP_ERR_INTERNAL; + idmapdlog(LOG_ERR, + "Database error during %s (%s)", + sql, CHECK_NULL(errmsg)); + break; } -out: if (errmsg != NULL) sqlite_freemem(errmsg); return (retcode); @@ -611,7 +731,7 @@ else dom = ""; } - sql = sqlite_mprintf("INSERT OR ROLLBACK into namerules " + sql = sqlite_mprintf("INSERT into namerules " "(is_user, windomain, winname, is_nt4, " "unixname, w2u_order, u2w_order) " "VALUES(%d, %Q, %Q, %d, %Q, %q, %q);", @@ -762,7 +882,6 @@ * * Return values: * IDMAP_SUCCESS - * IDMAP_ERR_BUSY * IDMAP_ERR_NOTFOUND * IDMAP_ERR_INTERNAL */ @@ -771,10 +890,9 @@ sql_compile_n_step_once(sqlite *db, char *sql, sqlite_vm **vm, int *ncol, int reqcol, const char ***values) { char *errmsg = NULL; - struct timespec rqtp; - int i, r, s; - - if (sqlite_compile(db, sql, NULL, vm, &errmsg) != SQLITE_OK) { + int r; + + if ((r = sqlite_compile(db, sql, NULL, vm, &errmsg)) != SQLITE_OK) { idmapdlog(LOG_ERR, "Database error during %s (%s)", sql, CHECK_NULL(errmsg)); @@ -782,18 +900,10 @@ return (IDMAP_ERR_INTERNAL); } - for (i = 0, s = SLEEP_TIME; i < MAX_TRIES; i++, s *= 2) { - r = sqlite_step(*vm, ncol, values, NULL); - if (r != SQLITE_BUSY) - break; - NANO_SLEEP(rqtp, s); - } - - if (r == SQLITE_BUSY) { - (void) sqlite_finalize(*vm, NULL); - *vm = NULL; - return (IDMAP_ERR_BUSY); - } else if (r == SQLITE_ROW) { + r = sqlite_step(*vm, ncol, values, NULL); + assert(r != SQLITE_LOCKED && r != SQLITE_BUSY); + + if (r == SQLITE_ROW) { if (ncol != NULL && *ncol < reqcol) { (void) sqlite_finalize(*vm, NULL); *vm = NULL; @@ -810,7 +920,7 @@ (void) sqlite_finalize(*vm, &errmsg); *vm = NULL; idmapdlog(LOG_ERR, "Database error during %s (%s)", - sql, CHECK_NULL(errmsg)); + sql, CHECK_NULL(errmsg)); sqlite_freemem(errmsg); return (IDMAP_ERR_INTERNAL); } @@ -1297,7 +1407,7 @@ } if (retcode == IDMAP_ERR_RETRIABLE_NET_ERR) - idmap_lookup_free_batch(&state->ad_lookup); + idmap_lookup_release_batch(&state->ad_lookup); else retcode = idmap_lookup_batch_end(&state->ad_lookup, NULL); @@ -1308,7 +1418,7 @@ out: idmapdlog(LOG_NOTICE, "Windows SID to user/group name lookup failed"); - idmap_lookup_free_batch(&state->ad_lookup); + idmap_lookup_release_batch(&state->ad_lookup); return (retcode); } @@ -1567,9 +1677,8 @@ char *end; const char **values; sqlite_vm *vm = NULL; - struct timespec rqtp; idmap_utf8str *str; - int ncol, r, i, s, is_user; + int ncol, r, i, is_user; const char *me = "name_based_mapping_sid2pid"; winname = req->id1name.idmap_utf8str_val; @@ -1613,18 +1722,11 @@ goto out; } - for (i = 0, s = SLEEP_TIME; ; ) { + for (; ; ) { r = sqlite_step(vm, &ncol, &values, NULL); - - if (r == SQLITE_BUSY) { - if (++i < MAX_TRIES) { - NANO_SLEEP(rqtp, s); - s *= 2; - continue; - } - retcode = IDMAP_ERR_BUSY; - goto out; - } else if (r == SQLITE_ROW) { + assert(r != SQLITE_LOCKED && r != SQLITE_BUSY); + + if (r == SQLITE_ROW) { if (ncol < 2) { retcode = IDMAP_ERR_INTERNAL; goto out; @@ -2262,13 +2364,13 @@ if (retcode != IDMAP_SUCCESS) { idmapdlog(LOG_ERR, "Failed to batch name2sid for AD lookup"); - idmap_lookup_free_batch(&qs); + idmap_lookup_release_batch(&qs); return (IDMAP_ERR_INTERNAL); } out: if (retcode == IDMAP_ERR_RETRIABLE_NET_ERR) - idmap_lookup_free_batch(&qs); + idmap_lookup_release_batch(&qs); else retcode = idmap_lookup_batch_end(&qs, NULL); @@ -2352,8 +2454,7 @@ const char **values; sqlite_vm *vm = NULL; idmap_utf8str *str; - struct timespec rqtp; - int ncol, r, i, s; + int ncol, r; const char *me = "name_based_mapping_pid2sid"; RDLOCK_CONFIG(); @@ -2390,17 +2491,10 @@ goto out; } - for (i = 0, s = SLEEP_TIME; ; ) { + for (;;) { r = sqlite_step(vm, &ncol, &values, NULL); - if (r == SQLITE_BUSY) { - if (++i < MAX_TRIES) { - NANO_SLEEP(rqtp, s); - s *= 2; - continue; - } - retcode = IDMAP_ERR_BUSY; - goto out; - } else if (r == SQLITE_ROW) { + assert(r != SQLITE_LOCKED && r != SQLITE_BUSY); + if (r == SQLITE_ROW) { if (ncol < 3) { retcode = IDMAP_ERR_INTERNAL; goto out;
--- a/usr/src/cmd/idmap/idmapd/idmapd.c Thu Aug 16 23:36:19 2007 -0700 +++ b/usr/src/cmd/idmap/idmapd/idmapd.c Fri Aug 17 08:49:55 2007 -0700 @@ -243,6 +243,8 @@ } else (void) umask(0077); + idmap_init_tsd_key(); + init_idmapd(); if (__init_daemon_priv(PU_RESETGROUPS|PU_CLEARLIMITSET,
--- a/usr/src/cmd/idmap/idmapd/idmapd.h Thu Aug 16 23:36:19 2007 -0700 +++ b/usr/src/cmd/idmap/idmapd/idmapd.h Fri Aug 17 08:49:55 2007 -0700 @@ -154,6 +154,7 @@ extern void print_idmapdstate(); extern int create_directory(const char *, uid_t, gid_t); extern int load_config(); +extern int idmap_init_tsd_key(void); extern int init_dbs();
--- a/usr/src/cmd/idmap/idmapd/server.c Thu Aug 16 23:36:19 2007 -0700 +++ b/usr/src/cmd/idmap/idmapd/server.c Fri Aug 17 08:49:55 2007 -0700 @@ -257,10 +257,6 @@ result->ids.ids_len = 0; result->ids.ids_val = NULL; } - if (cache) - (void) sqlite_close(cache); - if (db) - (void) sqlite_close(db); result->retcode = idmap_stat4prot(result->retcode); return (TRUE); } @@ -386,8 +382,6 @@ sqlite_freemem(sql); if (IDMAP_ERROR(result->retcode)) (void) xdr_free(xdr_idmap_mappings_res, (caddr_t)result); - if (cache) - (void) sqlite_close(cache); result->retcode = idmap_stat4prot(result->retcode); return (TRUE); } @@ -559,8 +553,6 @@ sqlite_freemem(sql); if (IDMAP_ERROR(result->retcode)) (void) xdr_free(xdr_idmap_namerules_res, (caddr_t)result); - if (db) - (void) sqlite_close(db); result->retcode = idmap_stat4prot(result->retcode); return (TRUE); } @@ -617,6 +609,7 @@ sqlite *db = NULL; idmap_update_op *up; int i; + int trans = FALSE; if (verify_rules_auth(rqstp) < 0) { *result = IDMAP_ERR_PERMISSION_DENIED; @@ -637,6 +630,7 @@ *result = sql_exec_no_cb(db, "BEGIN TRANSACTION;"); if (*result != IDMAP_SUCCESS) goto out; + trans = TRUE; for (i = 0; i < batch.idmap_update_batch_len; i++) { up = &batch.idmap_update_batch_val[i]; @@ -666,12 +660,12 @@ } out: - if (*result == IDMAP_SUCCESS && db) { - *result = sql_exec_no_cb(db, "COMMIT TRANSACTION;"); + if (trans) { + if (*result == IDMAP_SUCCESS) + *result = sql_exec_no_cb(db, "COMMIT TRANSACTION;"); + else + (void) sql_exec_no_cb(db, "ROLLBACK TRANSACTION;"); } - - if (db) - (void) sqlite_close(db); *result = idmap_stat4prot(*result); return (TRUE); } @@ -735,10 +729,6 @@ result->mappings.mappings_len = 0; result->mappings.mappings_val = NULL; } - if (cache) - (void) sqlite_close(cache); - if (db) - (void) sqlite_close(db); result->retcode = idmap_stat4prot(result->retcode); return (TRUE); }