Thanks for your reviewing.
The attached patch is a revised version.
I don't have any objections to your comments.
(2010/12/07 4:38), Robert Haas wrote:
> 2010/11/25 KaiGai Kohei<[email protected]>:
>> The attached patch is a revised one.
>>
>> It provides two hooks; the one informs core PG whether the supplied
>> function needs to be hooked, or not. the other is an actual hook on
>> prepare, start, end and abort of function invocations.
>>
>> typedef bool (*needs_function_call_type)(Oid fn_oid);
>>
>> typedef void (*function_call_type)(FunctionCallEventType event,
>> FmgrInfo *flinfo, Datum *private);
>>
>> The hook prototype was a bit modified since the suggestion from
>> Robert. Because FmgrInfo structure contain OID of the function,
>> it might be redundant to deliver OID of the function individually.
>>
>> Rest of parts are revised according to the comment.
>>
>> I also fixed up source code comments which might become incorrect.
>
> FCET_PREPARE looks completely unnecessary to me. Any necessary
> one-time work can easily be done at FCET_START time, assuming that the
> private-data field is initialized to (Datum) 0.
>
It seems to me a reasonable assumption.
I revised the code, and modified source code comments to ensure
the private shall be initialized to (Datum) 0.
> I'm fairly certain that the following is not portable:
>
> + ObjectAddress object = { .classId = ProcedureRelationId,
> + .objectId = fn_oid,
> + .objectSubId = 0 };
>
Fixed.
> I'd suggest renaming needs_function_call_type and function_call_type
> to needs_fmgr_hook_type and fmgr_hook_type.
>
I also think the suggested names are better than before.
According to the renaming, FunctionCallEventType was also renamed to
FmgrHookEventType. Perhaps, it is a reasonable change.
Thanks,
--
KaiGai Kohei <[email protected]>
contrib/dummy_seclabel/dummy_seclabel.c | 107 ++++++++++++++++++++++++-
src/backend/optimizer/util/clauses.c | 8 ++
src/backend/utils/fmgr/fmgr.c | 35 ++++++---
src/include/fmgr.h | 46 +++++++++++
src/test/regress/input/security_label.source | 28 +++++++
src/test/regress/output/security_label.source | 43 ++++++++++-
6 files changed, 255 insertions(+), 12 deletions(-)
diff --git a/contrib/dummy_seclabel/dummy_seclabel.c b/contrib/dummy_seclabel/dummy_seclabel.c
index 8bd50a3..237419a 100644
--- a/contrib/dummy_seclabel/dummy_seclabel.c
+++ b/contrib/dummy_seclabel/dummy_seclabel.c
@@ -12,14 +12,110 @@
*/
#include "postgres.h"
+#include "catalog/pg_proc.h"
#include "commands/seclabel.h"
#include "miscadmin.h"
PG_MODULE_MAGIC;
+PG_FUNCTION_INFO_V1(dummy_client_label);
+
/* Entrypoint of the module */
void _PG_init(void);
+/* SQL functions */
+Datum dummay_client_label(PG_FUNCTION_ARGS);
+
+static const char *client_label = "unclassified";
+
+typedef struct {
+ const char *old_label;
+ const char *new_label;
+ Datum next_private;
+} dummy_stack;
+
+static needs_fmgr_hook_type needs_fmgr_next = NULL;
+static fmgr_hook_type fmgr_next = NULL;
+
+static bool
+dummy_needs_fmgr_hook(Oid fn_oid)
+{
+ char *label;
+ bool result = false;
+ ObjectAddress object;
+
+ if (needs_fmgr_next && (*needs_fmgr_next)(fn_oid))
+ return true;
+
+ object.classId = ProcedureRelationId;
+ object.objectId = fn_oid;
+ object.objectSubId = 0;
+ label = GetSecurityLabel(&object, "dummy");
+ if (label && strcmp(label, "trusted") == 0)
+ result = true;
+
+ if (label)
+ pfree(label);
+
+ return result;
+}
+
+static void
+dummy_fmgr_hook(FmgrHookEventType event,
+ FmgrInfo *flinfo, Datum *private)
+{
+ dummy_stack *stack;
+
+ switch (event)
+ {
+ case FHET_START:
+ /*
+ * In the first call, we allocate a pseudo stack for private
+ * datum of the secondary plugin module.
+ */
+ if (*private == 0)
+ {
+ stack = MemoryContextAlloc(flinfo->fn_mcxt,
+ sizeof(dummy_stack));
+ stack->old_label = NULL;
+ stack->new_label = (!superuser() ? "secret" : "top secret");
+ stack->next_private = 0;
+
+ if (fmgr_next)
+ (*fmgr_next)(event, flinfo, &stack->next_private);
+
+ *private = PointerGetDatum(stack);
+ }
+ else
+ stack = (dummy_stack *)DatumGetPointer(*private);
+
+ stack->old_label = client_label;
+ client_label = stack->new_label;
+ break;
+
+ case FHET_END:
+ case FHET_ABORT:
+ stack = (dummy_stack *)DatumGetPointer(*private);
+
+ client_label = stack->old_label;
+ stack->old_label = NULL;
+ break;
+
+ default:
+ elog(ERROR, "unexpected event type: %d", (int)event);
+ break;
+ }
+}
+
+Datum
+dummy_client_label(PG_FUNCTION_ARGS)
+{
+ if (!client_label)
+ PG_RETURN_NULL();
+
+ PG_RETURN_TEXT_P(cstring_to_text(client_label));
+}
+
static void
dummy_object_relabel(const ObjectAddress *object, const char *seclabel)
{
@@ -28,7 +124,8 @@ dummy_object_relabel(const ObjectAddress *object, const char *seclabel)
strcmp(seclabel, "classified") == 0)
return;
- if (strcmp(seclabel, "secret") == 0 ||
+ if (strcmp(seclabel, "trusted") == 0 ||
+ strcmp(seclabel, "secret") == 0 ||
strcmp(seclabel, "top secret") == 0)
{
if (!superuser())
@@ -46,4 +143,12 @@ void
_PG_init(void)
{
register_label_provider("dummy", dummy_object_relabel);
+
+ /* needs_function_call_hook */
+ needs_fmgr_next = needs_fmgr_hook;
+ needs_fmgr_hook = dummy_needs_fmgr_hook;
+
+ /* function_call_hook */
+ fmgr_next = fmgr_hook;
+ fmgr_hook = dummy_fmgr_hook;
}
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 80dfaad..8df5331 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -3726,6 +3726,10 @@ inline_function(Oid funcid, Oid result_type, List *args,
if (pg_proc_aclcheck(funcid, GetUserId(), ACL_EXECUTE) != ACLCHECK_OK)
return NULL;
+ /* Check whether the function shall be hooked by plugins */
+ if (FmgrHookIsNeeded(funcid))
+ return NULL;
+
/*
* Make a temporary memory context, so that we don't leak all the stuff
* that parsing might create.
@@ -4158,6 +4162,10 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
if (pg_proc_aclcheck(func_oid, GetUserId(), ACL_EXECUTE) != ACLCHECK_OK)
return NULL;
+ /* Check whether the function shall be hooked by plugins */
+ if (FmgrHookIsNeeded(func_oid))
+ return NULL;
+
/*
* OK, let's take a look at the function's pg_proc entry.
*/
diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index 1c9d2c2..ad459dd 100644
--- a/src/backend/utils/fmgr/fmgr.c
+++ b/src/backend/utils/fmgr/fmgr.c
@@ -30,6 +30,11 @@
#include "utils/lsyscache.h"
#include "utils/syscache.h"
+/*
+ * Hooks for function calls
+ */
+PGDLLIMPORT needs_fmgr_hook_type needs_fmgr_hook = NULL;
+PGDLLIMPORT fmgr_hook_type fmgr_hook = NULL;
/*
* Declaration for old-style function pointer type. This is now used only
@@ -216,7 +221,7 @@ fmgr_info_cxt_security(Oid functionId, FmgrInfo *finfo, MemoryContext mcxt,
finfo->fn_retset = procedureStruct->proretset;
/*
- * If it has prosecdef set, or non-null proconfig, use
+ * If it has prosecdef set, non-null proconfig, hook by plugins use
* fmgr_security_definer call handler --- unless we are being called again
* by fmgr_security_definer.
*
@@ -230,7 +235,8 @@ fmgr_info_cxt_security(Oid functionId, FmgrInfo *finfo, MemoryContext mcxt,
*/
if (!ignore_security &&
(procedureStruct->prosecdef ||
- !heap_attisnull(procedureTuple, Anum_pg_proc_proconfig)))
+ !heap_attisnull(procedureTuple, Anum_pg_proc_proconfig) ||
+ FmgrHookIsNeeded(functionId)))
{
finfo->fn_addr = fmgr_security_definer;
finfo->fn_stats = TRACK_FUNC_ALL; /* ie, never track */
@@ -857,17 +863,18 @@ struct fmgr_security_definer_cache
FmgrInfo flinfo; /* lookup info for target function */
Oid userid; /* userid to set, or InvalidOid */
ArrayType *proconfig; /* GUC values to set, or NULL */
+ Datum private; /* private usage for plugin modules */
};
/*
- * Function handler for security-definer/proconfig functions. We extract the
- * OID of the actual function and do a fmgr lookup again. Then we fetch the
- * pg_proc row and copy the owner ID and proconfig fields. (All this info
- * is cached for the duration of the current query.) To execute a call,
- * we temporarily replace the flinfo with the cached/looked-up one, while
- * keeping the outer fcinfo (which contains all the actual arguments, etc.)
- * intact. This is not re-entrant, but then the fcinfo itself can't be used
- * re-entrantly anyway.
+ * Function handler for security-definer/proconfig/plugin-hooked functions.
+ * We extract the OID of the actual function and do a fmgr lookup again.
+ * Then we fetch the pg_proc row and copy the owner ID and proconfig fields.
+ * (All this info is cached for the duration of the current query.)
+ * To execute a call, we temporarily replace the flinfo with the cached
+ * and looked-up one, while keeping the outer fcinfo (which contains all
+ * the actual arguments, etc.) intact. This is not re-entrant, but then
+ * the fcinfo itself can't be used re-entrantly anyway.
*/
static Datum
fmgr_security_definer(PG_FUNCTION_ARGS)
@@ -940,6 +947,10 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
GUC_ACTION_SAVE);
}
+ /* function manager hook */
+ if (fmgr_hook)
+ (*fmgr_hook)(FHET_START, &fcache->flinfo, &fcache->private);
+
/*
* We don't need to restore GUC or userid settings on error, because the
* ensuing xact or subxact abort will do that. The PG_TRY block is only
@@ -968,6 +979,8 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
PG_CATCH();
{
fcinfo->flinfo = save_flinfo;
+ if (fmgr_hook)
+ (*fmgr_hook)(FHET_ABORT, &fcache->flinfo, &fcache->private);
PG_RE_THROW();
}
PG_END_TRY();
@@ -978,6 +991,8 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
AtEOXact_GUC(true, save_nestlevel);
if (OidIsValid(fcache->userid))
SetUserIdAndSecContext(save_userid, save_sec_context);
+ if (fmgr_hook)
+ (*fmgr_hook)(FHET_END, &fcache->flinfo, &fcache->private);
return result;
}
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index ca5a5ea..e42ff90 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -544,6 +544,52 @@ extern void **find_rendezvous_variable(const char *varName);
extern int AggCheckCallContext(FunctionCallInfo fcinfo,
MemoryContext *aggcontext);
+/*
+ * fmgr_hook
+ * ----------
+ * This hook allows plugin modules to hook events of function calls.
+ * It enables to switch some of its internal state (such as privilege
+ * of the client) during execution of the hooked function.
+ *
+ * The plugin module has to set up two hooks for this feature at least.
+ * The one is 'needs_fmgr_hook' which enables to inform whether the plugin
+ * module wants to hook the supplied function, or not.
+ * It returns 'true', if the supplied function shall be hooked. Elsewhere,
+ * 'false' shall be returned.
+ *
+ * The other hook is 'fmgr_hook' that enables plugin modules to acquire
+ * control when the supplied function is started, ended and aborted.
+ * It takes three arguments: event type (defined as FmgrHookEventType),
+ * FmgrInfo and pointer to the private data field.
+ *
+ * FHET_START event type allows plugins to acquire control just before
+ * invocation of the hooked function for each time. In the first call,
+ * the private data shall be initialized to (Datum)0. If the plugin
+ * module wants palloc(), it should use FmgrInfo->mcxt instead of the
+ * CurrentMemoryContext in the first call, because it is not available
+ * on the later invocations.
+ *
+ * FHET_END and FHET_ABORT allow plugins to acquire control just after
+ * invocation of the hooked function for each time. If necessary, the
+ * plugin module can restore its internal state.
+ */
+typedef enum FmgrHookEventType
+{
+ FHET_START,
+ FHET_END,
+ FHET_ABORT
+} FmgrHookEventType;
+
+typedef bool (*needs_fmgr_hook_type)(Oid fn_oid);
+
+typedef void (*fmgr_hook_type)(FmgrHookEventType event,
+ FmgrInfo *flinfo, Datum *private);
+
+extern PGDLLIMPORT needs_fmgr_hook_type needs_fmgr_hook;
+extern PGDLLIMPORT fmgr_hook_type fmgr_hook;
+
+#define FmgrHookIsNeeded(fn_oid) \
+ (!needs_fmgr_hook ? false : (*needs_fmgr_hook)(fn_oid))
/*
* !!! OLD INTERFACE !!!
diff --git a/src/test/regress/input/security_label.source b/src/test/regress/input/security_label.source
index 810a721..9fb2a2d 100644
--- a/src/test/regress/input/security_label.source
+++ b/src/test/regress/input/security_label.source
@@ -37,6 +37,15 @@ SECURITY LABEL ON TABLE seclabel_tbl3 IS 'unclassified'; -- fail
-- Load dummy external security provider
LOAD '@libdir@/dummy_secla...@dlsuffix@';
+CREATE FUNCTION dummy_client_label() RETURNS text LANGUAGE 'c'
+ AS '@libdir@/dummy_secla...@dlsuffix@', 'dummy_client_label';
+
+CREATE FUNCTION seclabel_regular() RETURNS text LANGUAGE 'sql'
+ AS 'SELECT dummy_client_label()';
+
+CREATE FUNCTION seclabel_trusted() RETURNS text LANGUAGE 'sql'
+ AS 'SELECT dummy_client_label()';
+
--
-- Test of SECURITY LABEL statement with a plugin
--
@@ -70,7 +79,26 @@ SELECT objtype, objname, provider, label FROM pg_seclabels
SECURITY LABEL ON LANGUAGE plpgsql IS NULL; -- OK
SECURITY LABEL ON SCHEMA public IS NULL; -- OK
+-- test for trusted procedures
+SECURITY LABEL ON FUNCTION seclabel_regular() IS 'unclassified'; -- OK
+SECURITY LABEL ON FUNCTION seclabel_trusted() IS 'trusted'; -- OK
+
+-- should be 'unclassified' and 'top secret'
+SELECT seclabel_regular(), seclabel_trusted();
+
+-- should be 'unclassified' and 'secret'
+SET SESSION AUTHORIZATION seclabel_user1;
+SELECT seclabel_regular(), seclabel_trusted();
+RESET SESSION AUTHORIZATION;
+
+-- hooked functions shall not be inlined
+EXPLAIN SELECT * FROM seclabel_regular(); -- shall be inlined
+EXPLAIN SELECT * FROM seclabel_trusted(); -- shall not be inlined
+
-- clean up objects
+RESET SESSION AUTHORIZATION;
+DROP FUNCTION seclabel_regular();
+DROP FUNCTION seclabel_trusted();
DROP FUNCTION seclabel_four();
DROP DOMAIN seclabel_domain;
DROP VIEW seclabel_view1;
diff --git a/src/test/regress/output/security_label.source b/src/test/regress/output/security_label.source
index 4bc803d..fb3809e 100644
--- a/src/test/regress/output/security_label.source
+++ b/src/test/regress/output/security_label.source
@@ -30,7 +30,13 @@ ERROR: no security label providers have been loaded
SECURITY LABEL ON TABLE seclabel_tbl3 IS 'unclassified'; -- fail
ERROR: no security label providers have been loaded
-- Load dummy external security provider
-LOAD '@abs_builddir@/dummy_secla...@dlsuffix@';
+LOAD '@libdir@/dummy_secla...@dlsuffix@';
+CREATE FUNCTION dummy_client_label() RETURNS text LANGUAGE 'c'
+ AS '@libdir@/dummy_secla...@dlsuffix@', 'dummy_client_label';
+CREATE FUNCTION seclabel_regular() RETURNS text LANGUAGE 'sql'
+ AS 'SELECT dummy_client_label()';
+CREATE FUNCTION seclabel_trusted() RETURNS text LANGUAGE 'sql'
+ AS 'SELECT dummy_client_label()';
--
-- Test of SECURITY LABEL statement with a plugin
--
@@ -75,7 +81,42 @@ SELECT objtype, objname, provider, label FROM pg_seclabels
SECURITY LABEL ON LANGUAGE plpgsql IS NULL; -- OK
SECURITY LABEL ON SCHEMA public IS NULL; -- OK
+-- test for trusted procedures
+SECURITY LABEL ON FUNCTION seclabel_regular() IS 'unclassified'; -- OK
+SECURITY LABEL ON FUNCTION seclabel_trusted() IS 'trusted'; -- OK
+-- should be 'unclassified' and 'top secret'
+SELECT seclabel_regular(), seclabel_trusted();
+ seclabel_regular | seclabel_trusted
+------------------+------------------
+ unclassified | top secret
+(1 row)
+
+-- should be 'unclassified' and 'secret'
+SET SESSION AUTHORIZATION seclabel_user1;
+SELECT seclabel_regular(), seclabel_trusted();
+ seclabel_regular | seclabel_trusted
+------------------+------------------
+ unclassified | secret
+(1 row)
+
+RESET SESSION AUTHORIZATION;
+-- hooked functions shall not be inlined
+EXPLAIN SELECT * FROM seclabel_regular(); -- shall be inlined
+ QUERY PLAN
+-----------------------------------------------------------------------------------------
+ Function Scan on dummy_client_label seclabel_regular (cost=0.00..0.01 rows=1 width=32)
+(1 row)
+
+EXPLAIN SELECT * FROM seclabel_trusted(); -- shall not be inlined
+ QUERY PLAN
+----------------------------------------------------------------------
+ Function Scan on seclabel_trusted (cost=0.25..0.26 rows=1 width=32)
+(1 row)
+
-- clean up objects
+RESET SESSION AUTHORIZATION;
+DROP FUNCTION seclabel_regular();
+DROP FUNCTION seclabel_trusted();
DROP FUNCTION seclabel_four();
DROP DOMAIN seclabel_domain;
DROP VIEW seclabel_view1;
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
