Fix race condition in TupleDescCompactAttr assert code

david-rowley · david-rowley · commit 6f3820f37aba · 2024-12-24T14:54:24.000+13:00
5983a4c added CompactAttribute as an abbreviated alternative to FormData_pg_attribute to allow more cache-friendly processing in tasks related to TupleDescs. That commit contained some assert-only code to check that the CompactAttribute had been populated correctly, however, the method used to do that checking caused the TupleDesc's CompactAttribute to be zeroed before it was repopulated and compared to the snapshot taken before the memset call. This caused issues as the type cache caches TupleDescs in shared memory which can be used by multiple backend processes at the same time. There was a window of time between the zero and repopulation of the CompactAttribute where another process would mistakenly think that the CompactAttribute is invalid due to the memset. To fix this, instead of taking a snapshot of the CompactAttribute and calling populate_compact_attribute() and comparing the snapshot to the freshly populated TupleDesc's CompactAttribute, refactor things so we can just populate a temporary CompactAttribute on the stack. This way we don't touch the TupleDesc's memory. Reported-by: Alexander Lakhin, SQLsmith Discussion: https://postgr.es/m/ca3a256a-5d12-42db-aabe-a75a030d9fb9@gmail.com
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
@@ -57,17 +57,13 @@ ResourceOwnerForgetTupleDesc(ResourceOwner owner, TupleDesc tupdesc)
 }
 
 /*
- * populate_compact_attribute
- *		Fill in the corresponding CompactAttribute element from the
- *		Form_pg_attribute for the given attribute number.  This must be called
- *		whenever a change is made to a Form_pg_attribute in the TupleDesc.
+ * populate_compact_attribute_internal
+ *		Helper function for populate_compact_attribute()
  */
-void
-populate_compact_attribute(TupleDesc tupdesc, int attnum)
+static inline void
+populate_compact_attribute_internal(Form_pg_attribute src,
+									CompactAttribute *dst)
 {
-	Form_pg_attribute src = TupleDescAttr(tupdesc, attnum);
-	CompactAttribute *dst = &tupdesc->compact_attrs[attnum];
-
 	memset(dst, 0, sizeof(CompactAttribute));
 
 	dst->attcacheoff = -1;
@@ -101,6 +97,63 @@ populate_compact_attribute(TupleDesc tupdesc, int attnum)
 	}
 }
 
+/*
+ * populate_compact_attribute
+ *		Fill in the corresponding CompactAttribute element from the
+ *		Form_pg_attribute for the given attribute number.  This must be called
+ *		whenever a change is made to a Form_pg_attribute in the TupleDesc.
+ */
+void
+populate_compact_attribute(TupleDesc tupdesc, int attnum)
+{
+	Form_pg_attribute src = TupleDescAttr(tupdesc, attnum);
+	CompactAttribute *dst;
+
+	/*
+	 * Don't use TupleDescCompactAttr to prevent infinite recursion in assert
+	 * builds.
+	 */
+	dst = &tupdesc->compact_attrs[attnum];
+
+	populate_compact_attribute_internal(src, dst);
+}
+
+#ifdef USE_ASSERT_CHECKING
+/*
+ * verify_compact_attribute
+ *		In Assert enabled builds, we verify that the CompactAttribute is
+ *		populated correctly.  This helps find bugs in places such as ALTER
+ *		TABLE where code makes changes to the FormData_pg_attribute but
+ *		forgets to call populate_compact_attribute().
+ *
+ * This is used in TupleDescCompactAttr(), but declared here to allow access
+ * to populate_compact_attribute_internal().
+ */
+void
+verify_compact_attribute(TupleDesc tupdesc, int attnum)
+{
+	CompactAttribute *cattr = &tupdesc->compact_attrs[attnum];
+	Form_pg_attribute attr = TupleDescAttr(tupdesc, attnum);
+	CompactAttribute tmp;
+
+	/*
+	 * Populate the temporary CompactAttribute from the corresponding
+	 * Form_pg_attribute
+	 */
+	populate_compact_attribute_internal(attr, &tmp);
+
+	/*
+	 * Make the attcacheoff match since it's been reset to -1 by
+	 * populate_compact_attribute_internal.
+	 */
+	tmp.attcacheoff = cattr->attcacheoff;
+
+	/* Check the freshly populated CompactAttribute matches the TupleDesc's */
+	Assert(memcmp(&tmp, cattr, sizeof(CompactAttribute)) == 0);
+}
+#endif
+
+
 /*
  * CreateTemplateTupleDesc
  *		This function allocates an empty tuple descriptor structure.
diff --git a/src/include/access/tupdesc.h b/src/include/access/tupdesc.h
@@ -158,37 +158,22 @@ TupleDescAttr(TupleDesc tupdesc, int i)
 
 #undef TupleDescAttrAddress
 
+#ifdef USE_ASSERT_CHECKING
+extern void verify_compact_attribute(TupleDesc, int attnum);
+#endif
+
 /*
  * Accessor for the i'th CompactAttribute element of tupdesc.
  */
 static inline CompactAttribute *
 TupleDescCompactAttr(TupleDesc tupdesc, int i)
 {
 	CompactAttribute *cattr = &tupdesc->compact_attrs[i];
-#ifdef USE_ASSERT_CHECKING
-	CompactAttribute snapshot;
 
-	/*
-	 * In Assert enabled builds we verify that the CompactAttribute is
-	 * populated correctly.  This helps find bugs in places such as ALTER
-	 * TABLE where code makes changes to the FormData_pg_attribute but forgets
-	 * to call populate_compact_attribute.
-	 */
-
-	/*
-	 * Take a snapshot of how the CompactAttribute is now before calling
-	 * populate_compact_attribute to make it up-to-date with the
-	 * FormData_pg_attribute.
-	 */
-	memcpy(&snapshot, cattr, sizeof(CompactAttribute));
-
-	populate_compact_attribute(tupdesc, i);
-
-	/* reset attcacheoff back to what it was */
-	cattr->attcacheoff = snapshot.attcacheoff;
+#ifdef USE_ASSERT_CHECKING
 
-	/* Ensure the snapshot matches the freshly populated CompactAttribute */
-	Assert(memcmp(&snapshot, cattr, sizeof(CompactAttribute)) == 0);
+	/* Check that the CompactAttribute is correctly populated */
+	verify_compact_attribute(tupdesc, i);
 #endif
 
 	return cattr;
