MDEV-29856 heap-use-after-poison in row_merge_spatial_rows() w/ column prefix

spatial_index_info: Replaces index_tuple_info_t. Always take
a memory heap as a parameter to the member functions.
Remove pointer indirection for m_dtuple_vec.

spatial_index_info::add(): Duplicate any PRIMARY KEY fields that would
point to within ext->buf because that buffer will be allocated in
a shorter-lifetime memory heap.

mysql-test/suite/innodb_gis/r/alter_spatial_index.result

@@ -794,4 +794,14 @@ ENGINE=InnoDB;
 INSERT INTO t VALUES (REPEAT('MariaDB Corporation Ab ',351),POINT(0,0));
 ALTER TABLE t FORCE;
 DROP TABLE t;
+#
+# MDEV-29856 heap-use-after-poison in row_merge_spatial_rows()
+# with PRIMARY KEY on column prefix
+#
+CREATE TABLE t (id INT, f TEXT, s POINT NOT NULL,
+PRIMARY KEY(id,f(1)), SPATIAL(s)) ENGINE=InnoDB;
+INSERT INTO t VALUES
+(1,REPEAT('x',8192),@p:=ST_GeomFromText('POINT(0 0)')),(2,'',@p);
+ALTER TABLE t FORCE;
+DROP TABLE t;
 # End of 10.3 tests

mysql-test/suite/innodb_gis/t/alter_spatial_index.test

@@ -793,4 +793,16 @@ ALTER TABLE t FORCE;
 # Cleanup
 DROP TABLE t;
 
+--echo #
+--echo # MDEV-29856 heap-use-after-poison in row_merge_spatial_rows()
+--echo # with PRIMARY KEY on column prefix
+--echo #
+
+CREATE TABLE t (id INT, f TEXT, s POINT NOT NULL,
+                PRIMARY KEY(id,f(1)), SPATIAL(s)) ENGINE=InnoDB;
+INSERT INTO t VALUES
+(1,REPEAT('x',8192),@p:=ST_GeomFromText('POINT(0 0)')),(2,'',@p);
+ALTER TABLE t FORCE;
+DROP TABLE t;
+
 --echo # End of 10.3 tests

storage/innobase/row/row0merge.cc

@@ -60,63 +60,48 @@ Completed by Sunny Bains and Marko Makela
 /* Whether to disable file system cache */
 char	srv_disable_sort_file_cache;
 
-/** Class that caches index row tuples made from a single cluster
+/** Class that caches spatial index row tuples made from a single cluster
 index page scan, and then insert into corresponding index tree */
