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.

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
@@ -37,22 +37,6 @@ __curstat_print_value(WT_SESSION_IMPL *session, uint64_t v, WT_ITEM *buf)
 }
 
 /*
- * __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: