[PATCH v2 1/2] Fix bug in pg_logicalinspect functions
Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To:
Date: 2025-03-04T05:43:15Z
Lists: pgsql-bugs
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/
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
--7V3m2fno5VtGG8jS
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v2-0002-Remove-obsolete-comment-in-SnapBuildRestoreSnapsh.patch"