From f258ae5ac12a0394c8c8559fe6aeefff06092266 Mon Sep 17 00:00:00 2001 From: Keith Bostic Date: Mon, 11 Jul 2016 11:23:05 -0400 Subject: WT-2693 Check open_cursor error paths for consistent handling (#2859) * WT-2693 Check open_cursor error paths for consistent handling There's a problem with cursor functions which don't call their typed close functions, relying instead on a simple free of the base cursor type because the __wt_cursor_init() function is the last function called before successful return. The problem is __wt_cursor_init() doesn't clean up after itself: for example, it allocates memory to hold the URI in WT_CURSOR.internal_uri, then has a number of subsequent potential error returns, and WT_CURSOR.internal_uri won't be free'd unless __wt_cursor_close() is called. In other words, we should always call the typed closed function (which in turn calls __wt_cursor_close()). Specific additional concerns: cursor/cur_backup.c: __wt_curbackup_open() doesn't turn off backups or clear the hot-backup flag if __wt_cursor_init() fails, which will hang the process. Remove the cleanup-on-error code from the __backup_start() function and rely instead on the backup cursor's close function to do the work. There's a problem if we fail the initial backup cursor test, in which case we don't want to do cleanup other than discarding the cursors themselves. Add the WT_CURBACKUP_LOCKER flag to the WT_CURSOR_BACKUP structure, which isn't set until we're holding the hot backup cursor flag, and use it to figure out how much cleanup needs to be done. cursor/cur_file.c: the change to call the __curfile_close() function has a problem with __wt_bulk_wrapup(), which assumes WT_CURSOR_BULK.reconcile has been initialized, change it to check for NULL. cursor/cur_file.c: the change to call the __curfile_close() function has a problem with incrementing the data-source's in-use counter. Instead of incrementing the counter in __wt_curfile_open() before calling __curfile_create(), increment it in __curfile_create() after the point where all failure paths close the cursor. cursor/cur_log.c: the change to call the __curlog_close() function means we may be required to clean up a cursor that has not yet acquired the log archive lock. Add the WT_CURLOG_ARCHIVE_LOCK flag to the WT_CURSOR_LOG structure so the close function knows if the log archive lock needs to be released. cursor/cur_log.c: remove the __curlog_reset() call from __curlog_close(), we're closing the cursor, resetting it doesn't do anything useful. cursor/cur_metadata.c: the change to call the __curmetadata_close() function is slightly tricky, be cautious about closing the underlying WT_CURSOR_METADATA.file_cursor field, it may not be set when we call __curmetadata_close(). * comment typo * fix a comment * Add explicit comment on the life-cycle of the WT_CURBACKUP_LOCKER flag. * Review comment, set a specific size on the flags field. Generally, the cursors all use uint8_t for flags, and I don't think it matters much what we use, use the same thing everywhere. --- src/btree/bt_split.c | 8 ++++---- src/cursor/cur_backup.c | 33 ++++++++++++++++++++++----------- src/cursor/cur_config.c | 4 ++-- src/cursor/cur_ds.c | 5 +---- src/cursor/cur_dump.c | 4 ++-- src/cursor/cur_file.c | 21 +++++++++++++-------- src/cursor/cur_log.c | 26 +++++++------------------- src/cursor/cur_metadata.c | 11 ++++++----- src/cursor/cur_stat.c | 28 ++++++++-------------------- src/include/cursor.h | 9 +++++++-- src/include/extern.h | 1 - src/lsm/lsm_cursor.c | 8 +++----- src/reconcile/rec_write.c | 3 ++- 13 files changed, 77 insertions(+), 84 deletions(-) diff --git a/src/btree/bt_split.c b/src/btree/bt_split.c index ee9c6ad23e8..ac352b79a25 100644 --- a/src/btree/bt_split.c +++ b/src/btree/bt_split.c @@ -1500,7 +1500,7 @@ __split_multi_inmem( * This code re-creates an in-memory page that is part of a set created * while evicting a large page, and adds references to any unresolved * update chains to the new page. We get here due to choosing to keep - * the results of a split in memory or because and update could not be + * the results of a split in memory or because an update could not be * written when attempting to evict a page. * * Clear the disk image and link the page into the passed-in WT_REF to @@ -2208,9 +2208,9 @@ __wt_split_rewrite(WT_SESSION_IMPL *session, WT_REF *ref) * * Build the new page. * - * Allocate a WT_REF because the error path uses routines that will ea - * free memory. The only field we need to set is the record number, as - * it's used by the search routines. + * Allocate a WT_REF, the error path calls routines that free memory. + * The only field we need to set is the record number, as it's used by + * the search routines. */ WT_RET(__wt_calloc_one(session, &new)); new->ref_recno = ref->ref_recno; diff --git a/src/cursor/cur_backup.c b/src/cursor/cur_backup.c index 4ee23008687..c37bc00b9b9 100644 --- a/src/cursor/cur_backup.c +++ b/src/cursor/cur_backup.c @@ -82,14 +82,25 @@ __curbackup_close(WT_CURSOR *cursor) CURSOR_API_CALL(cursor, session, close, NULL); - WT_TRET(__backup_cleanup_handles(session, cb)); + /* + * When starting a hot backup, we serialize hot backup cursors and set + * the connection's hot-backup flag. Once that's done, we set the + * cursor's backup-locker flag, implying the cursor owns all necessary + * cleanup (including removing temporary files), regardless of error or + * success. The cursor's backup-locker flag is never cleared (it's just + * discarded when the cursor is closed), because that cursor will never + * not be responsible for cleanup. + */ + if (F_ISSET(cb, WT_CURBACKUP_LOCKER)) { + WT_TRET(__backup_cleanup_handles(session, cb)); + WT_WITH_SCHEMA_LOCK(session, tret, + tret = __backup_stop(session)); + WT_TRET(tret); + } + WT_TRET(__wt_cursor_close(cursor)); session->bkp_cursor = NULL; - WT_WITH_SCHEMA_LOCK(session, tret, - tret = __backup_stop(session)); /* Stop the backup. */ - WT_TRET(tret); - err: API_END_RET(session, ret); } @@ -144,11 +155,11 @@ __wt_curbackup_open(WT_SESSION_IMPL *session, ret = __backup_start(session, cb, cfg))); WT_ERR(ret); - /* __wt_cursor_init is last so we don't have to clean up on error. */ WT_ERR(__wt_cursor_init(cursor, uri, NULL, cfg, cursorp)); if (0) { -err: __wt_free(session, cb); +err: WT_TRET(__curbackup_close(cursor)); + *cursorp = NULL; } return (ret); @@ -226,6 +237,9 @@ __backup_start( conn->hot_backup = true; WT_ERR(__wt_writeunlock(session, conn->hot_backup_lock)); + /* We're the lock holder, we own cleanup. */ + F_SET(cb, WT_CURBACKUP_LOCKER); + /* * Create a temporary backup file. This must be opened before * generating the list of targets in backup_uri. This file will @@ -282,10 +296,7 @@ err: /* Close the hot backup file. */ WT_TRET(__wt_fclose(session, &cb->bfs)); if (srcfs != NULL) WT_TRET(__wt_fclose(session, &srcfs)); - if (ret != 0) { - WT_TRET(__backup_cleanup_handles(session, cb)); - WT_TRET(__backup_stop(session)); - } else { + if (ret == 0) { WT_ASSERT(session, dest != NULL); WT_TRET(__wt_fs_rename(session, WT_BACKUP_TMP, dest)); } diff --git a/src/cursor/cur_config.c b/src/cursor/cur_config.c index e0d270e4245..2d3f3ffd176 100644 --- a/src/cursor/cur_config.c +++ b/src/cursor/cur_config.c @@ -58,11 +58,11 @@ __wt_curconfig_open(WT_SESSION_IMPL *session, cursor->session = &session->iface; cursor->key_format = cursor->value_format = "S"; - /* __wt_cursor_init is last so we don't have to clean up on error. */ WT_ERR(__wt_cursor_init(cursor, uri, NULL, cfg, cursorp)); if (0) { -err: __wt_free(session, cconfig); +err: WT_TRET(__curconfig_close(cursor)); + *cursorp = NULL; } return (ret); } diff --git a/src/cursor/cur_ds.c b/src/cursor/cur_ds.c index d2b8d81ab37..8d4b7a9384b 100644 --- a/src/cursor/cur_ds.c +++ b/src/cursor/cur_ds.c @@ -518,10 +518,7 @@ __wt_curds_open( source->flags = 0; if (0) { -err: if (F_ISSET(cursor, WT_CURSTD_OPEN)) - WT_TRET(cursor->close(cursor)); - else - __wt_free(session, data_source); +err: WT_TRET(__curds_close(cursor)); *cursorp = NULL; } diff --git a/src/cursor/cur_dump.c b/src/cursor/cur_dump.c index 595915df7b7..d7f18bb61ac 100644 --- a/src/cursor/cur_dump.c +++ b/src/cursor/cur_dump.c @@ -401,13 +401,13 @@ __wt_curdump_create(WT_CURSOR *child, WT_CURSOR *owner, WT_CURSOR **cursorp) cursor->json_private = child->json_private = json; } - /* __wt_cursor_init is last so we don't have to clean up on error. */ cfg[0] = WT_CONFIG_BASE(session, WT_SESSION_open_cursor); cfg[1] = NULL; WT_ERR(__wt_cursor_init(cursor, NULL, owner, cfg, cursorp)); if (0) { -err: __wt_free(session, cursor); +err: WT_TRET(__curdump_close(cursor)); + *cursorp = NULL; } return (ret); } diff --git a/src/cursor/cur_file.c b/src/cursor/cur_file.c index fac903b4770..94cd05e54ac 100644 --- a/src/cursor/cur_file.c +++ b/src/cursor/cur_file.c @@ -388,11 +388,11 @@ err: API_END_RET(session, ret); } /* - * __wt_curfile_create -- + * __curfile_create -- * Open a cursor for a given btree handle. */ -int -__wt_curfile_create(WT_SESSION_IMPL *session, +static int +__curfile_create(WT_SESSION_IMPL *session, WT_CURSOR *owner, const char *cfg[], bool bulk, bool bitmap, WT_CURSOR **cursorp) { @@ -439,6 +439,13 @@ __wt_curfile_create(WT_SESSION_IMPL *session, cursor->value_format = btree->value_format; cbt->btree = btree; + /* + * Increment the data-source's in-use counter; done now because closing + * the cursor will decrement it, and all failure paths from here close + * the cursor. + */ + __wt_cursor_dhandle_incr_use(session); + if (session->dhandle->checkpoint != NULL) F_SET(cbt, WT_CBT_NO_TXN); @@ -478,7 +485,6 @@ __wt_curfile_create(WT_SESSION_IMPL *session, /* Underlying btree initialization. */ __wt_btcur_open(cbt); - /* __wt_cursor_init is last so we don't have to clean up on error. */ WT_ERR(__wt_cursor_init( cursor, cursor->internal_uri, owner, cfg, cursorp)); @@ -486,7 +492,8 @@ __wt_curfile_create(WT_SESSION_IMPL *session, WT_STAT_FAST_DATA_INCR(session, cursor_create); if (0) { -err: __wt_free(session, cbt); +err: WT_TRET(__curfile_close(cursor)); + *cursorp = NULL; } return (ret); @@ -555,10 +562,8 @@ __wt_curfile_open(WT_SESSION_IMPL *session, const char *uri, } else WT_RET(__wt_bad_object_type(session, uri)); - WT_ERR(__wt_curfile_create(session, owner, cfg, bulk, bitmap, cursorp)); + WT_ERR(__curfile_create(session, owner, cfg, bulk, bitmap, cursorp)); - /* Increment the data-source's in-use counter. */ - __wt_cursor_dhandle_incr_use(session); return (0); err: /* If the cursor could not be opened, release the handle. */ diff --git a/src/cursor/cur_log.c b/src/cursor/cur_log.c index 0a13803da5d..2adf0c2b8ab 100644 --- a/src/cursor/cur_log.c +++ b/src/cursor/cur_log.c @@ -315,16 +315,16 @@ __curlog_close(WT_CURSOR *cursor) WT_CONNECTION_IMPL *conn; WT_CURSOR_LOG *cl; WT_DECL_RET; - WT_LOG *log; WT_SESSION_IMPL *session; CURSOR_API_CALL(cursor, session, close, NULL); cl = (WT_CURSOR_LOG *)cursor; conn = S2C(session); + WT_ASSERT(session, FLD_ISSET(conn->log_flags, WT_CONN_LOG_ENABLED)); - log = conn->log; - WT_TRET(__wt_readunlock(session, log->log_archive_lock)); - WT_TRET(__curlog_reset(cursor)); + if (F_ISSET(cl, WT_CURLOG_ARCHIVE_LOCK)) + WT_TRET(__wt_readunlock(session, conn->log->log_archive_lock)); + __wt_free(session, cl->cur_lsn); __wt_free(session, cl->next_lsn); __wt_scr_free(session, &cl->logrec); @@ -332,6 +332,7 @@ __curlog_close(WT_CURSOR *cursor) __wt_scr_free(session, &cl->opvalue); __wt_free(session, cl->packed_key); __wt_free(session, cl->packed_value); + WT_TRET(__wt_cursor_close(cursor)); err: API_END_RET(session, ret); @@ -401,23 +402,10 @@ __wt_curlog_open(WT_SESSION_IMPL *session, /* Log cursors block archiving. */ WT_ERR(__wt_readlock(session, log->log_archive_lock)); + F_SET(cl, WT_CURLOG_ARCHIVE_LOCK); if (0) { -err: if (F_ISSET(cursor, WT_CURSTD_OPEN)) - WT_TRET(cursor->close(cursor)); - else { - __wt_free(session, cl->cur_lsn); - __wt_free(session, cl->next_lsn); - __wt_scr_free(session, &cl->logrec); - __wt_scr_free(session, &cl->opkey); - __wt_scr_free(session, &cl->opvalue); - /* - * NOTE: We cannot get on the error path with the - * readlock held. No need to unlock it unless that - * changes above. - */ - __wt_free(session, cl); - } +err: WT_TRET(__curlog_close(cursor)); *cursorp = NULL; } diff --git a/src/cursor/cur_metadata.c b/src/cursor/cur_metadata.c index 3d702e2ea8c..fc63ca13f7c 100644 --- a/src/cursor/cur_metadata.c +++ b/src/cursor/cur_metadata.c @@ -475,9 +475,11 @@ __curmetadata_close(WT_CURSOR *cursor) mdc = (WT_CURSOR_METADATA *)cursor; file_cursor = mdc->file_cursor; CURSOR_API_CALL(cursor, session, - close, ((WT_CURSOR_BTREE *)file_cursor)->btree); + close, file_cursor == NULL ? + NULL : ((WT_CURSOR_BTREE *)file_cursor)->btree); - ret = file_cursor->close(file_cursor); + if (file_cursor != NULL) + ret = file_cursor->close(file_cursor); WT_TRET(__wt_cursor_close(cursor)); err: API_END_RET(session, ret); @@ -552,9 +554,8 @@ __wt_curmetadata_open(WT_SESSION_IMPL *session, } if (0) { -err: if (mdc->file_cursor != NULL) - WT_TRET(mdc->file_cursor->close(mdc->file_cursor)); - __wt_free(session, mdc); +err: WT_TRET(__curmetadata_close(cursor)); + *cursorp = NULL; } return (ret); } diff --git a/src/cursor/cur_stat.c b/src/cursor/cur_stat.c index f7a8f5fc866..5c9159a4c0b 100644 --- a/src/cursor/cur_stat.c +++ b/src/cursor/cur_stat.c @@ -36,22 +36,6 @@ __curstat_print_value(WT_SESSION_IMPL *session, uint64_t v, WT_ITEM *buf) return (0); } -/* - * __curstat_free_config -- - * Free the saved configuration string stack - */ -static void -__curstat_free_config(WT_SESSION_IMPL *session, WT_CURSOR_STAT *cst) -{ - size_t i; - - if (cst->cfg != NULL) { - for (i = 0; cst->cfg[i] != NULL; ++i) - __wt_free(session, cst->cfg[i]); - __wt_free(session, cst->cfg); - } -} - /* * __curstat_get_key -- * WT_CURSOR->get_key for statistics cursors. @@ -334,11 +318,16 @@ __curstat_close(WT_CURSOR *cursor) WT_CURSOR_STAT *cst; WT_DECL_RET; WT_SESSION_IMPL *session; + size_t i; cst = (WT_CURSOR_STAT *)cursor; CURSOR_API_CALL(cursor, session, close, NULL); - __curstat_free_config(session, cst); + if (cst->cfg != NULL) { + for (i = 0; cst->cfg[i] != NULL; ++i) + __wt_free(session, cst->cfg[i]); + __wt_free(session, cst->cfg); + } __wt_buf_free(session, &cst->pv); __wt_free(session, cst->desc_buf); @@ -691,7 +680,6 @@ __wt_curstat_open(WT_SESSION_IMPL *session, /* The cursor isn't yet positioned. */ cst->notpositioned = true; - /* __wt_cursor_init is last so we don't have to clean up on error. */ WT_ERR(__wt_cursor_init(cursor, uri, NULL, cfg, cursorp)); if (0) { @@ -701,8 +689,8 @@ config_err: WT_ERR_MSG(session, EINVAL, } if (0) { -err: __curstat_free_config(session, cst); - __wt_free(session, cst); +err: WT_TRET(__curstat_close(cursor)); + *cursorp = NULL; } return (ret); diff --git a/src/include/cursor.h b/src/include/cursor.h index 6357523a03f..dce24f20844 100644 --- a/src/include/cursor.h +++ b/src/include/cursor.h @@ -73,6 +73,9 @@ struct __wt_cursor_backup { WT_CURSOR_BACKUP_ENTRY *list; /* List of files to be copied. */ size_t list_allocated; size_t list_next; + +#define WT_CURBACKUP_LOCKER 0x01 /* Hot-backup started */ + uint8_t flags; }; #define WT_CURSOR_BACKUP_ID(cursor) (((WT_CURSOR_BACKUP *)cursor)->maxid) @@ -413,7 +416,9 @@ struct __wt_cursor_log { uint32_t step_count; /* Intra-record count */ uint32_t rectype; /* Record type */ uint64_t txnid; /* Record txnid */ - uint32_t flags; + +#define WT_CURLOG_ARCHIVE_LOCK 0x01 /* Archive lock held */ + uint8_t flags; }; struct __wt_cursor_metadata { @@ -424,7 +429,7 @@ struct __wt_cursor_metadata { #define WT_MDC_CREATEONLY 0x01 #define WT_MDC_ONMETADATA 0x02 #define WT_MDC_POSITIONED 0x04 - uint32_t flags; + uint8_t flags; }; struct __wt_join_stats_group { diff --git a/src/include/extern.h b/src/include/extern.h index 0cf8151b674..0cfcb12fdf4 100644 --- a/src/include/extern.h +++ b/src/include/extern.h @@ -282,7 +282,6 @@ extern int __wt_curconfig_open(WT_SESSION_IMPL *session, const char *uri, const extern int __wt_curds_open( WT_SESSION_IMPL *session, const char *uri, WT_CURSOR *owner, const char *cfg[], WT_DATA_SOURCE *dsrc, WT_CURSOR **cursorp); extern int __wt_curdump_create(WT_CURSOR *child, WT_CURSOR *owner, WT_CURSOR **cursorp); extern int __wt_curfile_update_check(WT_CURSOR *cursor); -extern int __wt_curfile_create(WT_SESSION_IMPL *session, WT_CURSOR *owner, const char *cfg[], bool bulk, bool bitmap, WT_CURSOR **cursorp); extern int __wt_curfile_open(WT_SESSION_IMPL *session, const char *uri, WT_CURSOR *owner, const char *cfg[], WT_CURSOR **cursorp); extern int __wt_curindex_open(WT_SESSION_IMPL *session, const char *uri, WT_CURSOR *owner, const char *cfg[], WT_CURSOR **cursorp); extern int __wt_curjoin_joined(WT_CURSOR *cursor); diff --git a/src/lsm/lsm_cursor.c b/src/lsm/lsm_cursor.c index 78235fb6a92..0fb917e8f4c 100644 --- a/src/lsm/lsm_cursor.c +++ b/src/lsm/lsm_cursor.c @@ -1521,6 +1521,8 @@ __wt_clsm_open(WT_SESSION_IMPL *session, WT_LSM_TREE *lsm_tree; bool bulk; + WT_STATIC_ASSERT(offsetof(WT_CURSOR_LSM, iface) == 0); + clsm = NULL; cursor = NULL; lsm_tree = NULL; @@ -1566,6 +1568,7 @@ __wt_clsm_open(WT_SESSION_IMPL *session, cursor->value_format = lsm_tree->value_format; clsm->lsm_tree = lsm_tree; + lsm_tree = NULL; /* * The tree's dsk_gen starts at one, so starting the cursor on zero @@ -1573,7 +1576,6 @@ __wt_clsm_open(WT_SESSION_IMPL *session, */ clsm->dsk_gen = 0; - WT_STATIC_ASSERT(offsetof(WT_CURSOR_LSM, iface) == 0); WT_ERR(__wt_cursor_init(cursor, cursor->uri, owner, cfg, cursorp)); if (bulk) @@ -1585,10 +1587,6 @@ err: if (clsm != NULL) else if (lsm_tree != NULL) __wt_lsm_tree_release(session, lsm_tree); - /* - * We open bulk cursors after setting the returned cursor. - * Fix that here. - */ *cursorp = NULL; } diff --git a/src/reconcile/rec_write.c b/src/reconcile/rec_write.c index da35747f97b..905e81a2e7e 100644 --- a/src/reconcile/rec_write.c +++ b/src/reconcile/rec_write.c @@ -3556,8 +3556,9 @@ __wt_bulk_wrapup(WT_SESSION_IMPL *session, WT_CURSOR_BULK *cbulk) WT_PAGE *parent; WT_RECONCILE *r; - r = cbulk->reconcile; btree = S2BT(session); + if ((r = cbulk->reconcile) == NULL) + return (0); switch (btree->type) { case BTREE_COL_FIX: -- cgit v1.2.1