Re: Getting rid of OverrideSearhPath in namespace.c

2023-07-31 Thread Noah Misch
On Mon, Jul 17, 2023 at 05:11:46PM +0300, Aleksander Alekseev wrote:
> > 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.
> >
> > [...]
> >
> > What do you think?
> 
> +1 to remove dead code.
> 
> The proposed patch however removes get_collation_oid(), apparently by
> mistake. Other than that the patch looks fine and passes `make
> installcheck-world`.
> 
> I added an entry to the nearest CF [1].
> 
> > 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.
> 
> +1 as well. I added the corresponding 0002 patch.

Pushed both.  Thanks.




Re: Getting rid of OverrideSearhPath in namespace.c

2023-07-17 Thread Aleksander Alekseev
Hi,

> 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.
>
> [...]
>
> What do you think?

+1 to remove dead code.

The proposed patch however removes get_collation_oid(), apparently by
mistake. Other than that the patch looks fine and passes `make
installcheck-world`.

I added an entry to the nearest CF [1].

> 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.

+1 as well. I added the corresponding 0002 patch.

[1] https://commitfest.postgresql.org/44/4447/

-- 
Best regards,
Aleksander Alekseev


v2-0002-Rename-OverrideSearchPath-to-SearchPathMatcher.patch
Description: Binary data


v2-0001-Remove-unused-search_path-override-stack.patch
Description: Binary data


Getting rid of OverrideSearhPath in namespace.c

2023-07-16 Thread Alexander Lakhin

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,
Alexanderdiff --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