From 7c5c4e1c0396b0617a6f9b659dd7375fb0bfb9dc Mon Sep 17 00:00:00 2001 From: Noah Misch Date: Mon, 31 Jul 2023 17:04:47 -0700 Subject: [PATCH] Remove PushOverrideSearchPath() and PopOverrideSearchPath(). Since commit 681d9e4621aac0a9c71364b6f54f00f6d8c4337f, they have no in-tree calls. Any new calls would introduce security vulnerabilities like the one fixed in that commit. Alexander Lakhin, reviewed by Aleksander Alekseev. Discussion: https://postgr.es/m/8ffb4650-52c4-6a81-38fc-8f99be981130@gmail.com --- src/backend/catalog/namespace.c | 233 +------------------------------ src/backend/commands/extension.c | 5 - src/include/catalog/namespace.h | 2 - src/tools/pgindent/typedefs.list | 1 - 4 files changed, 6 insertions(+), 235 deletions(-) diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c index 69ab1b8e4b..f51a13aa56 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,132 +3493,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 * @@ -3794,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; @@ -3936,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++; @@ -4142,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++; - } } /* @@ -4179,7 +4004,6 @@ void AtEOSubXact_Namespace(bool isCommit, SubTransactionId mySubid, SubTransactionId parentSubid) { - OverrideStackEntry *entry; if (myTempNamespaceSubID == mySubid) { @@ -4205,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 9a2ee1c600..4cc994ca31 100644 --- a/src/backend/commands/extension.c +++ b/src/backend/commands/extension.c @@ -967,11 +967,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 ab97f1794a..dc296afb63 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -1687,7 +1687,6 @@ OutputPluginCallbacks OutputPluginOptions OutputPluginOutputType OverrideSearchPath -OverrideStackEntry OverridingKind PACE_HEADER PACL