pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #15888: Bogus "idle in transaction" state for logical decoding client after creating a slot
18+ messages / 7 participants
[nested] [flat]
* BUG #15888: Bogus "idle in transaction" state for logical decoding client after creating a slot
@ 2019-07-03 08:03 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 18+ messages in thread
From: PG Bug reporting form @ 2019-07-03 08:03 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: marko@joh.to
The following bug has been logged on the website:
Bug reference: 15888
Logged by: Marko Tiikkaja
Email address: marko@joh.to
PostgreSQL version: 9.6.14
Operating system: OS X 10.12.6
Description:
Run:
pg_recvlogical -S foo -f /dev/null -d dbname --create-slot --start
and pg_stat_activity will show something like this until that session
disconnects:
-[ RECORD 1
]----+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
datid | 9672230
datname | dbname
pid | 81996
usesysid | 10
usename | marko
application_name | pg_recvlogical
client_addr |
client_hostname |
client_port | -1
backend_start | 2019-07-03 10:02:00.459737+02
xact_start |
query_start |
state_change | 2019-07-03 10:02:00.726645+02
wait_event_type |
wait_event |
state | idle in transaction
backend_xid |
backend_xmin |
query |
which is quite obviously bogus.
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15888: Bogus "idle in transaction" state for logical decoding client after creating a slot
@ 2019-07-04 04:35 Michael Paquier <michael@paquier.xyz>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 18+ messages in thread
From: Michael Paquier @ 2019-07-04 04:35 UTC (permalink / raw)
To: marko@joh.to; pgsql-bugs@lists.postgresql.org
On Wed, Jul 03, 2019 at 08:03:24AM +0000, PG Bug reporting form wrote:
> Run:
>
> pg_recvlogical -S foo -f /dev/null -d dbname --create-slot --start
>
> and pg_stat_activity will show something like this until that session
> disconnects:
>
> which is quite obviously bogus.
I think that's the transaction which is started when exporting the
snapshot at logical slot creation. Or in short:
#2 0x000055b5f8e5720f in StartTransactionCommand () at xact.c:2699
#3 0x000055b5f90fab4e in SnapBuildExportSnapshot
(builder=0x55b5fa3427b0) at snapbuild.c:575
#4 0x000055b5f90fe5df in CreateReplicationSlot (cmd=0x55b5fa2914e8)
at walsender.c:843
SnapBuildExportSnapshot() needs to keep a transaction context opened
to keep the wanted xmin around. In PG >= 10, this problem gets
"solved" by using NOEXPORT_SNAPSHOT in the CREATE_REPLICATION_SLOT
command. One idea would be to enforce a commit in this case from
CreateReplicationSlot/streamutil.c? We know that pg_recvlogical is
not going to use this snapshot anyway...
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15888: Bogus "idle in transaction" state for logical decoding client after creating a slot
@ 2019-07-09 20:15 Andres Freund <andres@anarazel.de>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 18+ messages in thread
From: Andres Freund @ 2019-07-09 20:15 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: marko@joh.to; pgsql-bugs@lists.postgresql.org
Hi,
On 2019-07-04 13:35:12 +0900, Michael Paquier wrote:
> On Wed, Jul 03, 2019 at 08:03:24AM +0000, PG Bug reporting form wrote:
> > Run:
> >
> > pg_recvlogical -S foo -f /dev/null -d dbname --create-slot --start
> >
> > and pg_stat_activity will show something like this until that session
> > disconnects:
> >
> > which is quite obviously bogus.
>
> I think that's the transaction which is started when exporting the
> snapshot at logical slot creation. Or in short:
> #2 0x000055b5f8e5720f in StartTransactionCommand () at xact.c:2699
> #3 0x000055b5f90fab4e in SnapBuildExportSnapshot
> (builder=0x55b5fa3427b0) at snapbuild.c:575
> #4 0x000055b5f90fe5df in CreateReplicationSlot (cmd=0x55b5fa2914e8)
> at walsender.c:843
>
> SnapBuildExportSnapshot() needs to keep a transaction context opened
> to keep the wanted xmin around. In PG >= 10, this problem gets
> "solved" by using NOEXPORT_SNAPSHOT in the CREATE_REPLICATION_SLOT
> command. One idea would be to enforce a commit in this case from
> CreateReplicationSlot/streamutil.c? We know that pg_recvlogical is
> not going to use this snapshot anyway...
The transaction ought to be finished by exec_replication_command()
calling SnapBuildClearExportedSnapshot(), which then does
AbortCurrentTransaction(). So something's wrong if that's not working
anymore.
I just tested this on a slightly older v12 checkout, and the general
mechanism works:
S1(replication):
postgres[13464][1]=# CREATE_REPLICATION_SLOT foo LOGICAL test_decoding;
S2(plain):
postgres[13317][1]=# SELECT state, wait_event_type, wait_event, backend_xid, backend_xmin FROM pg_stat_activity WHERE pid = 13464;
┌─────────────────────┬─────────────────┬────────────┬─────────────┬──────────────┐
│ state │ wait_event_type │ wait_event │ backend_xid │ backend_xmin │
├─────────────────────┼─────────────────┼────────────┼─────────────┼──────────────┤
│ idle in transaction │ Client │ ClientRead │ (null) │ 1494339463 │
└─────────────────────┴─────────────────┴────────────┴─────────────┴──────────────┘
(1 row)
S1(replication):
postgres[13464][1]=# IDENTIFY_SYSTEM;
┌─────────────────────┬──────────┬──────────────┬──────────┐
│ systemid │ timeline │ xlogpos │ dbname │
├─────────────────────┼──────────┼──────────────┼──────────┤
│ 6704433050907079638 │ 1 │ 3AC/1A25B700 │ postgres │
└─────────────────────┴──────────┴──────────────┴──────────┘
S2(plain):
postgres[13317][1]=# SELECT state, wait_event_type, wait_event, backend_xid, backend_xmin FROM pg_stat_activity WHERE pid = 13464;
┌───────┬─────────────────┬────────────┬─────────────┬──────────────┐
│ state │ wait_event_type │ wait_event │ backend_xid │ backend_xmin │
├───────┼─────────────────┼────────────┼─────────────┼──────────────┤
│ idle │ Client │ ClientRead │ (null) │ (null) │
└───────┴─────────────────┴────────────┴─────────────┴──────────────┘
(1 row)
I *can* reproduce with 9.6. It's worthwhile to note that the
transaction *is* actually aborted, via the path above. As far as I can
tell the problem is "solely" that we never reach a set_ps_display() that
resets the 'idle in transaction' bit, because START_REPLICATION "takes
over" the connection once started, and we don't go through
PostgresMain() again.
The problem doesn't exist in later versions, because there
exec_replication_command() does an explicit
/* Report to pgstat that this process is now idle */
pgstat_report_activity(STATE_IDLE, NULL);
That's due to
commit d02974e32e028fc078d8f5eca1d6a4516efb0aa6
Author: Magnus Hagander <magnus@hagander.net>
Date: 2017-12-29 16:19:51 +0100
Properly set base backup backends to active in pg_stat_activity
which is post 9.6. Not sure I really feel a huge appetite for whacking
this around in the back branches.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15888: Bogus "idle in transaction" state for logical decoding client after creating a slot
@ 2019-07-09 21:46 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 2 replies; 18+ messages in thread
From: Alvaro Herrera @ 2019-07-09 21:46 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Michael Paquier <michael@paquier.xyz>; marko@joh.to, pgsql-bugs@lists.postgresql.org, Magnus Hagander <magnus@hagander.net>
On 2019-Jul-09, Andres Freund wrote:
> The problem doesn't exist in later versions, because there
> exec_replication_command() does an explicit
> /* Report to pgstat that this process is now idle */
> pgstat_report_activity(STATE_IDLE, NULL);
> That's due to
>
> commit d02974e32e028fc078d8f5eca1d6a4516efb0aa6
> Author: Magnus Hagander <magnus@hagander.net>
> Date: 2017-12-29 16:19:51 +0100
>
> Properly set base backup backends to active in pg_stat_activity
>
> which is post 9.6. Not sure I really feel a huge appetite for whacking
> this around in the back branches.
Hmm ... so what that commit did is precisely to fix this bug.
Magnus thought at the time he was fixing a pg10 bug,
https://postgr.es/m/CABUevEwX4g8y=gmgfPzxFKS7gqjSYNR949Xc96OQm=YXJmh_Og@mail.gmail.com
but apparently now we see that the bug was older than that. Maybe it's
okay to backpatch further?
(Booh to missing "Discussion:" tags in commit messages)
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15888: Bogus "idle in transaction" state for logical decoding client after creating a slot
@ 2019-07-10 04:51 Michael Paquier <michael@paquier.xyz>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 1 reply; 18+ messages in thread
From: Michael Paquier @ 2019-07-10 04:51 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Andres Freund <andres@anarazel.de>; marko@joh.to, pgsql-bugs@lists.postgresql.org, Magnus Hagander <magnus@hagander.net>
On Tue, Jul 09, 2019 at 05:46:30PM -0400, Alvaro Herrera wrote:
> Hmm ... so what that commit did is precisely to fix this bug.
> Magnus thought at the time he was fixing a pg10 bug,
> https://postgr.es/m/CABUevEwX4g8y=gmgfPzxFKS7gqjSYNR949Xc96OQm=YXJmh_Og@mail.gmail.com
> but apparently now we see that the bug was older than that. Maybe it's
> okay to backpatch further?
Yes, I would not be against a back-patch in this case. There is a
minor conflict because pre-9.6 WAL senders cannot run SQL queries but
that looks simple enough to solve.
> (Booh to missing "Discussion:" tags in commit messages)
Booh++
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15888: Bogus "idle in transaction" state for logical decoding client after creating a slot
@ 2019-07-10 07:55 Andres Freund <andres@anarazel.de>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 18+ messages in thread
From: Andres Freund @ 2019-07-10 07:55 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; marko@joh.to, pgsql-bugs@lists.postgresql.org, Magnus Hagander <magnus@hagander.net>
Hi,
On 2019-07-10 13:51:47 +0900, Michael Paquier wrote:
> On Tue, Jul 09, 2019 at 05:46:30PM -0400, Alvaro Herrera wrote:
> > Hmm ... so what that commit did is precisely to fix this bug.
> > Magnus thought at the time he was fixing a pg10 bug,
> > https://postgr.es/m/CABUevEwX4g8y=gmgfPzxFKS7gqjSYNR949Xc96OQm=YXJmh_Og@mail.gmail.com
> > but apparently now we see that the bug was older than that. Maybe it's
> > okay to backpatch further?
>
> Yes, I would not be against a back-patch in this case. There is a
> minor conflict because pre-9.6 WAL senders cannot run SQL queries but
> that looks simple enough to solve.
I'm not worried about backpatching that in isolation - but I'm worried
that just backpatching that indidividual commit isn't going to yield
particularly satisfactory behaviour. The code around this isn't super
robust, and pg10+ are noticably different due to the changes needed to
allow to execute normal queries over a replication connection.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15888: Bogus "idle in transaction" state for logical decoding client after creating a slot
@ 2019-07-10 15:24 Magnus Hagander <magnus@hagander.net>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 1 reply; 18+ messages in thread
From: Magnus Hagander @ 2019-07-10 15:24 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Andres Freund <andres@anarazel.de>; Michael Paquier <michael@paquier.xyz>; Marko Tiikkaja <marko@joh.to>; pgsql-bugs@lists.postgresql.org
On Tue, Jul 9, 2019 at 11:46 PM Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:
> On 2019-Jul-09, Andres Freund wrote:
>
> > The problem doesn't exist in later versions, because there
> > exec_replication_command() does an explicit
> > /* Report to pgstat that this process is now idle */
> > pgstat_report_activity(STATE_IDLE, NULL);
> > That's due to
> >
> > commit d02974e32e028fc078d8f5eca1d6a4516efb0aa6
> > Author: Magnus Hagander <magnus@hagander.net>
> > Date: 2017-12-29 16:19:51 +0100
> >
> > Properly set base backup backends to active in pg_stat_activity
> >
> > which is post 9.6. Not sure I really feel a huge appetite for whacking
> > this around in the back branches.
>
> Hmm ... so what that commit did is precisely to fix this bug.
> Magnus thought at the time he was fixing a pg10 bug,
>
> https://postgr.es/m/CABUevEwX4g8y=gmgfPzxFKS7gqjSYNR949Xc96OQm=YXJmh_Og@mail.gmail.com
> but apparently now we see that the bug was older than that. Maybe it's
> okay to backpatch further?
>
I don't recall any particular reason around the backpatching other than as
you say, probably not realizing it. At the time we didn't have logical
replication yet (it landed later), so the actual uses of the logical
decoding were nowhere near as frequent...
(Booh to missing "Discussion:" tags in commit messages)
>
>
If only we could agree on a template/standard for those :P
//Magnus
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15888: Bogus "idle in transaction" state for logical decoding client after creating a slot
@ 2019-07-10 23:58 Michael Paquier <michael@paquier.xyz>
parent: Magnus Hagander <magnus@hagander.net>
0 siblings, 1 reply; 18+ messages in thread
From: Michael Paquier @ 2019-07-10 23:58 UTC (permalink / raw)
To: Magnus Hagander <magnus@hagander.net>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Andres Freund <andres@anarazel.de>; Marko Tiikkaja <marko@joh.to>; pgsql-bugs@lists.postgresql.org
On Wed, Jul 10, 2019 at 05:24:44PM +0200, Magnus Hagander wrote:
> On Tue, Jul 9, 2019 at 11:46 PM Alvaro Herrera <alvherre@2ndquadrant.com>
> wrote:
> (Booh to missing "Discussion:" tags in commit messages)
>>
>>
> If only we could agree on a template/standard for those :P
This would make an excellent topic for a developper meeting with most
of the main committers around the table.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15888: Bogus "idle in transaction" state for logical decoding client after creating a slot
@ 2019-07-11 11:21 Magnus Hagander <magnus@hagander.net>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 2 replies; 18+ messages in thread
From: Magnus Hagander @ 2019-07-11 11:21 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Andres Freund <andres@anarazel.de>; Marko Tiikkaja <marko@joh.to>; pgsql-bugs@lists.postgresql.org
On Thu, Jul 11, 2019 at 1:58 AM Michael Paquier <michael@paquier.xyz> wrote:
> On Wed, Jul 10, 2019 at 05:24:44PM +0200, Magnus Hagander wrote:
> > On Tue, Jul 9, 2019 at 11:46 PM Alvaro Herrera <alvherre@2ndquadrant.com
> >
> > wrote:
> > (Booh to missing "Discussion:" tags in commit messages)
> >>
> >>
> > If only we could agree on a template/standard for those :P
>
> This would make an excellent topic for a developper meeting with most
> of the main committers around the table.
>
We've tried that once, and I think even twice, and failed miserably at any
form of agreement. But we could certainly try again :)
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/;
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15888: Bogus "idle in transaction" state for logical decoding client after creating a slot
@ 2019-07-11 13:09 Michael Paquier <michael@paquier.xyz>
parent: Magnus Hagander <magnus@hagander.net>
1 sibling, 0 replies; 18+ messages in thread
From: Michael Paquier @ 2019-07-11 13:09 UTC (permalink / raw)
To: Magnus Hagander <magnus@hagander.net>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Andres Freund <andres@anarazel.de>; Marko Tiikkaja <marko@joh.to>; pgsql-bugs@lists.postgresql.org
On Thu, Jul 11, 2019 at 01:21:39PM +0200, Magnus Hagander wrote:
> We've tried that once, and I think even twice, and failed miserably at any
> form of agreement. But we could certainly try again :)
Or we could just drop a thread about that..
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15888: Bogus "idle in transaction" state for logical decoding client after creating a slot
@ 2019-07-11 13:44 Tom Lane <tgl@sss.pgh.pa.us>
parent: Magnus Hagander <magnus@hagander.net>
1 sibling, 1 reply; 18+ messages in thread
From: Tom Lane @ 2019-07-11 13:44 UTC (permalink / raw)
To: Magnus Hagander <magnus@hagander.net>; +Cc: Michael Paquier <michael@paquier.xyz>; Alvaro Herrera <alvherre@2ndquadrant.com>; Andres Freund <andres@anarazel.de>; Marko Tiikkaja <marko@joh.to>; pgsql-bugs@lists.postgresql.org
Magnus Hagander <magnus@hagander.net> writes:
> We've tried that once, and I think even twice, and failed miserably at any
> form of agreement. But we could certainly try again :)
I thought we *did* have an agreement, to wit using
Discussion: https://postgr.es/m/<message-id;
to link to relevant mail thread(s). Some people use more tags
but that seems inessential to me.
regards, tom lane
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15888: Bogus "idle in transaction" state for logical decoding client after creating a slot
@ 2019-07-12 01:25 Michael Paquier <michael@paquier.xyz>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 18+ messages in thread
From: Michael Paquier @ 2019-07-12 01:25 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Magnus Hagander <magnus@hagander.net>; Alvaro Herrera <alvherre@2ndquadrant.com>; Andres Freund <andres@anarazel.de>; Marko Tiikkaja <marko@joh.to>; pgsql-bugs@lists.postgresql.org
On Thu, Jul 11, 2019 at 09:44:07AM -0400, Tom Lane wrote:
> I thought we *did* have an agreement, to wit using
>
> Discussion: https://postgr.es/m/<message-id;
>
> to link to relevant mail thread(s). Some people use more tags
> but that seems inessential to me.
Hehe. I actually was thinking about advocating for having more of
them in the commit logs. I'll just start a new thread about what I
had in mind. Perhaps that will lead us nowhere, but let's see.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 18+ messages in thread
* [PATCH v2 1/2] Fix bug in pg_logicalinspect functions
@ 2025-03-04 05:43 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 0 replies; 18+ messages in thread
From: Bertrand Drouvot @ 2025-03-04 05:43 UTC (permalink / raw)
The functions did not handle passed empty strings correctly and were not sanitizing
the request to prevent reference to files outside the snapshots directory.
Per bug #18828 from Robins Tharakan.
---
contrib/pg_logicalinspect/Makefile | 1 +
.../expected/pg_logicalinspect.out | 71 +++++++++++++++++++
contrib/pg_logicalinspect/pg_logicalinspect.c | 46 +++++++++---
.../sql/pg_logicalinspect.sql | 43 +++++++++++
src/backend/replication/logical/snapbuild.c | 14 ++--
src/include/replication/snapbuild_internal.h | 2 +-
6 files changed, 159 insertions(+), 18 deletions(-)
41.9% contrib/pg_logicalinspect/expected/
29.0% contrib/pg_logicalinspect/sql/
21.6% contrib/pg_logicalinspect/
4.6% src/backend/replication/logical/
inline patch 1: contrib/pg_logicalinspect/Makefile, contrib/pg_logicalinspect/expected/pg_logicalinspect.out, contrib/pg_logicalinspect/pg_logicalinspect.c and 3 more (+159/-18 lines) download inline-1.patch
diff --git a/contrib/pg_logicalinspect/Makefile b/contrib/pg_logicalinspect/Makefile
index 55124514d42..17cee50d5c8 100644
--- a/contrib/pg_logicalinspect/Makefile
+++ b/contrib/pg_logicalinspect/Makefile
@@ -11,6 +11,7 @@ DATA = pg_logicalinspect--1.0.sql
EXTRA_INSTALL = contrib/test_decoding
+REGRESS = pg_logicalinspect
ISOLATION = logical_inspect
ISOLATION_OPTS = --temp-config $(top_srcdir)/contrib/pg_logicalinspect/logicalinspect.conf
diff --git a/contrib/pg_logicalinspect/expected/pg_logicalinspect.out b/contrib/pg_logicalinspect/expected/pg_logicalinspect.out
new file mode 100644
index 00000000000..f87a48e1597
--- /dev/null
+++ b/contrib/pg_logicalinspect/expected/pg_logicalinspect.out
@@ -0,0 +1,71 @@
+CREATE EXTENSION pg_logicalinspect;
+-- ===================================================================
+-- Tests for input validation
+-- ===================================================================
+SELECT pg_get_logical_snapshot_info('0-40796E18.foo');
+ERROR: invalid snapshot file name "0-40796E18.foo"
+SELECT pg_get_logical_snapshot_info('0/40796E18.snap');
+ERROR: invalid snapshot file name "0/40796E18.snap"
+SELECT pg_get_logical_snapshot_info('');
+ERROR: invalid snapshot file name ""
+SELECT pg_get_logical_snapshot_info(NULL);
+ pg_get_logical_snapshot_info
+------------------------------
+
+(1 row)
+
+SELECT pg_get_logical_snapshot_info('../snapshots');
+ERROR: invalid snapshot file name "../snapshots"
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo');
+ERROR: invalid snapshot file name "0-40796E18.foo"
+SELECT pg_get_logical_snapshot_meta('0/40796E18.snap');
+ERROR: invalid snapshot file name "0/40796E18.snap"
+SELECT pg_get_logical_snapshot_meta('');
+ERROR: invalid snapshot file name ""
+SELECT pg_get_logical_snapshot_meta(NULL);
+ pg_get_logical_snapshot_meta
+------------------------------
+
+(1 row)
+
+SELECT pg_get_logical_snapshot_meta('../snapshots');
+ERROR: invalid snapshot file name "../snapshots"
+-- ===================================================================
+-- Tests for permissions
+-- ===================================================================
+CREATE ROLE regress_pg_logicalinspect;
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- no
+ has_function_privilege
+------------------------
+ f
+(1 row)
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- no
+ has_function_privilege
+------------------------
+ f
+(1 row)
+
+-- Functions accessible by users with role pg_read_server_files.
+GRANT pg_read_server_files TO regress_pg_logicalinspect;
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- yes
+ has_function_privilege
+------------------------
+ t
+(1 row)
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- yes
+ has_function_privilege
+------------------------
+ t
+(1 row)
+
+-- ===================================================================
+-- Clean up
+-- ===================================================================
+DROP ROLE regress_pg_logicalinspect;
+DROP EXTENSION pg_logicalinspect;
diff --git a/contrib/pg_logicalinspect/pg_logicalinspect.c b/contrib/pg_logicalinspect/pg_logicalinspect.c
index cd575c6bd36..6f6a7bbcc83 100644
--- a/contrib/pg_logicalinspect/pg_logicalinspect.c
+++ b/contrib/pg_logicalinspect/pg_logicalinspect.c
@@ -48,6 +48,36 @@ get_snapbuild_state_desc(SnapBuildState state)
return stateDesc;
}
+/*
+ * Extract the LSN from the given serialized snapshot file name.
+ */
+static XLogRecPtr
+parse_snapshot_filename(const char *filename)
+{
+ uint32 hi;
+ uint32 lo;
+ XLogRecPtr lsn;
+ const char *file_ext;
+ bool report_error = false;
+
+ /* Check for the .snap extension */
+ file_ext = strrchr(filename, '.');
+
+ if (file_ext == NULL || strcmp(file_ext, ".snap") != 0)
+ report_error = true;
+
+ if (sscanf(filename, "%X-%X", &hi, &lo) != 2)
+ report_error = true;
+
+ if (report_error)
+ ereport(ERROR,
+ errmsg("invalid snapshot file name \"%s\"", filename));
+
+ lsn = ((uint64) hi) << 32 | lo;
+
+ return lsn;
+}
+
/*
* Retrieve the logical snapshot file metadata.
*/
@@ -60,7 +90,7 @@ pg_get_logical_snapshot_meta(PG_FUNCTION_ARGS)
Datum values[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
TupleDesc tupdesc;
- char path[MAXPGPATH];
+ XLogRecPtr lsn;
int i = 0;
text *filename_t = PG_GETARG_TEXT_PP(0);
@@ -68,12 +98,10 @@ pg_get_logical_snapshot_meta(PG_FUNCTION_ARGS)
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
- sprintf(path, "%s/%s",
- PG_LOGICAL_SNAPSHOTS_DIR,
- text_to_cstring(filename_t));
+ lsn = parse_snapshot_filename(text_to_cstring(filename_t));
/* Validate and restore the snapshot to 'ondisk' */
- SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);
+ SnapBuildRestoreSnapshot(&ondisk, lsn, CurrentMemoryContext, false);
values[i++] = UInt32GetDatum(ondisk.magic);
values[i++] = Int64GetDatum((int64) ondisk.checksum);
@@ -97,7 +125,7 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
Datum values[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
bool nulls[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
TupleDesc tupdesc;
- char path[MAXPGPATH];
+ XLogRecPtr lsn;
int i = 0;
text *filename_t = PG_GETARG_TEXT_PP(0);
@@ -105,12 +133,10 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
- sprintf(path, "%s/%s",
- PG_LOGICAL_SNAPSHOTS_DIR,
- text_to_cstring(filename_t));
+ lsn = parse_snapshot_filename(text_to_cstring(filename_t));
/* Validate and restore the snapshot to 'ondisk' */
- SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);
+ SnapBuildRestoreSnapshot(&ondisk, lsn, CurrentMemoryContext, false);
values[i++] = CStringGetTextDatum(get_snapbuild_state_desc(ondisk.builder.state));
values[i++] = TransactionIdGetDatum(ondisk.builder.xmin);
diff --git a/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql b/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql
new file mode 100644
index 00000000000..cf73c40df16
--- /dev/null
+++ b/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql
@@ -0,0 +1,43 @@
+CREATE EXTENSION pg_logicalinspect;
+
+-- ===================================================================
+-- Tests for input validation
+-- ===================================================================
+
+SELECT pg_get_logical_snapshot_info('0-40796E18.foo');
+SELECT pg_get_logical_snapshot_info('0/40796E18.snap');
+SELECT pg_get_logical_snapshot_info('');
+SELECT pg_get_logical_snapshot_info(NULL);
+SELECT pg_get_logical_snapshot_info('../snapshots');
+
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo');
+SELECT pg_get_logical_snapshot_meta('0/40796E18.snap');
+SELECT pg_get_logical_snapshot_meta('');
+SELECT pg_get_logical_snapshot_meta(NULL);
+SELECT pg_get_logical_snapshot_meta('../snapshots');
+
+-- ===================================================================
+-- Tests for permissions
+-- ===================================================================
+CREATE ROLE regress_pg_logicalinspect;
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- no
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- no
+
+-- Functions accessible by users with role pg_read_server_files.
+GRANT pg_read_server_files TO regress_pg_logicalinspect;
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- yes
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- yes
+
+-- ===================================================================
+-- Clean up
+-- ===================================================================
+
+DROP ROLE regress_pg_logicalinspect;
+
+DROP EXTENSION pg_logicalinspect;
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index bd0680dcbe5..b64e53de017 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1691,12 +1691,17 @@ out:
* If 'missing_ok' is true, will not throw an error if the file is not found.
*/
bool
-SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
+SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, XLogRecPtr lsn,
MemoryContext context, bool missing_ok)
{
int fd;
pg_crc32c checksum;
Size sz;
+ char path[MAXPGPATH];
+
+ sprintf(path, "%s/%X-%X.snap",
+ PG_LOGICAL_SNAPSHOTS_DIR,
+ LSN_FORMAT_ARGS(lsn));
fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
@@ -1788,18 +1793,13 @@ static bool
SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
{
SnapBuildOnDisk ondisk;
- char path[MAXPGPATH];
/* no point in loading a snapshot if we're already there */
if (builder->state == SNAPBUILD_CONSISTENT)
return false;
- sprintf(path, "%s/%X-%X.snap",
- PG_LOGICAL_SNAPSHOTS_DIR,
- LSN_FORMAT_ARGS(lsn));
-
/* validate and restore the snapshot to 'ondisk' */
- if (!SnapBuildRestoreSnapshot(&ondisk, path, builder->context, true))
+ if (!SnapBuildRestoreSnapshot(&ondisk, lsn, builder->context, true))
return false;
/*
diff --git a/src/include/replication/snapbuild_internal.h b/src/include/replication/snapbuild_internal.h
index 081b01b890a..3b915dc8793 100644
--- a/src/include/replication/snapbuild_internal.h
+++ b/src/include/replication/snapbuild_internal.h
@@ -193,7 +193,7 @@ typedef struct SnapBuildOnDisk
/* variable amount of TransactionIds follows */
} SnapBuildOnDisk;
-extern bool SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
+extern bool SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, XLogRecPtr lsn,
MemoryContext context, bool missing_ok);
#endif /* SNAPBUILD_INTERNAL_H */
--
2.34.1
* [PATCH v3 1/2] Fix bug in pg_logicalinspect functions
@ 2025-03-04 05:43 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 0 replies; 18+ messages in thread
From: Bertrand Drouvot @ 2025-03-04 05:43 UTC (permalink / raw)
The functions did not handle passed empty strings correctly and were not sanitizing
the request to prevent reference to files outside the snapshots directory.
Per bug #18828 from Robins Tharakan.
---
contrib/pg_logicalinspect/Makefile | 1 +
.../expected/pg_logicalinspect.out | 71 +++++++++++++++++++
contrib/pg_logicalinspect/pg_logicalinspect.c | 46 +++++++++---
.../sql/pg_logicalinspect.sql | 43 +++++++++++
src/backend/replication/logical/snapbuild.c | 14 ++--
src/include/replication/snapbuild_internal.h | 2 +-
6 files changed, 159 insertions(+), 18 deletions(-)
41.8% contrib/pg_logicalinspect/expected/
29.0% contrib/pg_logicalinspect/sql/
21.8% contrib/pg_logicalinspect/
4.5% src/backend/replication/logical/
inline patch 1: contrib/pg_logicalinspect/Makefile, contrib/pg_logicalinspect/expected/pg_logicalinspect.out, contrib/pg_logicalinspect/pg_logicalinspect.c and 3 more (+159/-18 lines) download inline-1.patch
diff --git a/contrib/pg_logicalinspect/Makefile b/contrib/pg_logicalinspect/Makefile
index 55124514d42..17cee50d5c8 100644
--- a/contrib/pg_logicalinspect/Makefile
+++ b/contrib/pg_logicalinspect/Makefile
@@ -11,6 +11,7 @@ DATA = pg_logicalinspect--1.0.sql
EXTRA_INSTALL = contrib/test_decoding
+REGRESS = pg_logicalinspect
ISOLATION = logical_inspect
ISOLATION_OPTS = --temp-config $(top_srcdir)/contrib/pg_logicalinspect/logicalinspect.conf
diff --git a/contrib/pg_logicalinspect/expected/pg_logicalinspect.out b/contrib/pg_logicalinspect/expected/pg_logicalinspect.out
new file mode 100644
index 00000000000..f87a48e1597
--- /dev/null
+++ b/contrib/pg_logicalinspect/expected/pg_logicalinspect.out
@@ -0,0 +1,71 @@
+CREATE EXTENSION pg_logicalinspect;
+-- ===================================================================
+-- Tests for input validation
+-- ===================================================================
+SELECT pg_get_logical_snapshot_info('0-40796E18.foo');
+ERROR: invalid snapshot file name "0-40796E18.foo"
+SELECT pg_get_logical_snapshot_info('0/40796E18.snap');
+ERROR: invalid snapshot file name "0/40796E18.snap"
+SELECT pg_get_logical_snapshot_info('');
+ERROR: invalid snapshot file name ""
+SELECT pg_get_logical_snapshot_info(NULL);
+ pg_get_logical_snapshot_info
+------------------------------
+
+(1 row)
+
+SELECT pg_get_logical_snapshot_info('../snapshots');
+ERROR: invalid snapshot file name "../snapshots"
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo');
+ERROR: invalid snapshot file name "0-40796E18.foo"
+SELECT pg_get_logical_snapshot_meta('0/40796E18.snap');
+ERROR: invalid snapshot file name "0/40796E18.snap"
+SELECT pg_get_logical_snapshot_meta('');
+ERROR: invalid snapshot file name ""
+SELECT pg_get_logical_snapshot_meta(NULL);
+ pg_get_logical_snapshot_meta
+------------------------------
+
+(1 row)
+
+SELECT pg_get_logical_snapshot_meta('../snapshots');
+ERROR: invalid snapshot file name "../snapshots"
+-- ===================================================================
+-- Tests for permissions
+-- ===================================================================
+CREATE ROLE regress_pg_logicalinspect;
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- no
+ has_function_privilege
+------------------------
+ f
+(1 row)
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- no
+ has_function_privilege
+------------------------
+ f
+(1 row)
+
+-- Functions accessible by users with role pg_read_server_files.
+GRANT pg_read_server_files TO regress_pg_logicalinspect;
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- yes
+ has_function_privilege
+------------------------
+ t
+(1 row)
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- yes
+ has_function_privilege
+------------------------
+ t
+(1 row)
+
+-- ===================================================================
+-- Clean up
+-- ===================================================================
+DROP ROLE regress_pg_logicalinspect;
+DROP EXTENSION pg_logicalinspect;
diff --git a/contrib/pg_logicalinspect/pg_logicalinspect.c b/contrib/pg_logicalinspect/pg_logicalinspect.c
index cd575c6bd36..193be8a6ed5 100644
--- a/contrib/pg_logicalinspect/pg_logicalinspect.c
+++ b/contrib/pg_logicalinspect/pg_logicalinspect.c
@@ -48,6 +48,36 @@ get_snapbuild_state_desc(SnapBuildState state)
return stateDesc;
}
+/*
+ * Extract the LSN from the given serialized snapshot file name.
+ */
+static XLogRecPtr
+parse_snapshot_filename(const char *filename)
+{
+ uint32 hi;
+ uint32 lo;
+ XLogRecPtr lsn;
+ const char *file_ext;
+ bool report_error = false;
+
+ /* Check for the .snap extension */
+ file_ext = strrchr(filename, '.');
+
+ if (file_ext == NULL || strcmp(file_ext, ".snap") != 0)
+ report_error = true;
+
+ if (!report_error && sscanf(filename, "%X-%X", &hi, &lo) != 2)
+ report_error = true;
+
+ if (report_error)
+ ereport(ERROR,
+ errmsg("invalid snapshot file name \"%s\"", filename));
+
+ lsn = ((uint64) hi) << 32 | lo;
+
+ return lsn;
+}
+
/*
* Retrieve the logical snapshot file metadata.
*/
@@ -60,7 +90,7 @@ pg_get_logical_snapshot_meta(PG_FUNCTION_ARGS)
Datum values[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
TupleDesc tupdesc;
- char path[MAXPGPATH];
+ XLogRecPtr lsn;
int i = 0;
text *filename_t = PG_GETARG_TEXT_PP(0);
@@ -68,12 +98,10 @@ pg_get_logical_snapshot_meta(PG_FUNCTION_ARGS)
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
- sprintf(path, "%s/%s",
- PG_LOGICAL_SNAPSHOTS_DIR,
- text_to_cstring(filename_t));
+ lsn = parse_snapshot_filename(text_to_cstring(filename_t));
/* Validate and restore the snapshot to 'ondisk' */
- SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);
+ SnapBuildRestoreSnapshot(&ondisk, lsn, CurrentMemoryContext, false);
values[i++] = UInt32GetDatum(ondisk.magic);
values[i++] = Int64GetDatum((int64) ondisk.checksum);
@@ -97,7 +125,7 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
Datum values[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
bool nulls[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
TupleDesc tupdesc;
- char path[MAXPGPATH];
+ XLogRecPtr lsn;
int i = 0;
text *filename_t = PG_GETARG_TEXT_PP(0);
@@ -105,12 +133,10 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
- sprintf(path, "%s/%s",
- PG_LOGICAL_SNAPSHOTS_DIR,
- text_to_cstring(filename_t));
+ lsn = parse_snapshot_filename(text_to_cstring(filename_t));
/* Validate and restore the snapshot to 'ondisk' */
- SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);
+ SnapBuildRestoreSnapshot(&ondisk, lsn, CurrentMemoryContext, false);
values[i++] = CStringGetTextDatum(get_snapbuild_state_desc(ondisk.builder.state));
values[i++] = TransactionIdGetDatum(ondisk.builder.xmin);
diff --git a/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql b/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql
new file mode 100644
index 00000000000..cf73c40df16
--- /dev/null
+++ b/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql
@@ -0,0 +1,43 @@
+CREATE EXTENSION pg_logicalinspect;
+
+-- ===================================================================
+-- Tests for input validation
+-- ===================================================================
+
+SELECT pg_get_logical_snapshot_info('0-40796E18.foo');
+SELECT pg_get_logical_snapshot_info('0/40796E18.snap');
+SELECT pg_get_logical_snapshot_info('');
+SELECT pg_get_logical_snapshot_info(NULL);
+SELECT pg_get_logical_snapshot_info('../snapshots');
+
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo');
+SELECT pg_get_logical_snapshot_meta('0/40796E18.snap');
+SELECT pg_get_logical_snapshot_meta('');
+SELECT pg_get_logical_snapshot_meta(NULL);
+SELECT pg_get_logical_snapshot_meta('../snapshots');
+
+-- ===================================================================
+-- Tests for permissions
+-- ===================================================================
+CREATE ROLE regress_pg_logicalinspect;
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- no
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- no
+
+-- Functions accessible by users with role pg_read_server_files.
+GRANT pg_read_server_files TO regress_pg_logicalinspect;
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- yes
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- yes
+
+-- ===================================================================
+-- Clean up
+-- ===================================================================
+
+DROP ROLE regress_pg_logicalinspect;
+
+DROP EXTENSION pg_logicalinspect;
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index bd0680dcbe5..b64e53de017 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1691,12 +1691,17 @@ out:
* If 'missing_ok' is true, will not throw an error if the file is not found.
*/
bool
-SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
+SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, XLogRecPtr lsn,
MemoryContext context, bool missing_ok)
{
int fd;
pg_crc32c checksum;
Size sz;
+ char path[MAXPGPATH];
+
+ sprintf(path, "%s/%X-%X.snap",
+ PG_LOGICAL_SNAPSHOTS_DIR,
+ LSN_FORMAT_ARGS(lsn));
fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
@@ -1788,18 +1793,13 @@ static bool
SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
{
SnapBuildOnDisk ondisk;
- char path[MAXPGPATH];
/* no point in loading a snapshot if we're already there */
if (builder->state == SNAPBUILD_CONSISTENT)
return false;
- sprintf(path, "%s/%X-%X.snap",
- PG_LOGICAL_SNAPSHOTS_DIR,
- LSN_FORMAT_ARGS(lsn));
-
/* validate and restore the snapshot to 'ondisk' */
- if (!SnapBuildRestoreSnapshot(&ondisk, path, builder->context, true))
+ if (!SnapBuildRestoreSnapshot(&ondisk, lsn, builder->context, true))
return false;
/*
diff --git a/src/include/replication/snapbuild_internal.h b/src/include/replication/snapbuild_internal.h
index 081b01b890a..3b915dc8793 100644
--- a/src/include/replication/snapbuild_internal.h
+++ b/src/include/replication/snapbuild_internal.h
@@ -193,7 +193,7 @@ typedef struct SnapBuildOnDisk
/* variable amount of TransactionIds follows */
} SnapBuildOnDisk;
-extern bool SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
+extern bool SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, XLogRecPtr lsn,
MemoryContext context, bool missing_ok);
#endif /* SNAPBUILD_INTERNAL_H */
--
2.34.1
* [PATCH v4] Fix bug in pg_logicalinspect functions
@ 2025-03-04 05:43 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 0 replies; 18+ messages in thread
From: Bertrand Drouvot @ 2025-03-04 05:43 UTC (permalink / raw)
The functions did not handle passed empty strings correctly and were not sanitizing
the request to prevent reference to files outside the snapshots directory.
Per bug #18828 from Robins Tharakan.
---
contrib/pg_logicalinspect/Makefile | 1 +
.../expected/pg_logicalinspect.out | 75 +++++++++++++++++++
contrib/pg_logicalinspect/pg_logicalinspect.c | 61 ++++++++++++---
.../sql/pg_logicalinspect.sql | 45 +++++++++++
src/backend/replication/logical/snapbuild.c | 14 ++--
src/include/replication/snapbuild_internal.h | 2 +-
6 files changed, 180 insertions(+), 18 deletions(-)
41.0% contrib/pg_logicalinspect/expected/
27.8% contrib/pg_logicalinspect/sql/
24.5% contrib/pg_logicalinspect/
4.1% src/backend/replication/logical/
inline patch 1: contrib/pg_logicalinspect/Makefile, contrib/pg_logicalinspect/expected/pg_logicalinspect.out, contrib/pg_logicalinspect/pg_logicalinspect.c and 3 more (+180/-18 lines) download inline-1.patch
diff --git a/contrib/pg_logicalinspect/Makefile b/contrib/pg_logicalinspect/Makefile
index 55124514d42..17cee50d5c8 100644
--- a/contrib/pg_logicalinspect/Makefile
+++ b/contrib/pg_logicalinspect/Makefile
@@ -11,6 +11,7 @@ DATA = pg_logicalinspect--1.0.sql
EXTRA_INSTALL = contrib/test_decoding
+REGRESS = pg_logicalinspect
ISOLATION = logical_inspect
ISOLATION_OPTS = --temp-config $(top_srcdir)/contrib/pg_logicalinspect/logicalinspect.conf
diff --git a/contrib/pg_logicalinspect/expected/pg_logicalinspect.out b/contrib/pg_logicalinspect/expected/pg_logicalinspect.out
new file mode 100644
index 00000000000..f1f0712bec2
--- /dev/null
+++ b/contrib/pg_logicalinspect/expected/pg_logicalinspect.out
@@ -0,0 +1,75 @@
+CREATE EXTENSION pg_logicalinspect;
+-- ===================================================================
+-- Tests for input validation
+-- ===================================================================
+SELECT pg_get_logical_snapshot_info('0-40796E18.foo');
+ERROR: invalid snapshot file name "0-40796E18.foo"
+SELECT pg_get_logical_snapshot_info('0/40796E18.foo.snap');
+ERROR: invalid snapshot file name "0/40796E18.foo.snap"
+SELECT pg_get_logical_snapshot_info('0/40796E18.snap');
+ERROR: invalid snapshot file name "0/40796E18.snap"
+SELECT pg_get_logical_snapshot_info('');
+ERROR: invalid snapshot file name ""
+SELECT pg_get_logical_snapshot_info(NULL);
+ pg_get_logical_snapshot_info
+------------------------------
+
+(1 row)
+
+SELECT pg_get_logical_snapshot_info('../snapshots');
+ERROR: invalid snapshot file name "../snapshots"
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo');
+ERROR: invalid snapshot file name "0-40796E18.foo"
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo.snap');
+ERROR: invalid snapshot file name "0-40796E18.foo.snap"
+SELECT pg_get_logical_snapshot_meta('0/40796E18.snap');
+ERROR: invalid snapshot file name "0/40796E18.snap"
+SELECT pg_get_logical_snapshot_meta('');
+ERROR: invalid snapshot file name ""
+SELECT pg_get_logical_snapshot_meta(NULL);
+ pg_get_logical_snapshot_meta
+------------------------------
+
+(1 row)
+
+SELECT pg_get_logical_snapshot_meta('../snapshots');
+ERROR: invalid snapshot file name "../snapshots"
+-- ===================================================================
+-- Tests for permissions
+-- ===================================================================
+CREATE ROLE regress_pg_logicalinspect;
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- no
+ has_function_privilege
+------------------------
+ f
+(1 row)
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- no
+ has_function_privilege
+------------------------
+ f
+(1 row)
+
+-- Functions accessible by users with role pg_read_server_files.
+GRANT pg_read_server_files TO regress_pg_logicalinspect;
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- yes
+ has_function_privilege
+------------------------
+ t
+(1 row)
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- yes
+ has_function_privilege
+------------------------
+ t
+(1 row)
+
+-- ===================================================================
+-- Clean up
+-- ===================================================================
+DROP ROLE regress_pg_logicalinspect;
+DROP EXTENSION pg_logicalinspect;
diff --git a/contrib/pg_logicalinspect/pg_logicalinspect.c b/contrib/pg_logicalinspect/pg_logicalinspect.c
index cd575c6bd36..1c37071ea2b 100644
--- a/contrib/pg_logicalinspect/pg_logicalinspect.c
+++ b/contrib/pg_logicalinspect/pg_logicalinspect.c
@@ -48,6 +48,51 @@ get_snapbuild_state_desc(SnapBuildState state)
return stateDesc;
}
+/*
+ * Extract the LSN from the given serialized snapshot file name.
+ */
+static XLogRecPtr
+parse_snapshot_filename(const char *filename)
+{
+ uint32 hi;
+ uint32 lo;
+ XLogRecPtr lsn;
+ const char *file_ext;
+ bool report_error = false;
+
+ /* Check for the .snap extension */
+ file_ext = strrchr(filename, '.');
+
+ if (file_ext == NULL || strcmp(file_ext, ".snap") != 0)
+ report_error = true;
+
+ /* Extract the values to build the LSN */
+ if (!report_error && sscanf(filename, "%X-%X", &hi, &lo) != 2)
+ report_error = true;
+
+ /* Ensure filename is %X-%X.snap */
+ if (!report_error)
+ {
+ size_t buffer_size = strlen(filename) + 1;
+ char *buffer = palloc(buffer_size);
+
+ snprintf(buffer, buffer_size, "%X-%X.snap", hi, lo);
+
+ if (strcmp(buffer, filename) != 0)
+ report_error = true;
+
+ pfree(buffer);
+ }
+
+ if (report_error)
+ ereport(ERROR,
+ errmsg("invalid snapshot file name \"%s\"", filename));
+
+ lsn = ((uint64) hi) << 32 | lo;
+
+ return lsn;
+}
+
/*
* Retrieve the logical snapshot file metadata.
*/
@@ -60,7 +105,7 @@ pg_get_logical_snapshot_meta(PG_FUNCTION_ARGS)
Datum values[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
TupleDesc tupdesc;
- char path[MAXPGPATH];
+ XLogRecPtr lsn;
int i = 0;
text *filename_t = PG_GETARG_TEXT_PP(0);
@@ -68,12 +113,10 @@ pg_get_logical_snapshot_meta(PG_FUNCTION_ARGS)
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
- sprintf(path, "%s/%s",
- PG_LOGICAL_SNAPSHOTS_DIR,
- text_to_cstring(filename_t));
+ lsn = parse_snapshot_filename(text_to_cstring(filename_t));
/* Validate and restore the snapshot to 'ondisk' */
- SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);
+ SnapBuildRestoreSnapshot(&ondisk, lsn, CurrentMemoryContext, false);
values[i++] = UInt32GetDatum(ondisk.magic);
values[i++] = Int64GetDatum((int64) ondisk.checksum);
@@ -97,7 +140,7 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
Datum values[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
bool nulls[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
TupleDesc tupdesc;
- char path[MAXPGPATH];
+ XLogRecPtr lsn;
int i = 0;
text *filename_t = PG_GETARG_TEXT_PP(0);
@@ -105,12 +148,10 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
- sprintf(path, "%s/%s",
- PG_LOGICAL_SNAPSHOTS_DIR,
- text_to_cstring(filename_t));
+ lsn = parse_snapshot_filename(text_to_cstring(filename_t));
/* Validate and restore the snapshot to 'ondisk' */
- SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);
+ SnapBuildRestoreSnapshot(&ondisk, lsn, CurrentMemoryContext, false);
values[i++] = CStringGetTextDatum(get_snapbuild_state_desc(ondisk.builder.state));
values[i++] = TransactionIdGetDatum(ondisk.builder.xmin);
diff --git a/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql b/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql
new file mode 100644
index 00000000000..7d4951f30b0
--- /dev/null
+++ b/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql
@@ -0,0 +1,45 @@
+CREATE EXTENSION pg_logicalinspect;
+
+-- ===================================================================
+-- Tests for input validation
+-- ===================================================================
+
+SELECT pg_get_logical_snapshot_info('0-40796E18.foo');
+SELECT pg_get_logical_snapshot_info('0/40796E18.foo.snap');
+SELECT pg_get_logical_snapshot_info('0/40796E18.snap');
+SELECT pg_get_logical_snapshot_info('');
+SELECT pg_get_logical_snapshot_info(NULL);
+SELECT pg_get_logical_snapshot_info('../snapshots');
+
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo');
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo.snap');
+SELECT pg_get_logical_snapshot_meta('0/40796E18.snap');
+SELECT pg_get_logical_snapshot_meta('');
+SELECT pg_get_logical_snapshot_meta(NULL);
+SELECT pg_get_logical_snapshot_meta('../snapshots');
+
+-- ===================================================================
+-- Tests for permissions
+-- ===================================================================
+CREATE ROLE regress_pg_logicalinspect;
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- no
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- no
+
+-- Functions accessible by users with role pg_read_server_files.
+GRANT pg_read_server_files TO regress_pg_logicalinspect;
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- yes
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- yes
+
+-- ===================================================================
+-- Clean up
+-- ===================================================================
+
+DROP ROLE regress_pg_logicalinspect;
+
+DROP EXTENSION pg_logicalinspect;
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index bd0680dcbe5..b64e53de017 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1691,12 +1691,17 @@ out:
* If 'missing_ok' is true, will not throw an error if the file is not found.
*/
bool
-SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
+SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, XLogRecPtr lsn,
MemoryContext context, bool missing_ok)
{
int fd;
pg_crc32c checksum;
Size sz;
+ char path[MAXPGPATH];
+
+ sprintf(path, "%s/%X-%X.snap",
+ PG_LOGICAL_SNAPSHOTS_DIR,
+ LSN_FORMAT_ARGS(lsn));
fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
@@ -1788,18 +1793,13 @@ static bool
SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
{
SnapBuildOnDisk ondisk;
- char path[MAXPGPATH];
/* no point in loading a snapshot if we're already there */
if (builder->state == SNAPBUILD_CONSISTENT)
return false;
- sprintf(path, "%s/%X-%X.snap",
- PG_LOGICAL_SNAPSHOTS_DIR,
- LSN_FORMAT_ARGS(lsn));
-
/* validate and restore the snapshot to 'ondisk' */
- if (!SnapBuildRestoreSnapshot(&ondisk, path, builder->context, true))
+ if (!SnapBuildRestoreSnapshot(&ondisk, lsn, builder->context, true))
return false;
/*
diff --git a/src/include/replication/snapbuild_internal.h b/src/include/replication/snapbuild_internal.h
index 081b01b890a..3b915dc8793 100644
--- a/src/include/replication/snapbuild_internal.h
+++ b/src/include/replication/snapbuild_internal.h
@@ -193,7 +193,7 @@ typedef struct SnapBuildOnDisk
/* variable amount of TransactionIds follows */
} SnapBuildOnDisk;
-extern bool SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
+extern bool SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, XLogRecPtr lsn,
MemoryContext context, bool missing_ok);
#endif /* SNAPBUILD_INTERNAL_H */
--
2.34.1
* [PATCH v5] Fix bug in pg_logicalinspect functions
@ 2025-03-04 05:43 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 0 replies; 18+ messages in thread
From: Bertrand Drouvot @ 2025-03-04 05:43 UTC (permalink / raw)
The functions did not handle passed empty strings correctly and were not sanitizing
the request to prevent reference to files outside the snapshots directory.
Per bug #18828 from Robins Tharakan.
---
contrib/pg_logicalinspect/Makefile | 1 +
.../expected/pg_logicalinspect.out | 75 +++++++++++++++++++
contrib/pg_logicalinspect/pg_logicalinspect.c | 54 ++++++++++---
.../sql/pg_logicalinspect.sql | 45 +++++++++++
src/backend/replication/logical/snapbuild.c | 14 ++--
src/include/replication/snapbuild_internal.h | 2 +-
6 files changed, 173 insertions(+), 18 deletions(-)
42.3% contrib/pg_logicalinspect/expected/
28.6% contrib/pg_logicalinspect/sql/
22.3% contrib/pg_logicalinspect/
4.2% src/backend/replication/logical/
inline patch 1: contrib/pg_logicalinspect/Makefile, contrib/pg_logicalinspect/expected/pg_logicalinspect.out, contrib/pg_logicalinspect/pg_logicalinspect.c and 3 more (+173/-18 lines) download inline-1.patch
diff --git a/contrib/pg_logicalinspect/Makefile b/contrib/pg_logicalinspect/Makefile
index 55124514d42..17cee50d5c8 100644
--- a/contrib/pg_logicalinspect/Makefile
+++ b/contrib/pg_logicalinspect/Makefile
@@ -11,6 +11,7 @@ DATA = pg_logicalinspect--1.0.sql
EXTRA_INSTALL = contrib/test_decoding
+REGRESS = pg_logicalinspect
ISOLATION = logical_inspect
ISOLATION_OPTS = --temp-config $(top_srcdir)/contrib/pg_logicalinspect/logicalinspect.conf
diff --git a/contrib/pg_logicalinspect/expected/pg_logicalinspect.out b/contrib/pg_logicalinspect/expected/pg_logicalinspect.out
new file mode 100644
index 00000000000..f1f0712bec2
--- /dev/null
+++ b/contrib/pg_logicalinspect/expected/pg_logicalinspect.out
@@ -0,0 +1,75 @@
+CREATE EXTENSION pg_logicalinspect;
+-- ===================================================================
+-- Tests for input validation
+-- ===================================================================
+SELECT pg_get_logical_snapshot_info('0-40796E18.foo');
+ERROR: invalid snapshot file name "0-40796E18.foo"
+SELECT pg_get_logical_snapshot_info('0/40796E18.foo.snap');
+ERROR: invalid snapshot file name "0/40796E18.foo.snap"
+SELECT pg_get_logical_snapshot_info('0/40796E18.snap');
+ERROR: invalid snapshot file name "0/40796E18.snap"
+SELECT pg_get_logical_snapshot_info('');
+ERROR: invalid snapshot file name ""
+SELECT pg_get_logical_snapshot_info(NULL);
+ pg_get_logical_snapshot_info
+------------------------------
+
+(1 row)
+
+SELECT pg_get_logical_snapshot_info('../snapshots');
+ERROR: invalid snapshot file name "../snapshots"
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo');
+ERROR: invalid snapshot file name "0-40796E18.foo"
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo.snap');
+ERROR: invalid snapshot file name "0-40796E18.foo.snap"
+SELECT pg_get_logical_snapshot_meta('0/40796E18.snap');
+ERROR: invalid snapshot file name "0/40796E18.snap"
+SELECT pg_get_logical_snapshot_meta('');
+ERROR: invalid snapshot file name ""
+SELECT pg_get_logical_snapshot_meta(NULL);
+ pg_get_logical_snapshot_meta
+------------------------------
+
+(1 row)
+
+SELECT pg_get_logical_snapshot_meta('../snapshots');
+ERROR: invalid snapshot file name "../snapshots"
+-- ===================================================================
+-- Tests for permissions
+-- ===================================================================
+CREATE ROLE regress_pg_logicalinspect;
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- no
+ has_function_privilege
+------------------------
+ f
+(1 row)
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- no
+ has_function_privilege
+------------------------
+ f
+(1 row)
+
+-- Functions accessible by users with role pg_read_server_files.
+GRANT pg_read_server_files TO regress_pg_logicalinspect;
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- yes
+ has_function_privilege
+------------------------
+ t
+(1 row)
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- yes
+ has_function_privilege
+------------------------
+ t
+(1 row)
+
+-- ===================================================================
+-- Clean up
+-- ===================================================================
+DROP ROLE regress_pg_logicalinspect;
+DROP EXTENSION pg_logicalinspect;
diff --git a/contrib/pg_logicalinspect/pg_logicalinspect.c b/contrib/pg_logicalinspect/pg_logicalinspect.c
index cd575c6bd36..57bc622f06b 100644
--- a/contrib/pg_logicalinspect/pg_logicalinspect.c
+++ b/contrib/pg_logicalinspect/pg_logicalinspect.c
@@ -48,6 +48,44 @@ get_snapbuild_state_desc(SnapBuildState state)
return stateDesc;
}
+/*
+ * Extract the LSN from the given serialized snapshot file name.
+ */
+static XLogRecPtr
+parse_snapshot_filename(const char *filename)
+{
+ uint32 hi;
+ uint32 lo;
+ XLogRecPtr lsn;
+ bool report_error = false;
+
+ /* Extract the values to build the LSN */
+ if (sscanf(filename, "%X-%X", &hi, &lo) != 2)
+ report_error = true;
+
+ /* Ensure filename is %X-%X.snap */
+ if (!report_error)
+ {
+ size_t buffer_size = strlen(filename) + 1;
+ char *buffer = palloc(buffer_size);
+
+ snprintf(buffer, buffer_size, "%X-%X.snap", hi, lo);
+
+ if (strcmp(buffer, filename) != 0)
+ report_error = true;
+
+ pfree(buffer);
+ }
+
+ if (report_error)
+ ereport(ERROR,
+ errmsg("invalid snapshot file name \"%s\"", filename));
+
+ lsn = ((uint64) hi) << 32 | lo;
+
+ return lsn;
+}
+
/*
* Retrieve the logical snapshot file metadata.
*/
@@ -60,7 +98,7 @@ pg_get_logical_snapshot_meta(PG_FUNCTION_ARGS)
Datum values[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
TupleDesc tupdesc;
- char path[MAXPGPATH];
+ XLogRecPtr lsn;
int i = 0;
text *filename_t = PG_GETARG_TEXT_PP(0);
@@ -68,12 +106,10 @@ pg_get_logical_snapshot_meta(PG_FUNCTION_ARGS)
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
- sprintf(path, "%s/%s",
- PG_LOGICAL_SNAPSHOTS_DIR,
- text_to_cstring(filename_t));
+ lsn = parse_snapshot_filename(text_to_cstring(filename_t));
/* Validate and restore the snapshot to 'ondisk' */
- SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);
+ SnapBuildRestoreSnapshot(&ondisk, lsn, CurrentMemoryContext, false);
values[i++] = UInt32GetDatum(ondisk.magic);
values[i++] = Int64GetDatum((int64) ondisk.checksum);
@@ -97,7 +133,7 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
Datum values[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
bool nulls[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
TupleDesc tupdesc;
- char path[MAXPGPATH];
+ XLogRecPtr lsn;
int i = 0;
text *filename_t = PG_GETARG_TEXT_PP(0);
@@ -105,12 +141,10 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
- sprintf(path, "%s/%s",
- PG_LOGICAL_SNAPSHOTS_DIR,
- text_to_cstring(filename_t));
+ lsn = parse_snapshot_filename(text_to_cstring(filename_t));
/* Validate and restore the snapshot to 'ondisk' */
- SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);
+ SnapBuildRestoreSnapshot(&ondisk, lsn, CurrentMemoryContext, false);
values[i++] = CStringGetTextDatum(get_snapbuild_state_desc(ondisk.builder.state));
values[i++] = TransactionIdGetDatum(ondisk.builder.xmin);
diff --git a/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql b/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql
new file mode 100644
index 00000000000..7d4951f30b0
--- /dev/null
+++ b/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql
@@ -0,0 +1,45 @@
+CREATE EXTENSION pg_logicalinspect;
+
+-- ===================================================================
+-- Tests for input validation
+-- ===================================================================
+
+SELECT pg_get_logical_snapshot_info('0-40796E18.foo');
+SELECT pg_get_logical_snapshot_info('0/40796E18.foo.snap');
+SELECT pg_get_logical_snapshot_info('0/40796E18.snap');
+SELECT pg_get_logical_snapshot_info('');
+SELECT pg_get_logical_snapshot_info(NULL);
+SELECT pg_get_logical_snapshot_info('../snapshots');
+
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo');
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo.snap');
+SELECT pg_get_logical_snapshot_meta('0/40796E18.snap');
+SELECT pg_get_logical_snapshot_meta('');
+SELECT pg_get_logical_snapshot_meta(NULL);
+SELECT pg_get_logical_snapshot_meta('../snapshots');
+
+-- ===================================================================
+-- Tests for permissions
+-- ===================================================================
+CREATE ROLE regress_pg_logicalinspect;
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- no
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- no
+
+-- Functions accessible by users with role pg_read_server_files.
+GRANT pg_read_server_files TO regress_pg_logicalinspect;
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- yes
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- yes
+
+-- ===================================================================
+-- Clean up
+-- ===================================================================
+
+DROP ROLE regress_pg_logicalinspect;
+
+DROP EXTENSION pg_logicalinspect;
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index bd0680dcbe5..b64e53de017 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1691,12 +1691,17 @@ out:
* If 'missing_ok' is true, will not throw an error if the file is not found.
*/
bool
-SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
+SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, XLogRecPtr lsn,
MemoryContext context, bool missing_ok)
{
int fd;
pg_crc32c checksum;
Size sz;
+ char path[MAXPGPATH];
+
+ sprintf(path, "%s/%X-%X.snap",
+ PG_LOGICAL_SNAPSHOTS_DIR,
+ LSN_FORMAT_ARGS(lsn));
fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
@@ -1788,18 +1793,13 @@ static bool
SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
{
SnapBuildOnDisk ondisk;
- char path[MAXPGPATH];
/* no point in loading a snapshot if we're already there */
if (builder->state == SNAPBUILD_CONSISTENT)
return false;
- sprintf(path, "%s/%X-%X.snap",
- PG_LOGICAL_SNAPSHOTS_DIR,
- LSN_FORMAT_ARGS(lsn));
-
/* validate and restore the snapshot to 'ondisk' */
- if (!SnapBuildRestoreSnapshot(&ondisk, path, builder->context, true))
+ if (!SnapBuildRestoreSnapshot(&ondisk, lsn, builder->context, true))
return false;
/*
diff --git a/src/include/replication/snapbuild_internal.h b/src/include/replication/snapbuild_internal.h
index 081b01b890a..3b915dc8793 100644
--- a/src/include/replication/snapbuild_internal.h
+++ b/src/include/replication/snapbuild_internal.h
@@ -193,7 +193,7 @@ typedef struct SnapBuildOnDisk
/* variable amount of TransactionIds follows */
} SnapBuildOnDisk;
-extern bool SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
+extern bool SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, XLogRecPtr lsn,
MemoryContext context, bool missing_ok);
#endif /* SNAPBUILD_INTERNAL_H */
--
2.34.1
* [PATCH v6] Fix bug in pg_logicalinspect functions
@ 2025-03-04 05:43 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 0 replies; 18+ messages in thread
From: Bertrand Drouvot @ 2025-03-04 05:43 UTC (permalink / raw)
The functions did not handle passed empty strings correctly and were not sanitizing
the request to prevent reference to files outside the snapshots directory.
Per bug #18828 from Robins Tharakan.
---
contrib/pg_logicalinspect/Makefile | 1 +
.../expected/pg_logicalinspect.out | 81 +++++++++++++++++++
contrib/pg_logicalinspect/pg_logicalinspect.c | 56 ++++++++++---
.../sql/pg_logicalinspect.sql | 48 +++++++++++
src/backend/replication/logical/snapbuild.c | 14 ++--
src/include/replication/snapbuild_internal.h | 2 +-
6 files changed, 184 insertions(+), 18 deletions(-)
43.0% contrib/pg_logicalinspect/expected/
28.3% contrib/pg_logicalinspect/sql/
22.5% contrib/pg_logicalinspect/
3.8% src/backend/replication/logical/
inline patch 1: contrib/pg_logicalinspect/Makefile, contrib/pg_logicalinspect/expected/pg_logicalinspect.out, contrib/pg_logicalinspect/pg_logicalinspect.c and 3 more (+184/-18 lines) download inline-1.patch
diff --git a/contrib/pg_logicalinspect/Makefile b/contrib/pg_logicalinspect/Makefile
index 55124514d42..17cee50d5c8 100644
--- a/contrib/pg_logicalinspect/Makefile
+++ b/contrib/pg_logicalinspect/Makefile
@@ -11,6 +11,7 @@ DATA = pg_logicalinspect--1.0.sql
EXTRA_INSTALL = contrib/test_decoding
+REGRESS = pg_logicalinspect
ISOLATION = logical_inspect
ISOLATION_OPTS = --temp-config $(top_srcdir)/contrib/pg_logicalinspect/logicalinspect.conf
diff --git a/contrib/pg_logicalinspect/expected/pg_logicalinspect.out b/contrib/pg_logicalinspect/expected/pg_logicalinspect.out
new file mode 100644
index 00000000000..f6c9b625d6b
--- /dev/null
+++ b/contrib/pg_logicalinspect/expected/pg_logicalinspect.out
@@ -0,0 +1,81 @@
+CREATE EXTENSION pg_logicalinspect;
+-- ===================================================================
+-- Tests for input validation
+-- ===================================================================
+SELECT pg_get_logical_snapshot_info('0-40796E18.foo');
+ERROR: invalid snapshot file name "0-40796E18.foo"
+SELECT pg_get_logical_snapshot_info('0--40796E18.snap');
+ERROR: invalid snapshot file name "0--40796E18.snap"
+SELECT pg_get_logical_snapshot_info('-1--40796E18.snap');
+ERROR: invalid snapshot file name "-1--40796E18.snap"
+SELECT pg_get_logical_snapshot_info('0/40796E18.foo.snap');
+ERROR: invalid snapshot file name "0/40796E18.foo.snap"
+SELECT pg_get_logical_snapshot_info('0/40796E18.snap');
+ERROR: invalid snapshot file name "0/40796E18.snap"
+SELECT pg_get_logical_snapshot_info('');
+ERROR: invalid snapshot file name ""
+SELECT pg_get_logical_snapshot_info(NULL);
+ pg_get_logical_snapshot_info
+------------------------------
+
+(1 row)
+
+SELECT pg_get_logical_snapshot_info('../snapshots');
+ERROR: invalid snapshot file name "../snapshots"
+SELECT pg_get_logical_snapshot_info('../snapshots/0-40796E18.snap');
+ERROR: invalid snapshot file name "../snapshots/0-40796E18.snap"
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo');
+ERROR: invalid snapshot file name "0-40796E18.foo"
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo.snap');
+ERROR: invalid snapshot file name "0-40796E18.foo.snap"
+SELECT pg_get_logical_snapshot_meta('0/40796E18.snap');
+ERROR: invalid snapshot file name "0/40796E18.snap"
+SELECT pg_get_logical_snapshot_meta('');
+ERROR: invalid snapshot file name ""
+SELECT pg_get_logical_snapshot_meta(NULL);
+ pg_get_logical_snapshot_meta
+------------------------------
+
+(1 row)
+
+SELECT pg_get_logical_snapshot_meta('../snapshots');
+ERROR: invalid snapshot file name "../snapshots"
+-- ===================================================================
+-- Tests for permissions
+-- ===================================================================
+CREATE ROLE regress_pg_logicalinspect;
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- no
+ has_function_privilege
+------------------------
+ f
+(1 row)
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- no
+ has_function_privilege
+------------------------
+ f
+(1 row)
+
+-- Functions accessible by users with role pg_read_server_files.
+GRANT pg_read_server_files TO regress_pg_logicalinspect;
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- yes
+ has_function_privilege
+------------------------
+ t
+(1 row)
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- yes
+ has_function_privilege
+------------------------
+ t
+(1 row)
+
+-- ===================================================================
+-- Clean up
+-- ===================================================================
+DROP ROLE regress_pg_logicalinspect;
+DROP EXTENSION pg_logicalinspect;
diff --git a/contrib/pg_logicalinspect/pg_logicalinspect.c b/contrib/pg_logicalinspect/pg_logicalinspect.c
index cd575c6bd36..7963e52f9d1 100644
--- a/contrib/pg_logicalinspect/pg_logicalinspect.c
+++ b/contrib/pg_logicalinspect/pg_logicalinspect.c
@@ -48,6 +48,46 @@ get_snapbuild_state_desc(SnapBuildState state)
return stateDesc;
}
+/*
+ * Extract the LSN from the given serialized snapshot file name.
+ */
+static XLogRecPtr
+parse_snapshot_filename(char *filename)
+{
+ uint32 hi;
+ uint32 lo;
+ XLogRecPtr lsn;
+ char tmpfname[MAXPGPATH];
+
+ /*
+ * Extract the values to build the LSN.
+ *
+ * Note: We deliberately don't use "%X-%X.snap" because sscanf only counts
+ * converted values (%X), not literal text matches.
+ */
+ if (sscanf(filename, "%X-%X", &hi, &lo) != 2)
+ goto parse_error;
+
+ /*
+ * Bring back the LSN to the snapshot file format and compare it to the
+ * given name to see if the extracted LSN is sane.
+ *
+ * This also ensures there are no extra characters between the LSN values
+ * and the .snap extension.
+ */
+ sprintf(tmpfname, "%X-%X.snap", hi, lo);
+ if (strcmp(tmpfname, filename) != 0)
+ goto parse_error;
+
+ lsn = ((uint64) hi) << 32 | lo;
+
+ return lsn;
+
+parse_error:
+ ereport(ERROR,
+ errmsg("invalid snapshot file name \"%s\"", filename));
+}
+
/*
* Retrieve the logical snapshot file metadata.
*/
@@ -60,7 +100,7 @@ pg_get_logical_snapshot_meta(PG_FUNCTION_ARGS)
Datum values[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
TupleDesc tupdesc;
- char path[MAXPGPATH];
+ XLogRecPtr lsn;
int i = 0;
text *filename_t = PG_GETARG_TEXT_PP(0);
@@ -68,12 +108,10 @@ pg_get_logical_snapshot_meta(PG_FUNCTION_ARGS)
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
- sprintf(path, "%s/%s",
- PG_LOGICAL_SNAPSHOTS_DIR,
- text_to_cstring(filename_t));
+ lsn = parse_snapshot_filename(text_to_cstring(filename_t));
/* Validate and restore the snapshot to 'ondisk' */
- SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);
+ SnapBuildRestoreSnapshot(&ondisk, lsn, CurrentMemoryContext, false);
values[i++] = UInt32GetDatum(ondisk.magic);
values[i++] = Int64GetDatum((int64) ondisk.checksum);
@@ -97,7 +135,7 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
Datum values[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
bool nulls[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
TupleDesc tupdesc;
- char path[MAXPGPATH];
+ XLogRecPtr lsn;
int i = 0;
text *filename_t = PG_GETARG_TEXT_PP(0);
@@ -105,12 +143,10 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
- sprintf(path, "%s/%s",
- PG_LOGICAL_SNAPSHOTS_DIR,
- text_to_cstring(filename_t));
+ lsn = parse_snapshot_filename(text_to_cstring(filename_t));
/* Validate and restore the snapshot to 'ondisk' */
- SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);
+ SnapBuildRestoreSnapshot(&ondisk, lsn, CurrentMemoryContext, false);
values[i++] = CStringGetTextDatum(get_snapbuild_state_desc(ondisk.builder.state));
values[i++] = TransactionIdGetDatum(ondisk.builder.xmin);
diff --git a/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql b/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql
new file mode 100644
index 00000000000..143cf45dc12
--- /dev/null
+++ b/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql
@@ -0,0 +1,48 @@
+CREATE EXTENSION pg_logicalinspect;
+
+-- ===================================================================
+-- Tests for input validation
+-- ===================================================================
+
+SELECT pg_get_logical_snapshot_info('0-40796E18.foo');
+SELECT pg_get_logical_snapshot_info('0--40796E18.snap');
+SELECT pg_get_logical_snapshot_info('-1--40796E18.snap');
+SELECT pg_get_logical_snapshot_info('0/40796E18.foo.snap');
+SELECT pg_get_logical_snapshot_info('0/40796E18.snap');
+SELECT pg_get_logical_snapshot_info('');
+SELECT pg_get_logical_snapshot_info(NULL);
+SELECT pg_get_logical_snapshot_info('../snapshots');
+SELECT pg_get_logical_snapshot_info('../snapshots/0-40796E18.snap');
+
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo');
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo.snap');
+SELECT pg_get_logical_snapshot_meta('0/40796E18.snap');
+SELECT pg_get_logical_snapshot_meta('');
+SELECT pg_get_logical_snapshot_meta(NULL);
+SELECT pg_get_logical_snapshot_meta('../snapshots');
+
+-- ===================================================================
+-- Tests for permissions
+-- ===================================================================
+CREATE ROLE regress_pg_logicalinspect;
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- no
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- no
+
+-- Functions accessible by users with role pg_read_server_files.
+GRANT pg_read_server_files TO regress_pg_logicalinspect;
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- yes
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- yes
+
+-- ===================================================================
+-- Clean up
+-- ===================================================================
+
+DROP ROLE regress_pg_logicalinspect;
+
+DROP EXTENSION pg_logicalinspect;
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index bd0680dcbe5..b64e53de017 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1691,12 +1691,17 @@ out:
* If 'missing_ok' is true, will not throw an error if the file is not found.
*/
bool
-SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
+SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, XLogRecPtr lsn,
MemoryContext context, bool missing_ok)
{
int fd;
pg_crc32c checksum;
Size sz;
+ char path[MAXPGPATH];
+
+ sprintf(path, "%s/%X-%X.snap",
+ PG_LOGICAL_SNAPSHOTS_DIR,
+ LSN_FORMAT_ARGS(lsn));
fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
@@ -1788,18 +1793,13 @@ static bool
SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
{
SnapBuildOnDisk ondisk;
- char path[MAXPGPATH];
/* no point in loading a snapshot if we're already there */
if (builder->state == SNAPBUILD_CONSISTENT)
return false;
- sprintf(path, "%s/%X-%X.snap",
- PG_LOGICAL_SNAPSHOTS_DIR,
- LSN_FORMAT_ARGS(lsn));
-
/* validate and restore the snapshot to 'ondisk' */
- if (!SnapBuildRestoreSnapshot(&ondisk, path, builder->context, true))
+ if (!SnapBuildRestoreSnapshot(&ondisk, lsn, builder->context, true))
return false;
/*
diff --git a/src/include/replication/snapbuild_internal.h b/src/include/replication/snapbuild_internal.h
index 081b01b890a..3b915dc8793 100644
--- a/src/include/replication/snapbuild_internal.h
+++ b/src/include/replication/snapbuild_internal.h
@@ -193,7 +193,7 @@ typedef struct SnapBuildOnDisk
/* variable amount of TransactionIds follows */
} SnapBuildOnDisk;
-extern bool SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
+extern bool SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, XLogRecPtr lsn,
MemoryContext context, bool missing_ok);
#endif /* SNAPBUILD_INTERNAL_H */
--
2.34.1
* [PATCH v7] Fix bug in pg_logicalinspect functions
@ 2025-03-04 05:43 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 0 replies; 18+ messages in thread
From: Bertrand Drouvot @ 2025-03-04 05:43 UTC (permalink / raw)
The functions did not handle passed empty strings correctly and were not sanitizing
the request to prevent reference to files outside the snapshots directory.
Per bug #18828 from Robins Tharakan.
---
contrib/pg_logicalinspect/Makefile | 1 +
.../expected/pg_logicalinspect.out | 81 +++++++++++++++++++
contrib/pg_logicalinspect/pg_logicalinspect.c | 55 ++++++++++---
.../sql/pg_logicalinspect.sql | 48 +++++++++++
src/backend/replication/logical/snapbuild.c | 14 ++--
src/include/replication/snapbuild_internal.h | 2 +-
6 files changed, 183 insertions(+), 18 deletions(-)
42.6% contrib/pg_logicalinspect/expected/
28.1% contrib/pg_logicalinspect/sql/
23.2% contrib/pg_logicalinspect/
3.7% src/backend/replication/logical/
inline patch 1: contrib/pg_logicalinspect/Makefile, contrib/pg_logicalinspect/expected/pg_logicalinspect.out, contrib/pg_logicalinspect/pg_logicalinspect.c and 3 more (+183/-18 lines) download inline-1.patch
diff --git a/contrib/pg_logicalinspect/Makefile b/contrib/pg_logicalinspect/Makefile
index 55124514d42..17cee50d5c8 100644
--- a/contrib/pg_logicalinspect/Makefile
+++ b/contrib/pg_logicalinspect/Makefile
@@ -11,6 +11,7 @@ DATA = pg_logicalinspect--1.0.sql
EXTRA_INSTALL = contrib/test_decoding
+REGRESS = pg_logicalinspect
ISOLATION = logical_inspect
ISOLATION_OPTS = --temp-config $(top_srcdir)/contrib/pg_logicalinspect/logicalinspect.conf
diff --git a/contrib/pg_logicalinspect/expected/pg_logicalinspect.out b/contrib/pg_logicalinspect/expected/pg_logicalinspect.out
new file mode 100644
index 00000000000..f6c9b625d6b
--- /dev/null
+++ b/contrib/pg_logicalinspect/expected/pg_logicalinspect.out
@@ -0,0 +1,81 @@
+CREATE EXTENSION pg_logicalinspect;
+-- ===================================================================
+-- Tests for input validation
+-- ===================================================================
+SELECT pg_get_logical_snapshot_info('0-40796E18.foo');
+ERROR: invalid snapshot file name "0-40796E18.foo"
+SELECT pg_get_logical_snapshot_info('0--40796E18.snap');
+ERROR: invalid snapshot file name "0--40796E18.snap"
+SELECT pg_get_logical_snapshot_info('-1--40796E18.snap');
+ERROR: invalid snapshot file name "-1--40796E18.snap"
+SELECT pg_get_logical_snapshot_info('0/40796E18.foo.snap');
+ERROR: invalid snapshot file name "0/40796E18.foo.snap"
+SELECT pg_get_logical_snapshot_info('0/40796E18.snap');
+ERROR: invalid snapshot file name "0/40796E18.snap"
+SELECT pg_get_logical_snapshot_info('');
+ERROR: invalid snapshot file name ""
+SELECT pg_get_logical_snapshot_info(NULL);
+ pg_get_logical_snapshot_info
+------------------------------
+
+(1 row)
+
+SELECT pg_get_logical_snapshot_info('../snapshots');
+ERROR: invalid snapshot file name "../snapshots"
+SELECT pg_get_logical_snapshot_info('../snapshots/0-40796E18.snap');
+ERROR: invalid snapshot file name "../snapshots/0-40796E18.snap"
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo');
+ERROR: invalid snapshot file name "0-40796E18.foo"
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo.snap');
+ERROR: invalid snapshot file name "0-40796E18.foo.snap"
+SELECT pg_get_logical_snapshot_meta('0/40796E18.snap');
+ERROR: invalid snapshot file name "0/40796E18.snap"
+SELECT pg_get_logical_snapshot_meta('');
+ERROR: invalid snapshot file name ""
+SELECT pg_get_logical_snapshot_meta(NULL);
+ pg_get_logical_snapshot_meta
+------------------------------
+
+(1 row)
+
+SELECT pg_get_logical_snapshot_meta('../snapshots');
+ERROR: invalid snapshot file name "../snapshots"
+-- ===================================================================
+-- Tests for permissions
+-- ===================================================================
+CREATE ROLE regress_pg_logicalinspect;
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- no
+ has_function_privilege
+------------------------
+ f
+(1 row)
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- no
+ has_function_privilege
+------------------------
+ f
+(1 row)
+
+-- Functions accessible by users with role pg_read_server_files.
+GRANT pg_read_server_files TO regress_pg_logicalinspect;
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- yes
+ has_function_privilege
+------------------------
+ t
+(1 row)
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- yes
+ has_function_privilege
+------------------------
+ t
+(1 row)
+
+-- ===================================================================
+-- Clean up
+-- ===================================================================
+DROP ROLE regress_pg_logicalinspect;
+DROP EXTENSION pg_logicalinspect;
diff --git a/contrib/pg_logicalinspect/pg_logicalinspect.c b/contrib/pg_logicalinspect/pg_logicalinspect.c
index cd575c6bd36..29f7f296653 100644
--- a/contrib/pg_logicalinspect/pg_logicalinspect.c
+++ b/contrib/pg_logicalinspect/pg_logicalinspect.c
@@ -48,6 +48,45 @@ get_snapbuild_state_desc(SnapBuildState state)
return stateDesc;
}
+/*
+ * Extract the LSN from the given serialized snapshot file name.
+ */
+static XLogRecPtr
+parse_snapshot_filename(const char *filename)
+{
+ uint32 hi;
+ uint32 lo;
+ XLogRecPtr lsn;
+ char tmpfname[MAXPGPATH];
+
+ /*
+ * Extract the values to build the LSN.
+ *
+ * Note: Including ".snap" documents the expected format, but be aware
+ * that sscanf only counts converted values (%X) in its return value, not
+ * literal text matches (meaning that ".snap" has no effect here).
+ */
+ if (sscanf(filename, "%X-%X.snap", &hi, &lo) != 2)
+ goto parse_error;
+
+ /*
+ * Bring back the extracted LSN to the snapshot file format and compare it
+ * to the given filename. This check strictly checks if the given filename
+ * follows the format of the snapshot filename.
+ */
+ sprintf(tmpfname, "%X-%X.snap", hi, lo);
+ if (strcmp(tmpfname, filename) != 0)
+ goto parse_error;
+
+ lsn = ((uint64) hi) << 32 | lo;
+
+ return lsn;
+
+parse_error:
+ ereport(ERROR,
+ errmsg("invalid snapshot file name \"%s\"", filename));
+}
+
/*
* Retrieve the logical snapshot file metadata.
*/
@@ -60,7 +99,7 @@ pg_get_logical_snapshot_meta(PG_FUNCTION_ARGS)
Datum values[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
TupleDesc tupdesc;
- char path[MAXPGPATH];
+ XLogRecPtr lsn;
int i = 0;
text *filename_t = PG_GETARG_TEXT_PP(0);
@@ -68,12 +107,10 @@ pg_get_logical_snapshot_meta(PG_FUNCTION_ARGS)
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
- sprintf(path, "%s/%s",
- PG_LOGICAL_SNAPSHOTS_DIR,
- text_to_cstring(filename_t));
+ lsn = parse_snapshot_filename(text_to_cstring(filename_t));
/* Validate and restore the snapshot to 'ondisk' */
- SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);
+ SnapBuildRestoreSnapshot(&ondisk, lsn, CurrentMemoryContext, false);
values[i++] = UInt32GetDatum(ondisk.magic);
values[i++] = Int64GetDatum((int64) ondisk.checksum);
@@ -97,7 +134,7 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
Datum values[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
bool nulls[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
TupleDesc tupdesc;
- char path[MAXPGPATH];
+ XLogRecPtr lsn;
int i = 0;
text *filename_t = PG_GETARG_TEXT_PP(0);
@@ -105,12 +142,10 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
- sprintf(path, "%s/%s",
- PG_LOGICAL_SNAPSHOTS_DIR,
- text_to_cstring(filename_t));
+ lsn = parse_snapshot_filename(text_to_cstring(filename_t));
/* Validate and restore the snapshot to 'ondisk' */
- SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);
+ SnapBuildRestoreSnapshot(&ondisk, lsn, CurrentMemoryContext, false);
values[i++] = CStringGetTextDatum(get_snapbuild_state_desc(ondisk.builder.state));
values[i++] = TransactionIdGetDatum(ondisk.builder.xmin);
diff --git a/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql b/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql
new file mode 100644
index 00000000000..143cf45dc12
--- /dev/null
+++ b/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql
@@ -0,0 +1,48 @@
+CREATE EXTENSION pg_logicalinspect;
+
+-- ===================================================================
+-- Tests for input validation
+-- ===================================================================
+
+SELECT pg_get_logical_snapshot_info('0-40796E18.foo');
+SELECT pg_get_logical_snapshot_info('0--40796E18.snap');
+SELECT pg_get_logical_snapshot_info('-1--40796E18.snap');
+SELECT pg_get_logical_snapshot_info('0/40796E18.foo.snap');
+SELECT pg_get_logical_snapshot_info('0/40796E18.snap');
+SELECT pg_get_logical_snapshot_info('');
+SELECT pg_get_logical_snapshot_info(NULL);
+SELECT pg_get_logical_snapshot_info('../snapshots');
+SELECT pg_get_logical_snapshot_info('../snapshots/0-40796E18.snap');
+
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo');
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo.snap');
+SELECT pg_get_logical_snapshot_meta('0/40796E18.snap');
+SELECT pg_get_logical_snapshot_meta('');
+SELECT pg_get_logical_snapshot_meta(NULL);
+SELECT pg_get_logical_snapshot_meta('../snapshots');
+
+-- ===================================================================
+-- Tests for permissions
+-- ===================================================================
+CREATE ROLE regress_pg_logicalinspect;
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- no
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- no
+
+-- Functions accessible by users with role pg_read_server_files.
+GRANT pg_read_server_files TO regress_pg_logicalinspect;
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- yes
+SELECT has_function_privilege('regress_pg_logicalinspect',
+ 'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- yes
+
+-- ===================================================================
+-- Clean up
+-- ===================================================================
+
+DROP ROLE regress_pg_logicalinspect;
+
+DROP EXTENSION pg_logicalinspect;
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index bd0680dcbe5..b64e53de017 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1691,12 +1691,17 @@ out:
* If 'missing_ok' is true, will not throw an error if the file is not found.
*/
bool
-SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
+SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, XLogRecPtr lsn,
MemoryContext context, bool missing_ok)
{
int fd;
pg_crc32c checksum;
Size sz;
+ char path[MAXPGPATH];
+
+ sprintf(path, "%s/%X-%X.snap",
+ PG_LOGICAL_SNAPSHOTS_DIR,
+ LSN_FORMAT_ARGS(lsn));
fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
@@ -1788,18 +1793,13 @@ static bool
SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
{
SnapBuildOnDisk ondisk;
- char path[MAXPGPATH];
/* no point in loading a snapshot if we're already there */
if (builder->state == SNAPBUILD_CONSISTENT)
return false;
- sprintf(path, "%s/%X-%X.snap",
- PG_LOGICAL_SNAPSHOTS_DIR,
- LSN_FORMAT_ARGS(lsn));
-
/* validate and restore the snapshot to 'ondisk' */
- if (!SnapBuildRestoreSnapshot(&ondisk, path, builder->context, true))
+ if (!SnapBuildRestoreSnapshot(&ondisk, lsn, builder->context, true))
return false;
/*
diff --git a/src/include/replication/snapbuild_internal.h b/src/include/replication/snapbuild_internal.h
index 081b01b890a..3b915dc8793 100644
--- a/src/include/replication/snapbuild_internal.h
+++ b/src/include/replication/snapbuild_internal.h
@@ -193,7 +193,7 @@ typedef struct SnapBuildOnDisk
/* variable amount of TransactionIds follows */
} SnapBuildOnDisk;
-extern bool SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
+extern bool SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, XLogRecPtr lsn,
MemoryContext context, bool missing_ok);
#endif /* SNAPBUILD_INTERNAL_H */
--
2.34.1
end of thread, other threads:[~2025-03-04 05:43 UTC | newest]
Thread overview: 18+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-07-03 08:03 BUG #15888: Bogus "idle in transaction" state for logical decoding client after creating a slot PG Bug reporting form <noreply@postgresql.org>
2019-07-04 04:35 ` Michael Paquier <michael@paquier.xyz>
2019-07-09 20:15 ` Andres Freund <andres@anarazel.de>
2019-07-09 21:46 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-07-10 04:51 ` Michael Paquier <michael@paquier.xyz>
2019-07-10 07:55 ` Andres Freund <andres@anarazel.de>
2019-07-10 15:24 ` Magnus Hagander <magnus@hagander.net>
2019-07-10 23:58 ` Michael Paquier <michael@paquier.xyz>
2019-07-11 11:21 ` Magnus Hagander <magnus@hagander.net>
2019-07-11 13:09 ` Michael Paquier <michael@paquier.xyz>
2019-07-11 13:44 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-07-12 01:25 ` Michael Paquier <michael@paquier.xyz>
2025-03-04 05:43 [PATCH v2 1/2] Fix bug in pg_logicalinspect functions Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2025-03-04 05:43 [PATCH v3 1/2] Fix bug in pg_logicalinspect functions Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2025-03-04 05:43 [PATCH v4] Fix bug in pg_logicalinspect functions Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2025-03-04 05:43 [PATCH v5] Fix bug in pg_logicalinspect functions Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2025-03-04 05:43 [PATCH v6] Fix bug in pg_logicalinspect functions Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2025-03-04 05:43 [PATCH v7] Fix bug in pg_logicalinspect functions Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox