database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-06-16 06:01:02 +03:00
Code Activity

pg_logicalinspect: Fix possible crash when passing a directory path.

Browse Source 
Previously, pg_logicalinspect functions were too trusting of their
input and blindly passed it to SnapBuildRestoreSnapshot(). If the
input pointed to a directory, the server could a PANIC error while
attempting to fsync_fname() with isdir=false on a directory.

This commit adds validation checks for input filenames and passes the
LSN extracted from the filename to SnapBuildRestoreSnapshot() instead
of the filename itself. It also adds regression tests for various
input patterns and permission checks.

Bug: #18828
Reported-by: Robins Tharakan <tharakan@gmail.com>
Co-authored-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Co-authored-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/18828-0f4701c635064211@postgresql.org
This commit is contained in:
Masahiko Sawada
2025-03-11 09:56:40 -07:00
parent a49927f04c
commit bd65cb3cd4
6 changed files with 183 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
contrib/pg_logicalinspect/Makefile
View File

@ -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

81
contrib/pg_logicalinspect/expected/pg_logicalinspect.out Normal file
View File

@ -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;

55
contrib/pg_logicalinspect/pg_logicalinspect.c
View File

@ -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" doesn't mean that sscanf() also does the format
* check including the suffix. The subsequent check validates if the given
* filename has the expected suffix.
*/
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);

48
contrib/pg_logicalinspect/sql/pg_logicalinspect.sql Normal file
View File

@ -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;

14
src/backend/replication/logical/snapbuild.c
View File

@ -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;
/*

2
src/include/replication/snapbuild_internal.h
View File

@ -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 */