Hello hackers,
As a follow-up for the CVE-2023-2454 fix, I think that it makes sense to
completely remove unsafe functions
PushOverrideSearchPath()/PopOverrideSearchPath(), which are not used in the
core now.
Please look at the patch attached.
Beside that, maybe it's worth to rename three functions in "Override" in
their names: GetOverrideSearchPath(), CopyOverrideSearchPath(),
OverrideSearchPathMatchesCurrent(), and then maybe struct OverrideSearchPath.
Noah Misch proposed name GetSearchPathMatcher() for the former.
What do you think?
Best regards,
Alexander
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 14e57adee2..93610f948e 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -67,9 +67,7 @@
* may be included:
*
* 1. If a TEMP table namespace has been initialized in this session, it
- * is implicitly searched first. (The only time this doesn't happen is
- * when we are obeying an override search path spec that says not to use the
- * temp namespace, or the temp namespace is included in the explicit list.)
+ * is implicitly searched first.
*
* 2. The system catalog namespace is always searched. If the system
* namespace is present in the explicit path then it will be searched in
@@ -108,19 +106,14 @@
* namespace (if it exists), preceded by the user's personal namespace
* (if one exists).
*
- * We support a stack of "override" search path settings for use within
- * specific sections of backend code. namespace_search_path is ignored
- * whenever the override stack is nonempty. activeSearchPath is always
- * the actually active path; it points either to the search list of the
- * topmost stack entry, or to baseSearchPath which is the list derived
- * from namespace_search_path.
+ * activeSearchPath is always the actually active path; it points to
+ * to baseSearchPath which is the list derived from namespace_search_path.
*
* If baseSearchPathValid is false, then baseSearchPath (and other
* derived variables) need to be recomputed from namespace_search_path.
* We mark it invalid upon an assignment to namespace_search_path or receipt
* of a syscache invalidation event for pg_namespace. The recomputation
- * is done during the next non-overridden lookup attempt. Note that an
- * override spec is never subject to recomputation.
+ * is done during the next lookup attempt.
*
* Any namespaces mentioned in namespace_search_path that are not readable
* by the current user ID are simply left out of baseSearchPath; so
@@ -161,17 +154,6 @@ static Oid namespaceUser = InvalidOid;
/* The above four values are valid only if baseSearchPathValid */
static bool baseSearchPathValid = true;
-/* Override requests are remembered in a stack of OverrideStackEntry structs */
-
-typedef struct
-{
- List *searchPath; /* the desired search path */
- Oid creationNamespace; /* the desired creation namespace */
- int nestLevel; /* subtransaction nesting level */
-} OverrideStackEntry;
-
-static List *overrideStack = NIL;
-
/*
* myTempNamespace is InvalidOid until and unless a TEMP namespace is set up
* in a particular backend session (this happens when a CREATE TEMP TABLE
@@ -3392,8 +3374,7 @@ SetTempNamespaceState(Oid tempNamespaceId, Oid tempToastNamespaceId)
/*
- * GetOverrideSearchPath - fetch current search path definition in form
- * used by PushOverrideSearchPath.
+ * GetOverrideSearchPath - fetch current search path definition.
*
* The result structure is allocated in the specified memory context
* (which might or might not be equal to CurrentMemoryContext); but any
@@ -3512,189 +3512,6 @@ OverrideSearchPathMatchesCurrent(OverrideSearchPath *path)
return true;
}
-/*
- * PushOverrideSearchPath - temporarily override the search path
- *
- * Do not use this function; almost any usage introduces a security
- * vulnerability. It exists for the benefit of legacy code running in
- * non-security-sensitive environments.
- *
- * We allow nested overrides, hence the push/pop terminology. The GUC
- * search_path variable is ignored while an override is active.
- *
- * It's possible that newpath->useTemp is set but there is no longer any
- * active temp namespace, if the path was saved during a transaction that
- * created a temp namespace and was later rolled back. In that case we just
- * ignore useTemp. A plausible alternative would be to create a new temp
- * namespace, but for existing callers that's not necessary because an empty
- * temp namespace wouldn't affect their results anyway.
- *
- * It's also worth noting that other schemas listed in newpath might not
- * exist anymore either. We don't worry about this because OIDs that match
- * no existing namespace will simply not produce any hits during searches.
- */
-void
-PushOverrideSearchPath(OverrideSearchPath *newpath)
-{
- OverrideStackEntry *entry;
- List *oidlist;
- Oid firstNS;
- MemoryContext oldcxt;
-
- /*
- * Copy the list for safekeeping, and insert implicitly-searched
- * namespaces as needed. This code should track recomputeNamespacePath.
- */
- oldcxt = MemoryContextSwitchTo(TopMemoryContext);
-
- oidlist = list_copy(newpath->schemas);
-
- /*
- * Remember the first member of the explicit list.
- */
- if (oidlist == NIL)
- firstNS = InvalidOid;
- else
- firstNS = linitial_oid(oidlist);
-
- /*
- * Add any implicitly-searched namespaces to the list. Note these go on
- * the front, not the back; also notice that we do not check USAGE
- * permissions for these.
- */
- if (newpath->addCatalog)
- oidlist = lcons_oid(PG_CATALOG_NAMESPACE, oidlist);
-
- if (newpath->addTemp && OidIsValid(myTempNamespace))
- oidlist = lcons_oid(myTempNamespace, oidlist);
-
- /*
- * Build the new stack entry, then insert it at the head of the list.
- */
- entry = (OverrideStackEntry *) palloc(sizeof(OverrideStackEntry));
- entry->searchPath = oidlist;
- entry->creationNamespace = firstNS;
- entry->nestLevel = GetCurrentTransactionNestLevel();
-
- overrideStack = lcons(entry, overrideStack);
-
- /* And make it active. */
- activeSearchPath = entry->searchPath;
- activeCreationNamespace = entry->creationNamespace;
- activeTempCreationPending = false; /* XXX is this OK? */
-
- /*
- * We always increment activePathGeneration when pushing/popping an
- * override path. In current usage, these actions always change the
- * effective path state, so there's no value in checking to see if it
- * didn't change.
- */
- activePathGeneration++;
-
- MemoryContextSwitchTo(oldcxt);
-}
-
-/*
- * PopOverrideSearchPath - undo a previous PushOverrideSearchPath
- *
- * Any push during a (sub)transaction will be popped automatically at abort.
- * But it's caller error if a push isn't popped in normal control flow.
- */
-void
-PopOverrideSearchPath(void)
-{
- OverrideStackEntry *entry;
-
- /* Sanity checks. */
- if (overrideStack == NIL)
- elog(ERROR, "bogus PopOverrideSearchPath call");
- entry = (OverrideStackEntry *) linitial(overrideStack);
- if (entry->nestLevel != GetCurrentTransactionNestLevel())
- elog(ERROR, "bogus PopOverrideSearchPath call");
-
- /* Pop the stack and free storage. */
- overrideStack = list_delete_first(overrideStack);
- list_free(entry->searchPath);
- pfree(entry);
-
- /* Activate the next level down. */
- if (overrideStack)
- {
- entry = (OverrideStackEntry *) linitial(overrideStack);
- activeSearchPath = entry->searchPath;
- activeCreationNamespace = entry->creationNamespace;
- activeTempCreationPending = false; /* XXX is this OK? */
- }
- else
- {
- /* If not baseSearchPathValid, this is useless but harmless */
- activeSearchPath = baseSearchPath;
- activeCreationNamespace = baseCreationNamespace;
- activeTempCreationPending = baseTempCreationPending;
- }
-
- /* As above, the generation always increments. */
- activePathGeneration++;
-}
-
-
-/*
- * get_collation_oid - find a collation by possibly qualified name
- *
- * Note that this will only find collations that work with the current
- * database's encoding.
- */
-Oid
-get_collation_oid(List *collname, bool missing_ok)
-{
- char *schemaname;
- char *collation_name;
- int32 dbencoding = GetDatabaseEncoding();
- Oid namespaceId;
- Oid colloid;
- ListCell *l;
-
- /* deconstruct the name list */
- DeconstructQualifiedName(collname, &schemaname, &collation_name);
-
- if (schemaname)
- {
- /* use exact schema given */
- namespaceId = LookupExplicitNamespace(schemaname, missing_ok);
- if (missing_ok && !OidIsValid(namespaceId))
- return InvalidOid;
-
- colloid = lookup_collation(collation_name, namespaceId, dbencoding);
- if (OidIsValid(colloid))
- return colloid;
- }
- else
- {
- /* search for it in search path */
- recomputeNamespacePath();
-
- foreach(l, activeSearchPath)
- {
- namespaceId = lfirst_oid(l);
-
- if (namespaceId == myTempNamespace)
- continue; /* do not look in temp namespace */
-
- colloid = lookup_collation(collation_name, namespaceId, dbencoding);
- if (OidIsValid(colloid))
- return colloid;
- }
- }
-
- /* Not found in path */
- if (!missing_ok)
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("collation \"%s\" for encoding \"%s\" does not exist",
- NameListToString(collname), GetDatabaseEncodingName())));
- return InvalidOid;
-}
-
/*
* get_conversion_oid - find a conversion by possibly qualified name
*/
@@ -3790,10 +3649,6 @@ recomputeNamespacePath(void)
bool pathChanged;
MemoryContext oldcxt;
- /* Do nothing if an override search spec is active. */
- if (overrideStack)
- return;
-
/* Do nothing if path is already valid. */
if (baseSearchPathValid && namespaceUser == roleid)
return;
@@ -3932,10 +3787,7 @@ recomputeNamespacePath(void)
/*
* Bump the generation only if something actually changed. (Notice that
- * what we compared to was the old state of the base path variables; so
- * this does not deal with the situation where we have just popped an
- * override path and restored the prior state of the base path. Instead
- * we rely on the override-popping logic to have bumped the generation.)
+ * what we compared to was the old state of the base path variables.)
*/
if (pathChanged)
activePathGeneration++;
@@ -4138,29 +3990,6 @@ AtEOXact_Namespace(bool isCommit, bool parallel)
myTempNamespaceSubID = InvalidSubTransactionId;
}
- /*
- * Clean up if someone failed to do PopOverrideSearchPath
- */
- if (overrideStack)
- {
- if (isCommit)
- elog(WARNING, "leaked override search path");
- while (overrideStack)
- {
- OverrideStackEntry *entry;
-
- entry = (OverrideStackEntry *) linitial(overrideStack);
- overrideStack = list_delete_first(overrideStack);
- list_free(entry->searchPath);
- pfree(entry);
- }
- /* If not baseSearchPathValid, this is useless but harmless */
- activeSearchPath = baseSearchPath;
- activeCreationNamespace = baseCreationNamespace;
- activeTempCreationPending = baseTempCreationPending;
- /* Always bump generation --- see note in recomputeNamespacePath */
- activePathGeneration++;
- }
}
/*
@@ -4175,7 +4004,6 @@ void
AtEOSubXact_Namespace(bool isCommit, SubTransactionId mySubid,
SubTransactionId parentSubid)
{
- OverrideStackEntry *entry;
if (myTempNamespaceSubID == mySubid)
{
@@ -4201,51 +4029,6 @@ AtEOSubXact_Namespace(bool isCommit, SubTransactionId mySubid,
MyProc->tempNamespaceId = InvalidOid;
}
}
-
- /*
- * Clean up if someone failed to do PopOverrideSearchPath
- */
- while (overrideStack)
- {
- entry = (OverrideStackEntry *) linitial(overrideStack);
- if (entry->nestLevel < GetCurrentTransactionNestLevel())
- break;
- if (isCommit)
- elog(WARNING, "leaked override search path");
- overrideStack = list_delete_first(overrideStack);
- list_free(entry->searchPath);
- pfree(entry);
- /* Always bump generation --- see note in recomputeNamespacePath */
- activePathGeneration++;
- }
-
- /* Activate the next level down. */
- if (overrideStack)
- {
- entry = (OverrideStackEntry *) linitial(overrideStack);
- activeSearchPath = entry->searchPath;
- activeCreationNamespace = entry->creationNamespace;
- activeTempCreationPending = false; /* XXX is this OK? */
-
- /*
- * It's probably unnecessary to bump generation here, but this should
- * not be a performance-critical case, so better to be over-cautious.
- */
- activePathGeneration++;
- }
- else
- {
- /* If not baseSearchPathValid, this is useless but harmless */
- activeSearchPath = baseSearchPath;
- activeCreationNamespace = baseCreationNamespace;
- activeTempCreationPending = baseTempCreationPending;
-
- /*
- * If we popped an override stack entry, then we already bumped the
- * generation above. If we did not, then the above assignments did
- * nothing and we need not bump the generation.
- */
- }
}
/*
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index b1509cc505..15cc1ff99e 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -947,11 +947,6 @@ execute_extension_script(Oid extensionOid, ExtensionControlFile *control,
* searched anyway. (Listing pg_catalog explicitly in a non-first
* position would be bad for security.) Finally add pg_temp to ensure
* that temp objects can't take precedence over others.
- *
- * Note: it might look tempting to use PushOverrideSearchPath for this,
- * but we cannot do that. We have to actually set the search_path GUC in
- * case the extension script examines or changes it. In any case, the
- * GUC_ACTION_SAVE method is just as convenient.
*/
initStringInfo(&pathbuf);
appendStringInfoString(&pathbuf, quote_identifier(schemaName));
diff --git a/src/include/catalog/namespace.h b/src/include/catalog/namespace.h
index f64a0ec26b..93e0c12345 100644
--- a/src/include/catalog/namespace.h
+++ b/src/include/catalog/namespace.h
@@ -167,8 +167,6 @@ extern void ResetTempTableNamespace(void);
extern OverrideSearchPath *GetOverrideSearchPath(MemoryContext context);
extern OverrideSearchPath *CopyOverrideSearchPath(OverrideSearchPath *path);
extern bool OverrideSearchPathMatchesCurrent(OverrideSearchPath *path);
-extern void PushOverrideSearchPath(OverrideSearchPath *newpath);
-extern void PopOverrideSearchPath(void);
extern Oid get_collation_oid(List *collname, bool missing_ok);
extern Oid get_conversion_oid(List *conname, bool missing_ok);
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 45fc5759ce..19fb4f8303 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1666,7 +1666,6 @@ OutputPluginCallbacks
OutputPluginOptions
OutputPluginOutputType
OverrideSearchPath
-OverrideStackEntry
OverridingKind
PACE_HEADER
PACL
