WT-3373 Access violation due to a bug in internal page splitting (#3478)

When acquiring a lock on our parent internal page, we use the WT_REF.home
field to reference our parent page.  As a child of the parent page, we
prevent its eviction, but that's a weak guarantee. If the parent page
splits, and our WT_REF were to move with the split, the WT_REF.home field
might change underneath us and we could race, and end up attempting to
access an evicted page. Set the session page-index generation so if the
parent splits, it still can't be evicted.

dist/s_style

@@ -101,10 +101,13 @@ else
 		cat $t
 	fi
 
-	# Alignment directive before "struct".
-	egrep 'WT_COMPILER_TYPE_ALIGN.*struct' $f > $t
-	test -s $t && {
-		echo "$f: compiler alignment direction must precede \"struct\""
+	# If we don't have matching pack-begin and pack-end calls, we don't get
+	# an error, we just get a Windows performance regression. Using awk and
+	# not wc to ensure there's no whitespace in the assignment.
+	egrep WT_PACKED_STRUCT $f > $t
+	cnt=`awk 'BEGIN { line = 0 } { ++line } END { print line }' < $t`
+	test `expr "$cnt" % 2` -ne 0 && {
+		echo "$f: mismatched WT_PACKED_STRUCT_BEGIN/END lines"
 		cat $t
 	}
 

src/btree/bt_split.c

@@ -1256,12 +1256,12 @@ err:	switch (complete) {
 }
 
 /*
- * __split_internal_lock --
+ * __split_internal_lock_worker --
  *	Lock an internal page.
  */
 static int
-__split_internal_lock(WT_SESSION_IMPL *session, WT_REF *ref, bool trylock,
-    WT_PAGE **parentp, bool *hazardp)
+__split_internal_lock_worker(WT_SESSION_IMPL *session,
+    WT_REF *ref, bool trylock, WT_PAGE **parentp, bool *hazardp)
 {
 	WT_DECL_RET;
 	WT_PAGE *parent;
@@ -1339,6 +1339,32 @@ err:	WT_PAGE_UNLOCK(session, parent);
 	return (ret);
 }
 
+/*
+ * __split_internal_lock --
+ *	Lock an internal page.
+ */
+static int
+__split_internal_lock(WT_SESSION_IMPL *session,
+    WT_REF *ref, bool trylock, WT_PAGE **parentp, bool *hazardp)
+{
+	WT_DECL_RET;
+
+	/*
+	 * There's no lock on our parent page and we're about to acquire one,
+	 * which implies using the WT_REF.home field to reference our parent
+	 * page. As a child of the parent page, we prevent its eviction, but
+	 * that's a weak guarantee. If the parent page splits, and our WT_REF
+	 * were to move with the split, the WT_REF.home field might change
+	 * underneath us and we could race, and end up attempting to access
+	 * an evicted page. Set the session page-index generation so if the
+	 * parent splits, it still can't be evicted.
+	 */
+	WT_WITH_PAGE_INDEX(session,
+	    ret = __split_internal_lock_worker(
+	    session, ref, trylock, parentp, hazardp));
+	return (ret);
+}
+
 /*
  * __split_internal_unlock --
  *	Unlock the parent page.

src/conn/conn_dhandle.c

@@ -475,6 +475,49 @@ err:	WT_DHANDLE_RELEASE(dhandle);
 	return (ret);
 }
 
+/*
+ * __conn_dhandle_close_one --
+ *	Lock and, if necessary, close a data handle.
+ */
+static int
+__conn_dhandle_close_one(WT_SESSION_IMPL *session,
+    const char *uri, const char *checkpoint, bool force)
+{
+	WT_DECL_RET;
+
+	/*
+	 * Lock the handle exclusively.  If this is part of schema-changing
+	 * operation (indicated by metadata tracking being enabled), hold the
+	 * lock for the duration of the operation.
+	 */
+	WT_RET(__wt_session_get_btree(session, uri, checkpoint,
+	    NULL, WT_DHANDLE_EXCLUSIVE | WT_DHANDLE_LOCK_ONLY));
+	if (WT_META_TRACKING(session))
+		WT_RET(__wt_meta_track_handle_lock(session, false));
+
+	/*
+	 * We have an exclusive lock, which means there are no cursors open at
+	 * this point.  Close the handle, if necessary.
+	 */
+	if (F_ISSET(session->dhandle, WT_DHANDLE_OPEN)) {
+		__wt_meta_track_sub_on(session);
+		ret = __wt_conn_btree_sync_and_close(session, false, force);
+
+		/*
+		 * If the close succeeded, drop any locks it acquired.  If
+		 * there was a failure, this function will fail and the whole
+		 * transaction will be rolled back.
+		 */
+		if (ret == 0)
+			ret = __wt_meta_track_sub_off(session);
+	}
+
+	if (!WT_META_TRACKING(session))
+		WT_TRET(__wt_session_release_btree(session));
+
+	return (ret);
+}
+
 /*
  * __wt_conn_dhandle_close_all --
  *	Close all data handles handles with matching name (including all
@@ -495,48 +538,22 @@ __wt_conn_dhandle_close_all(
 	    F_ISSET(session, WT_SESSION_LOCKED_HANDLE_LIST_WRITE));
 	WT_ASSERT(session, session->dhandle == NULL);
 
+	/*
+	 * Lock the live handle first.  This ordering is important: we rely on
+	 * locking the live handle to fail fast if the tree is busy (e.g., with
+	 * cursors open or in a checkpoint).
+	 */
+	WT_ERR(__conn_dhandle_close_one(session, uri, NULL, force));
+
 	bucket = __wt_hash_city64(uri, strlen(uri)) % WT_HASH_ARRAY_SIZE;
 	TAILQ_FOREACH(dhandle, &conn->dhhash[bucket], hashq) {
 		if (strcmp(dhandle->name, uri) != 0 ||
+		    dhandle->checkpoint == NULL ||
 		    F_ISSET(dhandle, WT_DHANDLE_DEAD))
 			continue;
 
-		session->dhandle = dhandle;
-
-		/*
-		 * Lock the handle exclusively.  If this is part of
-		 * schema-changing operation (indicated by metadata tracking
-		 * being enabled), hold the lock for the duration of the
-		 * operation.
-		 */
-		WT_ERR(__wt_session_get_btree(session,
-		    dhandle->name, dhandle->checkpoint,
-		    NULL, WT_DHANDLE_EXCLUSIVE | WT_DHANDLE_LOCK_ONLY));
-		if (WT_META_TRACKING(session))
-			WT_ERR(__wt_meta_track_handle_lock(session, false));
-
-		/*
-		 * We have an exclusive lock, which means there are no cursors
-		 * open at this point.  Close the handle, if necessary.
-		 */
-		if (F_ISSET(dhandle, WT_DHANDLE_OPEN)) {
-			__wt_meta_track_sub_on(session);
-			ret = __wt_conn_btree_sync_and_close(
-			    session, false, force);
-
-			/*
-			 * If the close succeeded, drop any locks it acquired.
-			 * If there was a failure, this function will fail and
-			 * the whole transaction will be rolled back.
-			 */
-			if (ret == 0)
-				ret = __wt_meta_track_sub_off(session);
-		}
-
-		if (!WT_META_TRACKING(session))
-			WT_TRET(__wt_session_release_btree(session));
-
-		WT_ERR(ret);
+		WT_ERR(__conn_dhandle_close_one(
+		    session, dhandle->name, dhandle->checkpoint, force));
 	}
 
 err:	session->dhandle = NULL;

src/cursor/cur_table.c

@@ -943,6 +943,12 @@ __wt_curtable_open(WT_SESSION_IMPL *session,
 		    table->cgroups[0]->source, NULL, cfg, cursorp);
 
 		__wt_schema_release_table(session, table);
+		if (ret == 0) {
+			/* Fix up the public URI to match what was passed in. */
+			cursor = *cursorp;
+			__wt_free(session, cursor->uri);
+			WT_TRET(__wt_strdup(session, uri, &cursor->uri));
+		}
 		return (ret);
 	}
 

src/include/btmem.h

@@ -930,7 +930,7 @@ WT_PACKED_STRUCT_BEGIN(__wt_update)
 #define	WT_UPDATE_MEMSIZE(upd)						\
 	WT_ALIGN(sizeof(WT_UPDATE) +					\
 	    (WT_UPDATE_DELETED_ISSET(upd) ? 0 : (upd)->size), 32)
-};
+WT_PACKED_STRUCT_END
 
 /*
  * WT_INSERT --

src/lsm/lsm_cursor.c

@@ -1661,8 +1661,6 @@ __wt_clsm_close(WT_CURSOR *cursor)
 	/* In case we were somehow left positioned, clear that. */
 	__clsm_leave(clsm);
 
