Modify APIs to return palloc'ed values and catalog lookup to validate…

… the user in get_physical_user_name (#2960) Earlier in Babelfish, The get_*_role_name() and get_*_schema_name() APIs returned constant values or string literal for few cases and palloc'ed copy for other cases. The get_physical_user_name() used to return the user_name after appending the db_name to the user_name without any checks to verify its existence/validity. With this change, We return Palloc'ed values for the API every time. In get_physical_user_name() if the user or role is not found in the sys.babelfish_authid_user_ext catalog, then an error is thrown. Additionally, In multidb.c, made changes to the APIs like get_role_name() and get_schema_name() to return palloc'ed values instead of constant values. Implemented a catalog lookup in get_physical_user_name() to verify if the user or role is found in the sys.babelfish_authid_user_ext catalog. The 'suppress_role_error' flag indicates if it is ok for the user or role to be absent from the catalog (required for cases when a new user/role is created and it can't be found in the catalog). Checked all the functions where the APIs are called and pfree the variables containing values from the above APIs after they are no longer used. Task: BABEL-4933 Signed-off-by: P Aswini Kumar <[email protected]>
babelfish-for-postgresql · Sep 30, 2024 · 1550114 · 1550114
1 parent 2b0b6d5
commit 1550114
Show file tree

Hide file tree

Showing 14 changed files with 324 additions and 205 deletions.
diff --git a/contrib/babelfishpg_tsql/runtime/functions.c b/contrib/babelfishpg_tsql/runtime/functions.c
@@ -1303,11 +1303,13 @@ schema_id(PG_FUNCTION_ARGS)
 	{
 		char	   *db_name = get_cur_db_name();
 		const char *user = get_user_for_database(db_name);
-		const char *guest_role_name = get_guest_role_name(db_name);
+		char 	   *guest_role_name = get_guest_role_name(db_name);
 
 		if (!user)
 		{
 			pfree(db_name);
+			pfree(guest_role_name);
+
 			PG_RETURN_NULL();
 		}
 		else if ((guest_role_name && strcmp(user, guest_role_name) == 0))
@@ -1320,6 +1322,7 @@ schema_id(PG_FUNCTION_ARGS)
 			physical_name = get_physical_schema_name(db_name, name);
 		}
 		pfree(db_name);
+		pfree(guest_role_name);
 	}
 	else
 	{
@@ -2224,15 +2227,18 @@ object_id(PG_FUNCTION_ARGS)
 		 * name
 		 */
 		const char *user = get_user_for_database(db_name);
-		const char *guest_role_name = get_guest_role_name(db_name);
+		char 	   *guest_role_name = get_guest_role_name(db_name);
 
 		if (!user)
 		{
 			pfree(db_name);
 			pfree(schema_name);
 			pfree(object_name);
+			pfree(guest_role_name);
+
 			if (object_type)
 				pfree(object_type);
+
 			PG_RETURN_NULL();
 		}
 		else if ((guest_role_name && strcmp(user, guest_role_name) == 0))
@@ -2245,6 +2251,8 @@ object_id(PG_FUNCTION_ARGS)
 			schema_name = get_authid_user_ext_schema_name((const char *) db_name, user);
 			physical_schema_name = get_physical_schema_name(db_name, schema_name);
 		}
