Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit acab757

Browse filesBrowse files
tglsfdctglsfdc
committed
Revise RelationBuildRowSecurity() to avoid memory leaks.
This function leaked some memory while loading qual clauses for an RLS policy. 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
1 parent 7c154f2 commit acab757
Copy full SHA for acab757

File tree

Expand file treeCollapse file tree

1 file changed

+95
-109
lines changed
Filter options
Expand file treeCollapse file tree

1 file changed

+95
-109
lines changed

‎src/backend/commands/policy.c

Copy file name to clipboardExpand all lines: src/backend/commands/policy.c
+95-109Lines changed: 95 additions & 109 deletions
Original file line numberDiff line numberDiff line change
@@ -186,148 +186,134 @@ policy_role_list_to_array(List *roles, int *num_roles)
186186
/*
187187
* Load row security policy from the catalog, and store it in
188188
* the relation's relcache entry.
189+
*
190+
* Note that caller should have verified that pg_class.relrowsecurity
191+
* is true for this relation.
189192
*/
190193
void
191194
RelationBuildRowSecurity(Relation relation)
192195
{
193196
MemoryContext rscxt;
194197
MemoryContext oldcxt = CurrentMemoryContext;
195-
RowSecurityDesc *volatile rsdesc = NULL;
198+
RowSecurityDesc *rsdesc;
199+
Relation catalog;
200+
ScanKeyData skey;
201+
SysScanDesc sscan;
202+
HeapTuple tuple;
196203

197204
/*
198205
* Create a memory context to hold everything associated with this
199206
* relation's row security policy. This makes it easy to clean up during
200-
* a relcache flush.
207+
* a relcache flush. However, to cover the possibility of an error
208+
* partway through, we don't make the context long-lived till we're done.
201209
*/
202-
rscxt = AllocSetContextCreate(CacheMemoryContext,
210+
rscxt = AllocSetContextCreate(CurrentMemoryContext,
203211
"row security descriptor",
204212
ALLOCSET_SMALL_SIZES);
205213

214+
rsdesc = MemoryContextAllocZero(rscxt, sizeof(RowSecurityDesc));
215+
rsdesc->rscxt = rscxt;
216+
206217
/*
207-
* Since rscxt lives under CacheMemoryContext, it is long-lived. Use a
208-
* PG_TRY block to ensure it'll get freed if we fail partway through.
218+
* Now scan pg_policy for RLS policies associated with this relation.
219+
* Because we use the index on (polrelid, polname), we should consistently
220+
* visit the rel's policies in name order, at least when system indexes
221+
* aren't disabled. This simplifies equalRSDesc().
209222
*/
210-
PG_TRY();
211-
{
212-
Relation catalog;
213-
ScanKeyData skey;
214-
SysScanDesc sscan;
215-
HeapTuple tuple;
223+
catalog = heap_open(PolicyRelationId, AccessShareLock);
216224

217-
rsdesc = MemoryContextAllocZero(rscxt, sizeof(RowSecurityDesc));
218-
rsdesc->rscxt = rscxt;
225+
ScanKeyInit(&skey,
226+
Anum_pg_policy_polrelid,
227+
BTEqualStrategyNumber, F_OIDEQ,
228+
ObjectIdGetDatum(RelationGetRelid(relation)));
219229

220-
catalog = heap_open(PolicyRelationId, AccessShareLock);
230+
sscan = systable_beginscan(catalog, PolicyPolrelidPolnameIndexId, true,
231+
NULL, 1, &skey);
221232

222-
ScanKeyInit(&skey,
223-
Anum_pg_policy_polrelid,
224-
BTEqualStrategyNumber, F_OIDEQ,
225-
ObjectIdGetDatum(RelationGetRelid(relation)));
233+
while (HeapTupleIsValid(tuple = systable_getnext(sscan)))
234+
{
235+
Form_pg_policy policy_form = (Form_pg_policy) GETSTRUCT(tuple);
236+
RowSecurityPolicy *policy;
237+
Datum datum;
238+
bool isnull;
239+
char *str_value;
226240

227-
sscan = systable_beginscan(catalog, PolicyPolrelidPolnameIndexId, true,
228-
NULL, 1, &skey);
241+
policy = MemoryContextAllocZero(rscxt, sizeof(RowSecurityPolicy));
229242

230243
/*
231-
* Loop through the row level security policies for this relation, if
232-
* any.
244+
* Note: we must be sure that pass-by-reference data gets copied into
245+
* rscxt. We avoid making that context current over wider spans than
246+
* we have to, though.
233247
*/
234-
while (HeapTupleIsValid(tuple = systable_getnext(sscan)))
235-
{
236-
Datum value_datum;
237-
char cmd_value;
238-
Datum roles_datum;
239-
char *qual_value;
240-
Expr *qual_expr;
241-
char *with_check_value;
242-
Expr *with_check_qual;
243-
char *policy_name_value;
244-
bool isnull;
245-
RowSecurityPolicy *policy;
246-
247-
/*
248-
* Note: all the pass-by-reference data we collect here is either
249-
* still stored in the tuple, or constructed in the caller's
250-
* short-lived memory context. We must copy it into rscxt
251-
* explicitly below.
252-
*/
253-
254-
/* Get policy command */
255-
value_datum = heap_getattr(tuple, Anum_pg_policy_polcmd,
256-
RelationGetDescr(catalog), &isnull);
257-
Assert(!isnull);
258-
cmd_value = DatumGetChar(value_datum);
259-
260-
/* Get policy name */
261-
value_datum = heap_getattr(tuple, Anum_pg_policy_polname,
262-
RelationGetDescr(catalog), &isnull);
263-
Assert(!isnull);
264-
policy_name_value = NameStr(*(DatumGetName(value_datum)));
265-
266-
/* Get policy roles */
267-
roles_datum = heap_getattr(tuple, Anum_pg_policy_polroles,
268-
RelationGetDescr(catalog), &isnull);
269-
/* shouldn't be null, but initdb doesn't mark it so, so check */
270-
if (isnull)
271-
elog(ERROR, "unexpected null value in pg_policy.polroles");
272-
273-
/* Get policy qual */
274-
value_datum = heap_getattr(tuple, Anum_pg_policy_polqual,
275-
RelationGetDescr(catalog), &isnull);
276-
if (!isnull)
277-
{
278-
qual_value = TextDatumGetCString(value_datum);
279-
qual_expr = (Expr *) stringToNode(qual_value);
280-
}
281-
else
282-
qual_expr = NULL;
283248

284-
/* Get WITH CHECK qual */
285-
value_datum = heap_getattr(tuple, Anum_pg_policy_polwithcheck,
286-
RelationGetDescr(catalog), &isnull);
287-
if (!isnull)
288-
{
289-
with_check_value = TextDatumGetCString(value_datum);
290-
with_check_qual = (Expr *) stringToNode(with_check_value);
291-
}
292-
else
293-
with_check_qual = NULL;
249+
/* Get policy command */
250+
policy->polcmd = policy_form->polcmd;
251+
252+
/* Get policy name */
253+
policy->policy_name =
254+
MemoryContextStrdup(rscxt, NameStr(policy_form->polname));
255+
256+
/* Get policy roles */
257+
datum = heap_getattr(tuple, Anum_pg_policy_polroles,
258+
RelationGetDescr(catalog), &isnull);
259+
/* shouldn't be null, but let's check for luck */
260+
if (isnull)
261+
elog(ERROR, "unexpected null value in pg_policy.polroles");
262+
MemoryContextSwitchTo(rscxt);
263+
policy->roles = DatumGetArrayTypePCopy(datum);
264+
MemoryContextSwitchTo(oldcxt);
294265

295-
/* Now copy everything into the cache context */
266+
/* Get policy qual */
267+
datum = heap_getattr(tuple, Anum_pg_policy_polqual,
268+
RelationGetDescr(catalog), &isnull);
269+
if (!isnull)
270+
{
271+
str_value = TextDatumGetCString(datum);
296272
MemoryContextSwitchTo(rscxt);
297-
298-
policy = palloc0(sizeof(RowSecurityPolicy));
299-
policy->policy_name = pstrdup(policy_name_value);
300-
policy->polcmd = cmd_value;
301-
policy->roles = DatumGetArrayTypePCopy(roles_datum);
302-
policy->qual = copyObject(qual_expr);
303-
policy->with_check_qual = copyObject(with_check_qual);
304-
policy->hassublinks = checkExprHasSubLink((Node *) qual_expr) ||
305-
checkExprHasSubLink((Node *) with_check_qual);
306-
307-
rsdesc->policies = lcons(policy, rsdesc->policies);
308-
273+
policy->qual = (Expr *) stringToNode(str_value);
309274
MemoryContextSwitchTo(oldcxt);
275+
pfree(str_value);
276+
}
277+
else
278+
policy->qual = NULL;
310279

311-
/* clean up some (not all) of the junk ... */
312-
if (qual_expr != NULL)
313-
pfree(qual_expr);
314-
if (with_check_qual != NULL)
315-
pfree(with_check_qual);
280+
/* Get WITH CHECK qual */
281+
datum = heap_getattr(tuple, Anum_pg_policy_polwithcheck,
282+
RelationGetDescr(catalog), &isnull);
283+
if (!isnull)
284+
{
285+
str_value = TextDatumGetCString(datum);
286+
MemoryContextSwitchTo(rscxt);
287+
policy->with_check_qual = (Expr *) stringToNode(str_value);
288+
MemoryContextSwitchTo(oldcxt);
289+
pfree(str_value);
316290
}
291+
else
292+
policy->with_check_qual = NULL;
317293

318-
systable_endscan(sscan);
319-
heap_close(catalog, AccessShareLock);
320-
}
321-
PG_CATCH();
322-
{
323-
/* Delete rscxt, first making sure it isn't active */
294+
/* We want to cache whether there are SubLinks in these expressions */
295+
policy->hassublinks = checkExprHasSubLink((Node *) policy->qual) ||
296+
checkExprHasSubLink((Node *) policy->with_check_qual);
297+
298+
/*
299+
* Add this object to list. For historical reasons, the list is built
300+
* in reverse order.
301+
*/
302+
MemoryContextSwitchTo(rscxt);
303+
rsdesc->policies = lcons(policy, rsdesc->policies);
324304
MemoryContextSwitchTo(oldcxt);
325-
MemoryContextDelete(rscxt);
326-
PG_RE_THROW();
327305
}
328-
PG_END_TRY();
329306

330-
/* Success --- attach the policy descriptor to the relcache entry */
307+
systable_endscan(sscan);
308+
heap_close(catalog, AccessShareLock);
309+
310+
/*
311+
* Success. Reparent the descriptor's memory context under
312+
* CacheMemoryContext so that it will live indefinitely, then attach the
313+
* policy descriptor to the relcache entry.
314+
*/
315+
MemoryContextSetParent(rscxt, CacheMemoryContext);
316+
331317
relation->rd_rsdesc = rsdesc;
332318
}
333319

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.