-class index_tuple_info_t {
+class spatial_index_info {
 public:
-	/** constructor
-	@param[in]	heap	memory heap
-	@param[in]	index	index to be created */
-	index_tuple_info_t(
-		mem_heap_t*	heap,
-		dict_index_t*	index) UNIV_NOTHROW
-	{
-		m_heap = heap;
-		m_index = index;
-		m_dtuple_vec = UT_NEW_NOKEY(idx_tuple_vec());
-	}
-
-	/** destructor */
-	~index_tuple_info_t()
-	{
-		UT_DELETE(m_dtuple_vec);
-	}
-
-	/** Get the index object
-	@return the index object */
-	dict_index_t*   get_index() UNIV_NOTHROW
-	{
-		return(m_index);
-	}
-
-	/** Caches an index row into index tuple vector
-	@param[in]	row	table row
-	@param[in]	ext	externally stored column
-	prefixes, or NULL */
-	void add(
-		const dtuple_t*		row,
-		const row_ext_t*	ext) UNIV_NOTHROW
-	{
-		dtuple_t*	dtuple;
-
-		dtuple = row_build_index_entry(row, ext, m_index, m_heap);
-
-		ut_ad(dtuple);
-
-		m_dtuple_vec->push_back(dtuple);
-	}
+  /** constructor
+  @param index	spatial index to be created */
+  spatial_index_info(dict_index_t *index) : index(index)
+  {
+    ut_ad(index->is_spatial());
+  }
+
+  /** Caches an index row into index tuple vector
+  @param[in]	row	table row
+  @param[in]	ext	externally stored column prefixes, or NULL */
+  void add(const dtuple_t *row, const row_ext_t *ext, mem_heap_t *heap)
+  {
+    dtuple_t *dtuple= row_build_index_entry(row, ext, index, heap);
+    ut_ad(dtuple);
+    ut_ad(dtuple->n_fields == index->n_fields);
+    if (ext)
+    {
+      /* Replace any references to ext, because ext will be allocated
+      from row_heap. */
+      for (ulint i= 1; i < dtuple->n_fields; i++)
+      {
+        dfield_t &dfield= dtuple->fields[i];
+        if (dfield.data >= ext->buf &&
+            dfield.data <= &ext->buf[ext->n_ext * ext->max_len])
+          dfield_dup(&dfield, heap);
+      }
+    }
+    m_dtuple_vec.push_back(dtuple);
+  }
 
 	/** Insert spatial index rows cached in vector into spatial index
 	@param[in]	trx_id		transaction id
-	@param[in,out]	row_heap	memory heap
 	@param[in]	pcur		cluster index scanning cursor
+	@param[in,out]	heap		temporary memory heap
 	@param[in,out]	scan_mtr	mini-transaction for pcur
 	@return DB_SUCCESS if successful, else error number */
-	inline dberr_t insert(
-		trx_id_t		trx_id,
-		mem_heap_t*		row_heap,
-		btr_pcur_t*		pcur,
-		mtr_t*			scan_mtr)
+	inline dberr_t insert(trx_id_t trx_id, btr_pcur_t* pcur,
+			      mem_heap_t* heap, mtr_t* scan_mtr)
 	{
 		big_rec_t*      big_rec;
 		rec_t*          rec;
@@ -130,14 +115,12 @@ class index_tuple_info_t {
 				       | BTR_NO_LOCKING_FLAG
 				       | BTR_KEEP_SYS_FLAG | BTR_CREATE_FLAG;
 
-		ut_ad(dict_index_is_spatial(m_index));
-
 		DBUG_EXECUTE_IF("row_merge_instrument_log_check_flush",
 			log_sys.check_flush_or_checkpoint = true;
 		);
 
-		for (idx_tuple_vec::iterator it = m_dtuple_vec->begin();
-		     it != m_dtuple_vec->end();
+		for (idx_tuple_vec::iterator it = m_dtuple_vec.begin();
+		     it != m_dtuple_vec.end();
 		     ++it) {
 			dtuple = *it;
 			ut_ad(dtuple);
@@ -153,14 +136,14 @@ class index_tuple_info_t {
 			}
 
 			mtr.start();
-			m_index->set_modified(mtr);
+			index->set_modified(mtr);
 
-			ins_cur.index = m_index;
-			rtr_init_rtr_info(&rtr_info, false, &ins_cur, m_index,
+			ins_cur.index = index;
+			rtr_init_rtr_info(&rtr_info, false, &ins_cur, index,
 					  false);
 			rtr_info_update_btr(&ins_cur, &rtr_info);
 
-			btr_cur_search_to_nth_level(m_index, 0, dtuple,
+			btr_cur_search_to_nth_level(index, 0, dtuple,
 						    PAGE_CUR_RTREE_INSERT,
 						    BTR_MODIFY_LEAF, &ins_cur,
 						     __FILE__, __LINE__,
@@ -169,44 +152,44 @@ class index_tuple_info_t {
 			/* It need to update MBR in parent entry,
 			so change search mode to BTR_MODIFY_TREE */
 			if (rtr_info.mbr_adj) {
-				mtr_commit(&mtr);
+				mtr.commit();
 				rtr_clean_rtr_info(&rtr_info, true);
 				rtr_init_rtr_info(&rtr_info, false, &ins_cur,
-						  m_index, false);
+						  index, false);
 				rtr_info_update_btr(&ins_cur, &rtr_info);
-				mtr_start(&mtr);
-				m_index->set_modified(mtr);
+				mtr.start();
+				index->set_modified(mtr);
 				btr_cur_search_to_nth_level(
-					m_index, 0, dtuple,
+					index, 0, dtuple,
 					PAGE_CUR_RTREE_INSERT,
 					BTR_MODIFY_TREE, &ins_cur,
 					__FILE__, __LINE__, &mtr);
 			}
 
 			error = btr_cur_optimistic_insert(
-				flag, &ins_cur, &ins_offsets, &row_heap,
+				flag, &ins_cur, &ins_offsets, &heap,
 				dtuple, &rec, &big_rec, 0, NULL, &mtr);
 
 			if (error == DB_FAIL) {
 				ut_ad(!big_rec);
 				mtr.commit();
 				mtr.start();
-				m_index->set_modified(mtr);
+				index->set_modified(mtr);
 
 				rtr_clean_rtr_info(&rtr_info, true);
 				rtr_init_rtr_info(&rtr_info, false,
-						  &ins_cur, m_index, false);
+						  &ins_cur, index, false);
 
 				rtr_info_update_btr(&ins_cur, &rtr_info);
 				btr_cur_search_to_nth_level(
-					m_index, 0, dtuple,
+					index, 0, dtuple,
 					PAGE_CUR_RTREE_INSERT,
 					BTR_MODIFY_TREE,
 					&ins_cur, __FILE__, __LINE__, &mtr);
 
 				error = btr_cur_pessimistic_insert(
 						flag, &ins_cur, &ins_offsets,
-						&row_heap, dtuple, &rec,
+						&heap, dtuple, &rec,
 						&big_rec, 0, NULL, &mtr);
 			}
 
@@ -229,30 +212,26 @@ class index_tuple_info_t {
 				}
 			}
 
-			mtr_commit(&mtr);
+			mtr.commit();
 
 			rtr_clean_rtr_info(&rtr_info, true);
 		}
 
-		m_dtuple_vec->clear();
+		m_dtuple_vec.clear();
 
 		return(error);
 	}
 
 private:
-	/** Cache index rows made from a cluster index scan. Usually
-	for rows on single cluster index page */
-	typedef std::vector<dtuple_t*, ut_allocator<dtuple_t*> >
-		idx_tuple_vec;
+  /** Cache index rows made from a cluster index scan. Usually
+  for rows on single cluster index page */
+  typedef std::vector<dtuple_t*, ut_allocator<dtuple_t*> > idx_tuple_vec;
 
-	/** vector used to cache index rows made from cluster index scan */
-	idx_tuple_vec*		m_dtuple_vec;
-
-	/** the index being built */
-	dict_index_t*		m_index;
-
-	/** memory heap for creating index tuples */
-	mem_heap_t*		m_heap;
+  /** vector used to cache index rows made from cluster index scan */
+  idx_tuple_vec m_dtuple_vec;
+public:
+  /** the index being built */
+  dict_index_t*const	index;
 };
 
 /* Maximum pending doc memory limit in bytes for a fts tokenization thread */
@@ -1566,7 +1545,6 @@ row_mtuple_cmp(
 @param[in]	trx_id		transaction id
 @param[in]	sp_tuples	cached spatial rows
 @param[in]	num_spatial	number of spatial indexes
-@param[in,out]	row_heap	heap for insert
 @param[in,out]	sp_heap		heap for tuples
 @param[in,out]	pcur		cluster index cursor
 @param[in,out]	mtr		mini transaction
@@ -1575,9 +1553,8 @@ static
 dberr_t
 row_merge_spatial_rows(
 	trx_id_t		trx_id,
-	index_tuple_info_t**	sp_tuples,
+	spatial_index_info**	sp_tuples,
 	ulint			num_spatial,
-	mem_heap_t*		row_heap,
 	mem_heap_t*		sp_heap,
 	btr_pcur_t*		pcur,
 	mtr_t*			mtr)
@@ -1591,7 +1568,7 @@ row_merge_spatial_rows(
 	ut_ad(sp_heap != NULL);
 
 	for (ulint j = 0; j < num_spatial; j++) {
-		err = sp_tuples[j]->insert(trx_id, row_heap, pcur, mtr);
+		err = sp_tuples[j]->insert(trx_id, pcur, sp_heap, mtr);
 
 		if (err != DB_SUCCESS) {
 			return(err);
@@ -1714,8 +1691,7 @@ row_merge_read_clustered_index(
 	os_event_t		fts_parallel_sort_event = NULL;
 	ibool			fts_pll_sort = FALSE;
 	int64_t			sig_count = 0;
-	index_tuple_info_t**	sp_tuples = NULL;
-	mem_heap_t*		sp_heap = NULL;
+	spatial_index_info**	sp_tuples = NULL;
 	ulint			num_spatial = 0;
 	BtrBulk*		clust_btr_bulk = NULL;
 	bool			clust_temp_file = false;
@@ -1805,9 +1781,7 @@ row_merge_read_clustered_index(
 	if (num_spatial > 0) {
 		ulint	count = 0;
 
-		sp_heap = mem_heap_create(512);
-
-		sp_tuples = static_cast<index_tuple_info_t**>(
+		sp_tuples = static_cast<spatial_index_info**>(
 			ut_malloc_nokey(num_spatial
 					* sizeof(*sp_tuples)));
 
@@ -1815,9 +1789,7 @@ row_merge_read_clustered_index(
 			if (dict_index_is_spatial(index[i])) {
 				sp_tuples[count]
 					= UT_NEW_NOKEY(
-						index_tuple_info_t(
-							sp_heap,
-							index[i]));
+						spatial_index_info(index[i]));
 				count++;
 			}
 		}
@@ -1948,7 +1920,7 @@ row_merge_read_clustered_index(
 			/* Insert the cached spatial index rows. */
 			err = row_merge_spatial_rows(
 				trx->id, sp_tuples, num_spatial,
-				row_heap, sp_heap, &pcur, &mtr);
+				row_heap, &pcur, &mtr);
 
 			if (err != DB_SUCCESS) {
 				goto func_exit;
@@ -2329,7 +2301,7 @@ row_merge_read_clustered_index(
 					continue;
 				}
 
-				ut_ad(sp_tuples[s_idx_cnt]->get_index()
+				ut_ad(sp_tuples[s_idx_cnt]->index
 				      == buf->index);
 
 				/* If the geometry field is invalid, report
@@ -2339,7 +2311,7 @@ row_merge_read_clustered_index(
 					break;
 				}
 
-				sp_tuples[s_idx_cnt]->add(row, ext);
+				sp_tuples[s_idx_cnt]->add(row, ext, buf->heap);
 				s_idx_cnt++;
 
 				continue;
@@ -2469,7 +2441,7 @@ row_merge_read_clustered_index(
 						err = row_merge_spatial_rows(
 							trx->id, sp_tuples,
 							num_spatial,
-							row_heap, sp_heap,
+							row_heap,
 							&pcur, &mtr);
 
 						if (err != DB_SUCCESS) {
@@ -2847,10 +2819,6 @@ row_merge_read_clustered_index(
 			UT_DELETE(sp_tuples[i]);
 		}
 		ut_free(sp_tuples);
-
-		if (sp_heap) {
-			mem_heap_free(sp_heap);
-		}
 	}
 
 	/* Update the next Doc ID we used. Table should be locked, so