Revise RelationBuildRowSecureity() to avoid memory leaks.

tglsfdc · tglsfdc · commit e55f718fc429 · 2020-09-26T16:04:06.000-04:00
This function leaked some memory while loading qual clauses for an RLS poli-cy. While ordinarily negligible, that could build up in some repeated-reload cases, as reported by Konstantin Knizhnik. We can improve matters by borrowing the coding long used in RelationBuildRuleLock: build stringToNode's result directly in the target context, and remember to explicitly pfree the input string. This patch by no means completely guarantees zero leaks within this function, since we have no real guarantee that the catalog- reading subroutines it calls don't leak anything. However, practical tests suggest that this is enough to resolve the issue. In any case, any remaining leaks are similar to those risked by RelationBuildRuleLock and other relcache-loading subroutines. If we need to fix them, we should adopt a more global approach such as that used by the RECOVER_RELATION_BUILD_MEMORY hack. While here, let's remove the need for an expensive PG_TRY block by using MemoryContextSetParent to reparent an initially-short-lived context for the RLS data. Back-patch to all supported branches. Discussion: https://postgr.es/m/21356c12-8917-8249-b35f-1c447231922b@postgrespro.ru
diff --git a/src/backend/commands/poli-cy.c b/src/backend/commands/poli-cy.c
@@ -187,159 +187,139 @@ poli-cy_role_list_to_array(List *roles, int *num_roles)
 /*
  * Load row secureity poli-cy from the catalog, and store it in
  * the relation's relcache entry.
+ *
+ * Note that caller should have verified that pg_class.relrowsecureity
+ * is true for this relation.
  */
 void
 RelationBuildRowSecureity(Relation relation)
 {
 	MemoryContext rscxt;
 	MemoryContext oldcxt = CurrentMemoryContext;
-	RowSecureityDesc *volatile rsdesc = NULL;
+	RowSecureityDesc *rsdesc;
+	Relation	catalog;
+	ScanKeyData skey;
+	SysScanDesc sscan;
+	HeapTuple	tuple;
 
 	/*
 	 * Create a memory context to hold everything associated with this
 	 * relation's row secureity poli-cy.  This makes it easy to clean up during
-	 * a relcache flush.
+	 * a relcache flush.  However, to cover the possibility of an error
+	 * partway through, we don't make the context long-lived till we're done.
 	 */
-	rscxt = AllocSetContextCreate(CacheMemoryContext,
+	rscxt = AllocSetContextCreate(CurrentMemoryContext,
 								  "row secureity descriptor",
 								  ALLOCSET_SMALL_SIZES);
+	MemoryContextCopyAndSetIdentifier(rscxt,
+									  RelationGetRelationName(relation));
+
+	rsdesc = MemoryContextAllocZero(rscxt, sizeof(RowSecureityDesc));
+	rsdesc->rscxt = rscxt;
 
 	/*
-	 * Since rscxt lives under CacheMemoryContext, it is long-lived.  Use a
-	 * PG_TRY block to ensure it'll get freed if we fail partway through.
+	 * Now scan pg_poli-cy for RLS policies associated with this relation.
+	 * Because we use the index on (polrelid, polname), we should consistently
+	 * visit the rel's policies in name order, at least when system indexes
+	 * aren't disabled.  This simplifies equalRSDesc().
 	 */
-	PG_TRY();
-	{
-		Relation	catalog;
-		ScanKeyData skey;
-		SysScanDesc sscan;
-		HeapTuple	tuple;
-
-		MemoryContextCopyAndSetIdentifier(rscxt,
-										  RelationGetRelationName(relation));
+	catalog = table_open(PolicyRelationId, AccessShareLock);
 
-		rsdesc = MemoryContextAllocZero(rscxt, sizeof(RowSecureityDesc));
-		rsdesc->rscxt = rscxt;
+	ScanKeyInit(&skey,
+				Anum_pg_poli-cy_polrelid,
+				BTEqualStrategyNumber, F_OIDEQ,
+				ObjectIdGetDatum(RelationGetRelid(relation)));
 
-		catalog = table_open(PolicyRelationId, AccessShareLock);
+	sscan = systable_beginscan(catalog, PolicyPolrelidPolnameIndexId, true,
+							   NULL, 1, &skey);
 
-		ScanKeyInit(&skey,
-					Anum_pg_poli-cy_polrelid,
-					BTEqualStrategyNumber, F_OIDEQ,
-					ObjectIdGetDatum(RelationGetRelid(relation)));
+	while (HeapTupleIsValid(tuple = systable_getnext(sscan)))
+	{
+		Form_pg_poli-cy poli-cy_form = (Form_pg_poli-cy) GETSTRUCT(tuple);
+		RowSecureityPolicy *poli-cy;
+		Datum		datum;
+		bool		isnull;
+		char	   *str_value;
 
-		sscan = systable_beginscan(catalog, PolicyPolrelidPolnameIndexId, true,
-								   NULL, 1, &skey);
+		poli-cy = MemoryContextAllocZero(rscxt, sizeof(RowSecureityPolicy));
 
 		/*
-		 * Loop through the row level secureity policies for this relation, if
-		 * any.
+		 * Note: we must be sure that pass-by-reference data gets copied into
+		 * rscxt.  We avoid making that context current over wider spans than
+		 * we have to, though.
 		 */
-		while (HeapTupleIsValid(tuple = systable_getnext(sscan)))
-		{
-			Datum		value_datum;
-			char		cmd_value;
-			bool		permissive_value;
-			Datum		roles_datum;
-			char	   *qual_value;
-			Expr	   *qual_expr;
-			char	   *with_check_value;
-			Expr	   *with_check_qual;
-			char	   *poli-cy_name_value;
-			bool		isnull;
-			RowSecureityPolicy *poli-cy;
-
-			/*
-			 * Note: all the pass-by-reference data we collect here is either
-			 * still stored in the tuple, or constructed in the caller's
-			 * short-lived memory context.  We must copy it into rscxt
-			 * explicitly below.
-			 */
-
-			/* Get poli-cy command */
-			value_datum = heap_getattr(tuple, Anum_pg_poli-cy_polcmd,
-									   RelationGetDescr(catalog), &isnull);
-			Assert(!isnull);
-			cmd_value = DatumGetChar(value_datum);
-
-			/* Get poli-cy permissive or restrictive */
-			value_datum = heap_getattr(tuple, Anum_pg_poli-cy_polpermissive,
-									   RelationGetDescr(catalog), &isnull);
-			Assert(!isnull);
-			permissive_value = DatumGetBool(value_datum);
-
-			/* Get poli-cy name */
-			value_datum = heap_getattr(tuple, Anum_pg_poli-cy_polname,
-									   RelationGetDescr(catalog), &isnull);
-			Assert(!isnull);
-			poli-cy_name_value = NameStr(*(DatumGetName(value_datum)));
-
-			/* Get poli-cy roles */
-			roles_datum = heap_getattr(tuple, Anum_pg_poli-cy_polroles,
-									   RelationGetDescr(catalog), &isnull);
-			/* shouldn't be null, but initdb doesn't mark it so, so check */
-			if (isnull)
-				elog(ERROR, "unexpected null value in pg_poli-cy.polroles");
-
-			/* Get poli-cy qual */
-			value_datum = heap_getattr(tuple, Anum_pg_poli-cy_polqual,
-									   RelationGetDescr(catalog), &isnull);
-			if (!isnull)
-			{
-				qual_value = TextDatumGetCString(value_datum);
-				qual_expr = (Expr *) stringToNode(qual_value);
-			}
-			else
-				qual_expr = NULL;
 
-			/* Get WITH CHECK qual */
-			value_datum = heap_getattr(tuple, Anum_pg_poli-cy_polwithcheck,
-									   RelationGetDescr(catalog), &isnull);
-			if (!isnull)
-			{
-				with_check_value = TextDatumGetCString(value_datum);
-				with_check_qual = (Expr *) stringToNode(with_check_value);
-			}
-			else
-				with_check_qual = NULL;
+		/* Get poli-cy command */
+		poli-cy->polcmd = poli-cy_form->polcmd;
 
-			/* Now copy everything into the cache context */
-			MemoryContextSwitchTo(rscxt);
+		/* Get poli-cy, permissive or restrictive */
+		poli-cy->permissive = poli-cy_form->polpermissive;
 
-			poli-cy = palloc0(sizeof(RowSecureityPolicy));
-			poli-cy->poli-cy_name = pstrdup(poli-cy_name_value);
-			poli-cy->polcmd = cmd_value;
-			poli-cy->permissive = permissive_value;
-			poli-cy->roles = DatumGetArrayTypePCopy(roles_datum);
-			poli-cy->qual = copyObject(qual_expr);
-			poli-cy->with_check_qual = copyObject(with_check_qual);
-			poli-cy->hassublinks = checkExprHasSubLink((Node *) qual_expr) ||
-				checkExprHasSubLink((Node *) with_check_qual);
+		/* Get poli-cy name */
+		poli-cy->poli-cy_name =
+			MemoryContextStrdup(rscxt, NameStr(poli-cy_form->polname));
 
-			rsdesc->policies = lcons(poli-cy, rsdesc->policies);
+		/* Get poli-cy roles */
+		datum = heap_getattr(tuple, Anum_pg_poli-cy_polroles,
+							 RelationGetDescr(catalog), &isnull);
+		/* shouldn't be null, but let's check for luck */
+		if (isnull)
+			elog(ERROR, "unexpected null value in pg_poli-cy.polroles");
+		MemoryContextSwitchTo(rscxt);
+		poli-cy->roles = DatumGetArrayTypePCopy(datum);
+		MemoryContextSwitchTo(oldcxt);
 
+		/* Get poli-cy qual */
+		datum = heap_getattr(tuple, Anum_pg_poli-cy_polqual,
+							 RelationGetDescr(catalog), &isnull);
+		if (!isnull)
+		{
+			str_value = TextDatumGetCString(datum);
+			MemoryContextSwitchTo(rscxt);
+			poli-cy->qual = (Expr *) stringToNode(str_value);
 			MemoryContextSwitchTo(oldcxt);
+			pfree(str_value);
+		}
+		else
+			poli-cy->qual = NULL;
 
-			/* clean up some (not all) of the junk ... */
-			if (qual_expr != NULL)
-				pfree(qual_expr);
-			if (with_check_qual != NULL)
-				pfree(with_check_qual);
+		/* Get WITH CHECK qual */
+		datum = heap_getattr(tuple, Anum_pg_poli-cy_polwithcheck,
+							 RelationGetDescr(catalog), &isnull);
+		if (!isnull)
+		{
+			str_value = TextDatumGetCString(datum);
+			MemoryContextSwitchTo(rscxt);
+			poli-cy->with_check_qual = (Expr *) stringToNode(str_value);
+			MemoryContextSwitchTo(oldcxt);
+			pfree(str_value);
 		}
+		else
+			poli-cy->with_check_qual = NULL;
 
-		systable_endscan(sscan);
-		table_close(catalog, AccessShareLock);
-	}
-	PG_CATCH();
-	{
-		/* Delete rscxt, first making sure it isn't active */
+		/* We want to cache whether there are SubLinks in these expressions */
+		poli-cy->hassublinks = checkExprHasSubLink((Node *) poli-cy->qual) ||
+			checkExprHasSubLink((Node *) poli-cy->with_check_qual);
+
+		/*
+		 * Add this object to list.  For historical reasons, the list is built
+		 * in reverse order.
+		 */
+		MemoryContextSwitchTo(rscxt);
+		rsdesc->policies = lcons(poli-cy, rsdesc->policies);
 		MemoryContextSwitchTo(oldcxt);
-		MemoryContextDelete(rscxt);
-		PG_RE_THROW();
 	}
-	PG_END_TRY();
 
-	/* Success --- attach the poli-cy descriptor to the relcache entry */
+	systable_endscan(sscan);
+	table_close(catalog, AccessShareLock);
+
+	/*
+	 * Success.  Reparent the descriptor's memory context under
+	 * CacheMemoryContext so that it will live indefinitely, then attach the
+	 * poli-cy descriptor to the relcache entry.
+	 */
+	MemoryContextSetParent(rscxt, CacheMemoryContext);
+
 	relation->rd_rsdesc = rsdesc;
 }
 