+
+		pfree(guest_role_name);
 	}
 	else
 	{
@@ -2677,14 +2685,16 @@ type_id(PG_FUNCTION_ARGS)
         if (!OidIsValid(result))
         {
             /* find the default schema for current user and get physical schema name */
-            const char *user = get_user_for_database(db_name);
-            const char *guest_role_name = get_guest_role_name(db_name);
+            const char  *user = get_user_for_database(db_name);
+            char        *guest_role_name = get_guest_role_name(db_name);
 
             if (!user)
             {
                 pfree(db_name);
                 pfree(schema_name);
                 pfree(object_name);
+                pfree(guest_role_name);
+
                 PG_RETURN_NULL();
             }
             else if ((guest_role_name && strcmp(user, guest_role_name) == 0))
@@ -2697,6 +2707,8 @@ type_id(PG_FUNCTION_ARGS)
                 schema_name = get_authid_user_ext_schema_name((const char *) db_name, user);
                 physical_schema_name = get_physical_schema_name(db_name, schema_name);
             }
+
+            pfree(guest_role_name);
         }
         else
         {
@@ -2826,20 +2838,20 @@ replace_special_chars_fts(PG_FUNCTION_ARGS)
 Datum
 has_dbaccess(PG_FUNCTION_ARGS)
 {
-	char	   *db_name = text_to_cstring(PG_GETARG_TEXT_P(0));
+	char        *db_name = text_to_cstring(PG_GETARG_TEXT_P(0));
 
 	/*
 	 * Ensure the database name input argument is lower-case, as all Babel
 	 * table names are lower-case
 	 */
-	char	   *lowercase_db_name = lowerstr(db_name);
+	char        *lowercase_db_name = lowerstr(db_name);
 
 	/* Also strip trailing whitespace to mimic SQL Server behaviour */
-	int			i;
-	const char *user = NULL;
-	const char *login;
-	int16		db_id;
-	bool		login_is_db_owner;
+	int         i;
+	char        *user = NULL;
+	const char  *login;
+	int16       db_id;
+	bool        login_is_db_owner;
 
 	i = strlen(lowercase_db_name);
 	while (i > 0 && isspace((unsigned char) lowercase_db_name[i - 1]))
@@ -2885,7 +2897,10 @@ has_dbaccess(PG_FUNCTION_ARGS)
 	if (!user)
 		PG_RETURN_INT32(0);
 	else
+	{
+		pfree(user);
 		PG_RETURN_INT32(1);
+	}
 }
 
 Datum

diff --git a/contrib/babelfishpg_tsql/src/catalog.c b/contrib/babelfishpg_tsql/src/catalog.c
@@ -3041,7 +3041,7 @@ update_user_catalog_for_guest(PG_FUNCTION_ARGS)
 bool
 guest_role_exists_for_db(const char *dbname)
 {
-	const char *guest_role = get_guest_role_name(dbname);
+	char		*guest_role = get_guest_role_name(dbname);
 	bool		role_exists = false;
 	HeapTuple	tuple;
 
@@ -3053,6 +3053,8 @@ guest_role_exists_for_db(const char *dbname)
 		ReleaseSysCache(tuple);
 	}
 
+	pfree(guest_role);
+
 	return role_exists;
 }
 
@@ -3109,7 +3111,7 @@ get_login_for_user(Oid user_id, const char *physical_schema_name)
 static void
 create_guest_role_for_db(const char *dbname)
 {
-	const char *guest = get_guest_role_name(dbname);
+	char	   *guest = get_guest_role_name(dbname);
 	const char *db_owner_role = get_db_owner_role_name(dbname);
 	List	   *logins = NIL;
 	List	   *res;
@@ -3195,6 +3197,8 @@ create_guest_role_for_db(const char *dbname)
 		set_cur_db(old_dbid, old_dbname);
 	}
 	PG_END_TRY();
+
+	pfree(guest);
 }
 
 /*
@@ -4659,7 +4663,7 @@ update_babelfish_authid_user_ext_rename_db(
 							Anum_bbf_authid_user_ext_orig_username, bbf_authid_user_ext_dsc, &isNull));
 		NameData rolename_namedata;
 
-		namestrcpy(&rolename_namedata, get_physical_user_name((char *)new_db_name, role_name, true));
+		namestrcpy(&rolename_namedata, get_physical_user_name((char *)new_db_name, role_name, true, true));
 		list_of_roles_to_rename = lappend(list_of_roles_to_rename, pstrdup(role_name));
 
 		/* update rolname */
@@ -4938,9 +4942,12 @@ rename_tsql_db(char *old_db_name, char *new_db_name)
 				(strlen(role) == 8 && strncmp(role, "db_owner", 8) == 0)))
 				continue;
 