-	/* The WT_LSM_TREE owns the URI. */
-	cursor->uri = NULL;
 	if (clsm->lsm_tree != NULL)
 		__wt_lsm_tree_release(session, clsm->lsm_tree);
 	WT_TRET(__wt_cursor_close(cursor));
@@ -1744,7 +1742,7 @@ __wt_clsm_open(WT_SESSION_IMPL *session,
 	cursor = &clsm->iface;
 	*cursor = iface;
 	cursor->session = &session->iface;
-	cursor->uri = lsm_tree->name;
+	WT_ERR(__wt_strdup(session, lsm_tree->name, &cursor->uri));
 	cursor->key_format = lsm_tree->key_format;
 	cursor->value_format = lsm_tree->value_format;
 

src/session/session_dhandle.c

@@ -229,7 +229,8 @@ __wt_session_lock_dhandle(
 			WT_ASSERT(session, !F_ISSET(dhandle, WT_DHANDLE_DEAD));
 			return (0);
 		}
-		if (ret != EBUSY || (is_open && want_exclusive))
+		if (ret != EBUSY || (is_open && want_exclusive) ||
+		    LF_ISSET(WT_DHANDLE_LOCK_ONLY))
 			return (ret);
 		lock_busy = true;
 

test/suite/test_cursor01.py

@@ -99,6 +99,7 @@ class test_cursor01(wttest.WiredTigerTestCase):
         self.pr('creating cursor')
         cursor = self.session.open_cursor(tablearg, None, None)
         self.assertCursorHasNoKeyValue(cursor)
+        self.assertEqual(cursor.uri, tablearg)
 
         for i in range(0, self.nentries):
             cursor[self.genkey(i)] = self.genvalue(i)