[PATCH] Prevent replacement of a function if it's used in an index expression and is not IMMUTABLE

Started by sundayjiang(蒋浩天)7 months ago2 messages
#1sundayjiang(蒋浩天)
sundayjiang@tencent.com
1 attachment(s)

Hi hackers,

The purpose of this patch is to prevent replacing a function via `CREATE OR REPLACE FUNCTION` with a new definition that is not marked as `IMMUTABLE`, if the existing function is referenced by an index expression.

Replacing such functions may lead to index corruption or runtime semantic inconsistencies, especially when the function’s output is not stable for the same input.

This patch is motivated by our analysis of both recent and earlier PostgreSQL security vulnerabilities, notably:

- CVE-2020-25695: A privilege escalation issue caused by non-IMMUTABLE expressions.
- CVE-2024-1713: A similar privilege escalation issue related to the `plv8` extension, akin to CVE-2020-25695.

Although these CVE vulnerabilities have been fixed, we believe this patch enforces a stricter rule that further enhances PostgreSQL’s robustness:

If a function is used in an index, it can only be replaced if it is declared as `IMMUTABLE`.

This strategy aligns with PostgreSQL’s established assumption that `IMMUTABLE` functions are safe to use in indexes and their behavior should remain consistent after updates.

We hope this contribution benefits the community, and we welcome your valuable feedback.

Sincerely, 
xiaojiluo (Tencent Yunding Lab) 

Attachments:

0001-Prevent-replacement-of-a-function-if-it-s-used-in-an.patchapplication/octet-stream; charset=utf-8; name=0001-Prevent-replacement-of-a-function-if-it-s-used-in-an.patchDownload
From 99aeb8868edd0cdb5406d1321198b018220b1e96 Mon Sep 17 00:00:00 2001
From: xiaojiluo <xiaojiluo@tencent.com>
Date: Wed, 25 Jun 2025 12:04:05 +0800
Subject: [PATCH] Prevent replacement of a function if it's used in an index
 expression and is not IMMUTABLE

In ProcedureCreate(), add a check to disallow replacing an existing function
if it is referenced by an index expression and is not marked as IMMUTABLE.
Replacing such a function could break index semantics or lead to inconsistent
behavior at runtime, especially if the function's output is not guaranteed
to be stable for the same input.

IMMUTABLE functions are assumed to always return the same output for the same
input and thus are considered safe to replace even when referenced in indexes.

This check adds a dependency scan on pg_depend to detect any usage of the
function in indexes. If the function is found to be in use and is not
IMMUTABLE, an error is thrown.

This refinement ensures safer function replacement behavior by blocking
redefinition only in cases where semantic consistency of indexed expressions
could be compromised.

Author: xiaojiluo <xiaojiluo@tencent.com>
---
 src/backend/catalog/pg_proc.c | 54 +++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index 5fdcf24d5f8..751f15a60de 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -26,6 +26,8 @@
 #include "catalog/pg_proc.h"
 #include "catalog/pg_transform.h"
 #include "catalog/pg_type.h"
+#include "catalog/pg_depend.h"
+#include "utils/fmgroids.h"
 #include "executor/functions.h"
 #include "funcapi.h"
 #include "mb/pg_wchar.h"
@@ -420,6 +422,58 @@ ProcedureCreate(const char *procedureName,
 					  errdetail("\"%s\" is a window function.", procedureName) :
 					  0)));
 
+		if(oldproc->prokind == PROKIND_FUNCTION && volatility != PROVOLATILE_IMMUTABLE){
+			Relation depRel = table_open(DependRelationId, AccessShareLock);
+			bool index_found = false;
+			SysScanDesc scan;
+			ScanKeyData key;
+			HeapTuple dtup;
+
+			/* refobjid = oldproc->oid */
+			ScanKeyInit(&key,
+						Anum_pg_depend_refobjid,
+						BTEqualStrategyNumber, F_OIDEQ,
+						ObjectIdGetDatum(oldproc->oid));
+
+			scan = systable_beginscan(depRel,
+									DependReferenceIndexId,
+									true,
+									NULL,
+									1, &key);
+
+			while (HeapTupleIsValid(dtup = systable_getnext(scan)))
+			{
+				Form_pg_depend d = (Form_pg_depend) GETSTRUCT(dtup);
+
+				if (d->classid == RelationRelationId && d->objsubid == 0)
+				{
+					/* query relkind */
+					HeapTuple reltup = SearchSysCache1(RELOID, ObjectIdGetDatum(d->objid));
+					if (HeapTupleIsValid(reltup))
+					{
+						Form_pg_class classForm = (Form_pg_class) GETSTRUCT(reltup);
+						if (classForm->relkind == RELKIND_INDEX)
+						{
+							index_found = true;
+							ReleaseSysCache(reltup);
+							break;
+						}
+						ReleaseSysCache(reltup);
+					}
+				}
+			}
+
+			systable_endscan(scan);
+			table_close(depRel, AccessShareLock);
+
+			if (index_found)
+				ereport(ERROR,
+						(errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
+						errmsg("cannot replace function \"%s\" with a non-IMMUTABLE function because it is used by an index",
+								procedureName)));
+
+		}
+
 		dropcmd = (prokind == PROKIND_PROCEDURE ? "DROP PROCEDURE" :
 				   prokind == PROKIND_AGGREGATE ? "DROP AGGREGATE" :
 				   "DROP FUNCTION");
-- 
2.43.0

#2jian he
jian.universality@gmail.com
In reply to: sundayjiang(蒋浩天) (#1)
Re: [PATCH] Prevent replacement of a function if it's used in an index expression and is not IMMUTABLE

On Mon, Jun 30, 2025 at 5:34 PM sundayjiang(蒋浩天)
<sundayjiang@tencent.com> wrote:

The purpose of this patch is to prevent replacing a function via `CREATE OR REPLACE FUNCTION` with a new definition that is not marked as `IMMUTABLE`, if the existing function is referenced by an index expression.

Replacing such functions may lead to index corruption or runtime semantic inconsistencies, especially when the function’s output is not stable for the same input.

If a function is used in an index, it can only be replaced if it is declared as `IMMUTABLE`.

looking at the right above code ``if (oldproc->prokind != prokind)``

+ if(oldproc->prokind == PROKIND_FUNCTION && volatility !=
PROVOLATILE_IMMUTABLE){

we can change it to

+        if(prokind == PROKIND_FUNCTION && oldproc->provolatile ==
PROVOLATILE_IMMUTABLE &&
+           volatility != PROVOLATILE_IMMUTABLE)
 +     {
curly brace generally begins with a new line.
if (index_found)
+ ereport(ERROR,
+ (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
+ errmsg("cannot replace function \"%s\" with a non-IMMUTABLE function
because it is used by an index",
+ procedureName)));
Here, errcode ERRCODE_FEATURE_NOT_SUPPORTED would be more appropriate.

you can add a simple test in src/test/regress/sql/create_function_sql.sql
for example:
CREATE OR REPLACE FUNCTION get_a_int(int default 2) RETURNS int
IMMUTABLE AS 'select $1' LANGUAGE sql;
create table t1(a int);
create index on t1((get_a_int(a)));
CREATE OR REPLACE FUNCTION get_a_int(int default 2) RETURNS int AS
'select $1' LANGUAGE sql;