-			old_role_name = get_physical_user_name(old_db_name, role, true);
-			new_role_name = get_physical_user_name(new_db_name, role, true);
+			old_role_name = get_physical_user_name(old_db_name, role, true, true);
+			new_role_name = get_physical_user_name(new_db_name, role, true, true);
 			exec_rename_db_util(old_role_name, new_role_name, false);
+
+			pfree(old_role_name);
+			pfree(new_role_name);
 		}
 
 		/* Update the default_database field in babelfish_authid_login_ext. */
@@ -4977,6 +4984,38 @@ rename_tsql_db(char *old_db_name, char *new_db_name)
 		CommitTransactionCommand();
 }
 
+/*
+ * Returns true if the user/role exists in the sys.babelfish_authid_user_ext catalog,
+ * false otherwise.
+ */
+bool
+user_exists_for_db(const char *db_name, const char *user_name)
+{
+	HeapTuple		tuple_cache;
+	NameData		rolname;
+	bool			user_exists = false;
+
+	namestrcpy(&rolname, user_name);
+
+	tuple_cache = SearchSysCache1(AUTHIDUSEREXTROLENAME, NameGetDatum(&rolname));
+
+	if (HeapTupleIsValid(tuple_cache))
+	{
+		bool isnull;
+		char *db_name_from_cache = TextDatumGetCString(SysCacheGetAttr(AUTHIDUSEREXTROLENAME, tuple_cache,
+												 Anum_bbf_authid_user_ext_database_name, &isnull));
+
+		Assert(!isnull);
+
+		if (strcmp(db_name_from_cache, db_name) == 0)
+			user_exists = true;
+
+		pfree(db_name_from_cache);
+	}
+
+	return user_exists;
+}
+
 /*
  * partition_function_id_exists
  *		Returns true if provided function id is in use, false otherwise.

diff --git a/contrib/babelfishpg_tsql/src/catalog.h b/contrib/babelfishpg_tsql/src/catalog.h
@@ -162,6 +162,7 @@ extern List *update_babelfish_namespace_ext_nsp_name(int16 db_id, char *new_db_n
 extern List *update_babelfish_authid_user_ext_db_name(const char *old_db_name, const char *new_db_name);
 extern void rename_tsql_db(char *old_db_name, char *new_db_name);
 extern Oid get_login_for_user(Oid user_id, const char *physical_schema_name);
+extern bool user_exists_for_db(const char *db_name, const char *user_name);
 
 /* MUST comply with babelfish_authid_user_ext table */
 typedef struct FormData_authid_user_ext

