Skip to content

Commit bfbb182

Browse files
steve-chavezsteve-chavez
committed
fix: evtrig detection under another extension fmgr_hook
Problem: Under the following conditions: - plpgsql_check is enabled (can be any other extension that uses the fmgr_hook). + This means that the `supautils_needs_fmgr_hook` returns true anyway. - After doing `create extension`, function owners are downgraded from superuser to non-super. Supautils event trigger skipping mechanism will wrongly intercept the downgraded functions and force a noop (internally it's the `version()` function) causing the extension to misbehave. The `pgmq` extension is used to test this failure. Solution: Ensure we only detect event trigger functions even on the presence of other extensions fmgr hooks.
1 parent f9fbfe4 commit bfbb182

File tree

8 files changed

+291
-24
lines changed

8 files changed

+291
-24
lines changed

‎Makefile‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ OBJS = $(patsubst $(SRC_DIR)/%.c, $(BUILD_DIR)/%.o, $(SRC))
2929
PG_VERSION = $(strip $(shell $(PG_CONFIG) --version | $(GREP) -oP '(?<=PostgreSQL )[0-9]+'))
3030
# 0 is true
3131
PG_EQ15 = $(shell test $(PG_VERSION) -eq 15; echo $$?)
32+
PG_NEQ15 = $(shell test $(PG_VERSION) -ne 15; echo $$?)
3233
PG_GE16 = $(shell test $(PG_VERSION) -ge 16; echo $$?)
3334
PG_GE14 = $(shell test $(PG_VERSION) -ge 14; echo $$?)
3435
SYSTEM = $(shell uname -s)
@@ -43,6 +44,10 @@ else
4344
TESTS := $(filter-out test/sql/lt16_%.sql, $(TESTS))
4445
endif
4546

47+
ifeq ($(PG_NEQ15), 0)
48+
TESTS := $(filter-out test/sql/eq15_%.sql, $(TESTS))
49+
endif
50+
4651
REGRESS = $(patsubst test/sql/%.sql,%,$(TESTS))
4752
REGRESS_OPTS = --use-existing --inputdir=test
4853

‎nix/pgmq.nix‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
{ lib, stdenv, fetchFromGitHub, postgresql }:
2+
3+
stdenv.mkDerivation rec {
4+
pname = "pgmq";
5+
version = "1.4.4";
6+
buildInputs = [ postgresql ];
7+
src = fetchFromGitHub {
8+
owner = "tembo-io";
9+
repo = pname;
10+
rev = "v${version}";
11+
hash = "sha256-z+8/BqIlHwlMnuIzMz6eylmYbSmhtsNt7TJf/CxbdVw=";
12+
};
13+
14+
buildPhase = ''
15+
cd pgmq-extension
16+
'';
17+
18+
installPhase = ''
19+
install -D sql/pgmq.sql "$out/pgmq--${version}.sql"
20+
install -D -t $out sql/*.sql
21+
install -D -t $out *.control
22+
'';
23+
24+
meta = with lib; {
25+
description = "A lightweight message queue. Like AWS SQS and RSMQ but on Postgres.";
26+
homepage = "https://github.com/tembo-io/pgmq";
27+
platforms = postgresql.meta.platforms;
28+
license = licenses.postgresql;
29+
};
30+
}

‎shell.nix‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,13 @@ mkShell {
1010
pgsqlcheck15 = callPackage ./nix/plpgsql-check.nix {
1111
postgresql = xpg.postgresql_15;
1212
};
13+
pgmq15 = callPackage ./nix/pgmq.nix {
14+
postgresql = xpg.postgresql_15;
15+
};
1316
in
1417
[
15-
(xpg.xpgWithExtensions { exts15 = [ pgsqlcheck15 ]; })
18+
(xpg.xpgWithExtensions {
19+
exts15 = [ pgsqlcheck15 pgmq15 ];
20+
})
1621
];
1722
}

‎src/event_triggers.c‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
#include "pg_prelude.h"
22
#include "event_triggers.h"
3+
#include "utils.h"
34

45
// this is the underlying function of `select version();`
56
extern Datum pgsql_version(PG_FUNCTION_ARGS);
@@ -44,3 +45,6 @@ Oid get_function_owner(func_owner_search search){
4445
return func_owner;
4546
}
4647

48+
bool is_event_trigger_function(Oid foid){
49+
return get_func_rettype(foid) == SUPAUTILS_EVENT_TRIGGER_OID;
50+
}

‎src/event_triggers.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,4 +18,6 @@ extern Oid get_function_owner(func_owner_search search);
1818

1919
extern void force_noop(FmgrInfo *finfo);
2020

21+
extern bool is_event_trigger_function(Oid foid);
22+
2123
#endif

‎src/supautils.c‎

Lines changed: 21 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
#include "extensions_parameter_overrides.h"
55
#include "policy_grants.h"
66
#include "privileged_extensions.h"
7-
#include "utils.h"
87
#include "event_triggers.h"
98

109
#define EREPORT_RESERVED_MEMBERSHIP(name) \
@@ -88,10 +87,7 @@ static bool supautils_needs_fmgr_hook(Oid functionId) {
8887
if (next_needs_fmgr_hook && (*next_needs_fmgr_hook) (functionId))
8988
return true;
9089

91-
if (get_func_rettype(functionId) == SUPAUTILS_EVENT_TRIGGER_OID)
92-
return true;
93-
else
94-
return false;
90+
return is_event_trigger_function(functionId);
9591
}
9692

9793
// This function will fire twice: once before execution of the database function (event=FHET_START)
@@ -100,25 +96,27 @@ static void supautils_fmgr_hook(FmgrHookEventType event, FmgrInfo *flinfo, Datum
10096
switch (event) {
10197
// we only need to change behavior before the function gets executed
10298
case FHET_START: {
103-
const char *current_role_name = GetUserNameFromId(GetUserId(), false);
104-
const bool role_is_super = superuser();
105-
const bool role_is_reserved = is_reserved_role(current_role_name, false);
106-
if (role_is_super || role_is_reserved) {
107-
Oid func_owner = get_function_owner((func_owner_search){ .as = FO_SEARCH_FINFO, .val.finfo = flinfo });
108-
bool function_is_owned_by_super = superuser_arg(func_owner);
109-
if (!function_is_owned_by_super){
110-
if (log_skipped_evtrigs){
111-
char *func_name = get_func_name(flinfo->fn_oid);
112-
ereport(
113-
NOTICE,
114-
errmsg("Skipping event trigger function \"%s\" for user \"%s\"", func_name, current_role_name),
115-
errdetail("\"%s\" %s and the function \"%s\" is not superuser-owned, it's owned by \"%s\"",
116-
current_role_name, role_is_super?"is a superuser":"is a reserved role", func_name, GetUserNameFromId(func_owner, false))
117-
);
99+
if (is_event_trigger_function(flinfo->fn_oid)){ // recheck the function is an event trigger in case another extension need_fmgr_hook passed our supautils_needs_fmgr_hook
100+
const char *current_role_name = GetUserNameFromId(GetUserId(), false);
101+
const bool role_is_super = superuser();
102+
const bool role_is_reserved = is_reserved_role(current_role_name, false);
103+
if (role_is_super || role_is_reserved) {
104+
Oid func_owner = get_function_owner((func_owner_search){ .as = FO_SEARCH_FINFO, .val.finfo = flinfo });
105+
bool function_is_owned_by_super = superuser_arg(func_owner);
106+
if (!function_is_owned_by_super){
107+
if (log_skipped_evtrigs){
108+
char *func_name = get_func_name(flinfo->fn_oid);
109+
ereport(
110+
NOTICE,
111+
errmsg("Skipping event trigger function \"%s\" for user \"%s\"", func_name, current_role_name),
112+
errdetail("\"%s\" %s and the function \"%s\" is not superuser-owned, it's owned by \"%s\"",
113+
current_role_name, role_is_super?"is a superuser":"is a reserved role", func_name, GetUserNameFromId(func_owner, false))
114+
);
115+
}
116+
// we can't skip execution directly inside the fmgr_hook (although we can abort it with ereport)
117+
// so instead we use the workaround of changing the event trigger function to a noop function
118+
force_noop(flinfo);
118119
}
119-
// we can't skip execution directly inside the fmgr_hook (although we can abort it with ereport)
120-
// so instead we use the workaround of changing the event trigger function to a noop function
121-
force_noop(flinfo);
122120
}
123121
}
124122

‎test/expected/eq15_pgmq.out‎

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
-- create the extension as usual
2+
create extension if not exists pgmq;
3+
set supautils.log_skipped_evtrigs = true;
4+
\echo
5+
6+
-- downgrade the extension functions owners to non-superuser, to ensure the following function calls are not wrongly skipped by the event trigger mechanism
7+
do $$
8+
declare
9+
extoid oid := (select oid from pg_extension where extname = 'pgmq');
10+
r record;
11+
cls pg_class%rowtype;
12+
begin
13+
for r in (select * from pg_depend where refobjid = extoid) loop
14+
if r.classid = 'pg_proc'::regclass then
15+
execute(format('alter function %s(%s) owner to privileged_role;', r.objid::regproc, pg_get_function_identity_arguments(r.objid)));
16+
end if;
17+
end loop;
18+
end $$;
19+
\echo
20+
21+
-- Test the standard flow
22+
select
23+
pgmq.create('Foo');
24+
create
25+
--------
26+
27+
(1 row)
28+
29+
select
30+
*
31+
from
32+
pgmq.send(
33+
queue_name:='Foo',
34+
msg:='{"foo": "bar1"}'
35+
);
36+
send
37+
------
38+
1
39+
(1 row)
40+
41+
-- Test queue is not case sensitive
42+
select
43+
*
44+
from
45+
pgmq.send(
46+
queue_name:='foo', -- note: lowercase useage
47+
msg:='{"foo": "bar2"}',
48+
delay:=5
49+
);
50+
send
51+
------
52+
2
53+
(1 row)
54+
55+
select
56+
msg_id,
57+
read_ct,
58+
message
59+
from
60+
pgmq.read(
61+
queue_name:='Foo',
62+
vt:=30,
63+
qty:=2
64+
);
65+
msg_id | read_ct | message
66+
--------+---------+-----------------
67+
1 | 1 | {"foo": "bar1"}
68+
(1 row)
69+
70+
select
71+
msg_id,
72+
read_ct,
73+
message
74+
from
75+
pgmq.pop('Foo');
76+
msg_id | read_ct | message
77+
--------+---------+---------
78+
(0 rows)
79+
80+
-- Archive message with msg_id=2.
81+
select
82+
pgmq.archive(
83+
queue_name:='Foo',
84+
msg_id:=2
85+
);
86+
archive
87+
---------
88+
t
89+
(1 row)
90+
91+
select
92+
pgmq.create('my_queue');
93+
create
94+
--------
95+
96+
(1 row)
97+
98+
select
99+
pgmq.send_batch(
100+
queue_name:='my_queue',
101+
msgs:=array['{"foo": "bar3"}','{"foo": "bar4"}','{"foo": "bar5"}']::jsonb[]
102+
);
103+
send_batch
104+
------------
105+
1
106+
2
107+
3
108+
(3 rows)
109+
110+
select
111+
pgmq.archive(
112+
queue_name:='my_queue',
113+
msg_ids:=array[3, 4, 5]
114+
);
115+
archive
116+
---------
117+
3
118+
(1 row)
119+
120+
select
121+
pgmq.delete('my_queue', 6);
122+
delete
123+
--------
124+
f
125+
(1 row)
126+
127+
select
128+
pgmq.drop_queue('my_queue');
129+
drop_queue
130+
------------
131+
t
132+
(1 row)
133+

‎test/sql/eq15_pgmq.sql‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
-- create the extension as usual
2+
create extension if not exists pgmq;
3+
set supautils.log_skipped_evtrigs = true;
4+
\echo
5+
6+
-- downgrade the extension functions owners to non-superuser, to ensure the following function calls are not wrongly skipped by the event trigger mechanism
7+
do $$
8+
declare
9+
extoid oid := (select oid from pg_extension where extname = 'pgmq');
10+
r record;
11+
cls pg_class%rowtype;
12+
begin
13+
for r in (select * from pg_depend where refobjid = extoid) loop
14+
if r.classid = 'pg_proc'::regclass then
15+
execute(format('alter function %s(%s) owner to privileged_role;', r.objid::regproc, pg_get_function_identity_arguments(r.objid)));
16+
end if;
17+
end loop;
18+
end $$;
19+
\echo
20+
21+
-- Test the standard flow
22+
select
23+
pgmq.create('Foo');
24+
25+
select
26+
*
27+
from
28+
pgmq.send(
29+
queue_name:='Foo',
30+
msg:='{"foo": "bar1"}'
31+
);
32+
33+
-- Test queue is not case sensitive
34+
select
35+
*
36+
from
37+
pgmq.send(
38+
queue_name:='foo', -- note: lowercase useage
39+
msg:='{"foo": "bar2"}',
40+
delay:=5
41+
);
42+
43+
select
44+
msg_id,
45+
read_ct,
46+
message
47+
from
48+
pgmq.read(
49+
queue_name:='Foo',
50+
vt:=30,
51+
qty:=2
52+
);
53+
54+
select
55+
msg_id,
56+
read_ct,
57+
message
58+
from
59+
pgmq.pop('Foo');
60+
61+
62+
-- Archive message with msg_id=2.
63+
select
64+
pgmq.archive(
65+
queue_name:='Foo',
66+
msg_id:=2
67+
);
68+
69+
70+
select
71+
pgmq.create('my_queue');
72+
73+
select
74+
pgmq.send_batch(
75+
queue_name:='my_queue',
76+
msgs:=array['{"foo": "bar3"}','{"foo": "bar4"}','{"foo": "bar5"}']::jsonb[]
77+
);
78+
79+
select
80+
pgmq.archive(
81+
queue_name:='my_queue',
82+
msg_ids:=array[3, 4, 5]
83+
);
84+
85+
select
86+
pgmq.delete('my_queue', 6);
87+
88+
89+
select
90+
pgmq.drop_queue('my_queue');

0 commit comments

Comments
 (0)