diff --git a/contrib/babelfishpg_tsql/src/dbcmds.c b/contrib/babelfishpg_tsql/src/dbcmds.c
@@ -450,25 +450,25 @@ check_database_collation_name(const char *database_collation_name)
 static void
 create_bbf_db_internal(ParseState *pstate, const char *dbname, List *options, const char *owner, int16 dbid)
 {
-	int16		old_dbid;
-	char	   *old_dbname;
-	Oid			datdba;
-	Datum	   *new_record;
-	bool	   *new_record_nulls;
-	Relation	sysdatabase_rel;
-	HeapTuple	tuple;
-	List	   *parsetree_list;
-	ListCell   *parsetree_item;
-	const char *dbo_role;
-	NameData	default_collation;
-	NameData	owner_namedata;
-	int			stmt_number = 0;
-	int 			save_sec_context;
-	bool 			is_set_userid = false;
-	Oid 			save_userid;
-	const char	*old_createrole_self_grant;
-	ListCell	*option;
-	const char *database_collation_name = NULL;
+	int16       old_dbid;
+	char        *old_dbname;
+	Oid         datdba;
+	Datum       *new_record;
+	bool        *new_record_nulls;
+	Relation    sysdatabase_rel;
+	HeapTuple   tuple;
+	List        *parsetree_list;
+	ListCell    *parsetree_item;
+	char        *dbo_role;
+	NameData    default_collation;
+	NameData    owner_namedata;
+	int         stmt_number = 0;
+	int         save_sec_context;
+	bool        is_set_userid = false;
+	Oid         save_userid;
+	const char  *old_createrole_self_grant;
+	ListCell    *option;
+	const char  *database_collation_name = NULL;
 
 	/* Check options */
 	foreach(option, options)
@@ -636,23 +636,25 @@ create_bbf_db_internal(ParseState *pstate, const char *dbname, List *options, co
 		set_cur_db(old_dbid, old_dbname);
 	}
 	PG_END_TRY();
+
+	pfree(dbo_role);
 }
 
 void
 drop_bbf_db(const char *dbname, bool missing_ok, bool force_drop)
 {
-	volatile Relation sysdatabase_rel;
-	HeapTuple	tuple;
-	Form_sysdatabases bbf_db;
-	int16		dbid;
-	const char *dbo_role;
-	List	   *db_users_list;
-	List	   *parsetree_list;
-	ListCell   *parsetree_item;
-	const char *prev_current_user;
-	int 		save_sec_context;
-	bool 		is_set_userid = false;
-	Oid 		save_userid;
+	volatile Relation   sysdatabase_rel;
+	HeapTuple           tuple;
+	Form_sysdatabases   bbf_db;
+	int16               dbid;
+	char               *dbo_role;
+	List               *db_users_list;
+	List               *parsetree_list;
+	ListCell           *parsetree_item;
+	const char         *prev_current_user;
+	int                save_sec_context;
+	bool               is_set_userid = false;
+	Oid                save_userid;
 
 	if ((strlen(dbname) == 6 && (strncmp(dbname, "master", 6) == 0)) ||
 		((strlen(dbname) == 6 && strncmp(dbname, "tempdb", 6) == 0)) ||
@@ -808,6 +810,8 @@ drop_bbf_db(const char *dbname, bool missing_ok, bool force_drop)
 	}
 	PG_END_TRY();
 
+	pfree(dbo_role);
+
 	/* Set current user back to previous user */
 	bbf_set_current_user(prev_current_user);
 }
@@ -1064,8 +1068,8 @@ create_schema_if_not_exists(const uint16 dbid,
 	const char *prev_current_user;
 	uint16		old_dbid;
 	const char *old_dbname,
-			   *phys_schema_name,
-			   *phys_role;
+			   *phys_schema_name;
+	char	   *phys_role;
 
 	/*
 	 * During upgrade, the migration mode is reset to single-db so we cannot
@@ -1089,7 +1093,7 @@ create_schema_if_not_exists(const uint16 dbid,
 	 * some reason guest role does not exist, then that is a bigger problem.
 	 * We skip creating the guest schema entirely instead of crashing though.
 	 */
-	phys_role = get_physical_user_name((char *) dbname, (char *) owner_role, false);
+	phys_role = get_physical_user_name((char *) dbname, (char *) owner_role, false, true);
 	if (!OidIsValid(get_role_oid(phys_role, true)))
 	{
 		ereport(LOG,
@@ -1148,6 +1152,8 @@ create_schema_if_not_exists(const uint16 dbid,
 	}
 	PG_END_TRY();
 
+	pfree(phys_role);
+
 	bbf_set_current_user(prev_current_user);
 	set_cur_db(old_dbid, old_dbname);
 

diff --git a/contrib/babelfishpg_tsql/src/hooks.c b/contrib/babelfishpg_tsql/src/hooks.c
@@ -5022,8 +5022,8 @@ get_local_schema_for_bbf_functions(Oid proc_nsp_oid)
 	HeapTuple 	 	tuple;
 	char 			*func_schema_name = NULL,
 					*new_search_path = NULL;
-	const char  	*func_dbo_schema,
-					*cur_dbname = get_cur_db_name();
+	char  			*func_dbo_schema;
+	const char		*cur_dbname = get_cur_db_name();
 
 	tuple = SearchSysCache1(NAMESPACEOID,
 						ObjectIdGetDatum(proc_nsp_oid));
@@ -5039,7 +5039,10 @@ get_local_schema_for_bbf_functions(Oid proc_nsp_oid)
 										quote_identifier(func_dbo_schema));
 
 		ReleaseSysCache(tuple);
+
+		pfree(func_dbo_schema);
 	}
+
 	return new_search_path;
 }