add some more error checks into _fileExistsInDirectory function to report "too long name" error
Hi,
In another thread[1]/messages/by-id/202504110938.4kx73ylnv6p4@alvherre.pgsql, Álvaro gave some feedback for _fileExistsInDirectory
function for "too long name" error.
Basically, in _fileExistsInDirectory function, we pass dirname and filename
but we were checking only the combined length of these two names.
Here, I am attaching a patch which will check lengths of dirname and
filename separately and will report errors if the name is too long.
I added a check in some other parts also to report an error for "too long
name".
Please review the attached patch and let me know feedback.
[1]: /messages/by-id/202504110938.4kx73ylnv6p4@alvherre.pgsql
/messages/by-id/202504110938.4kx73ylnv6p4@alvherre.pgsql
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v01-add-some-more-error-checks-into-_fileExistsInDirecto.patchapplication/octet-stream; name=v01-add-some-more-error-checks-into-_fileExistsInDirecto.patchDownload
From c508fb100757e2968247865eaa0a678dd5afb146 Mon Sep 17 00:00:00 2001
From: Mahendra Singh Thalor <mahi6run@gmail.com>
Date: Fri, 11 Apr 2025 18:31:06 +0530
Subject: [PATCH] add some more error checks into _fileExistsInDirectory
function to report "too long name" error.
---
src/bin/pg_dump/pg_backup_archiver.c | 9 +++++++-
src/bin/pg_dump/pg_dumpall.c | 22 ++++++++++++++----
src/bin/pg_dump/pg_restore.c | 34 ++++++++++++++++++++++------
3 files changed, 53 insertions(+), 12 deletions(-)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index f961162f365..30dcf5780a3 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2218,9 +2218,16 @@ _fileExistsInDirectory(const char *dir, const char *filename)
struct stat st;
char buf[MAXPGPATH];
- if (snprintf(buf, MAXPGPATH, "%s/%s", dir, filename) >= MAXPGPATH)
+ if (strlen(dir) >= MAXPGPATH)
pg_fatal("directory name too long: \"%s\"", dir);
+ if (strlen(filename) >= MAXPGPATH)
+ pg_fatal("file name too long: \"%s\"", filename);
+
+ /* Now check path length of dir/filename */
+ if (snprintf(buf, MAXPGPATH, "%s/%s", dir, filename) >= MAXPGPATH)
+ pg_fatal("combined name of directory:\"%s\" and file:\"%s\" is too long", filename, dir);
+
return (stat(buf, &st) == 0 && S_ISREG(st.st_mode));
}
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 3395d559518..f4a9bb07503 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -506,6 +506,9 @@ main(int argc, char *argv[])
if (statistics_only)
appendPQExpBufferStr(pgdumpopts, " --statistics-only");
+ if (filename && strlen(filename) >= MAXPGPATH)
+ pg_fatal("out file name:\"%s\" is too long", filename);
+
/*
* Open the output file if required, otherwise use stdout. If required,
* then create new directory and global.dat file.
@@ -517,7 +520,8 @@ main(int argc, char *argv[])
/* Create new directory or accept the empty existing directory. */
create_or_open_dir(filename);
- snprintf(global_path, MAXPGPATH, "%s/global.dat", filename);
+ if (snprintf(global_path, MAXPGPATH, "%s/global.dat", filename) >= MAXPGPATH)
+ pg_fatal("combined name of out file:\"%s\" and global.dat is too long", filename);
OPF = fopen(global_path, PG_BINARY_W);
if (!OPF)
@@ -1650,6 +1654,7 @@ dumpDatabases(PGconn *conn, ArchiveFormat archDumpFormat)
{
char map_file_path[MAXPGPATH];
+ /* No need to check path length as %s/global.dat was OK. */
snprintf(db_subdir, MAXPGPATH, "%s/databases", filename);
/* Create a subdirectory with 'databases' name under main directory. */
@@ -1689,11 +1694,20 @@ dumpDatabases(PGconn *conn, ArchiveFormat archDumpFormat)
if (archDumpFormat != archNull)
{
if (archDumpFormat == archCustom)
- snprintf(dbfilepath, MAXPGPATH, "\"%s\"/\"%s\".dmp", db_subdir, oid);
+ {
+ if (snprintf(dbfilepath, MAXPGPATH, "\"%s\"/\"%s\".dmp", db_subdir, oid) >= MAXPGPATH)
+ pg_fatal("combined name of file:\"%s\" and \"%s\".dmp is too long", db_subdir, oid);
+ }
else if (archDumpFormat == archTar)
- snprintf(dbfilepath, MAXPGPATH, "\"%s\"/\"%s\".tar", db_subdir, oid);
+ {
+ if (snprintf(dbfilepath, MAXPGPATH, "\"%s\"/\"%s\".tar", db_subdir, oid) >= MAXPGPATH)
+ pg_fatal("combined name of file:\"%s\" and \"%s\".tar is too long", db_subdir, oid);
+ }
else
- snprintf(dbfilepath, MAXPGPATH, "\"%s\"/\"%s\"", db_subdir, oid);
+ {
+ if (snprintf(dbfilepath, MAXPGPATH, "\"%s\"/\"%s\"", db_subdir, oid) >= MAXPGPATH)
+ pg_fatal("combined name of file:\"%s\" and \"%s\" is too long", db_subdir, oid);
+ }
/* Put one line entry for dboid and dbname in map file. */
fprintf(map_file, "%s %s\n", oid, dbname);
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 24a44b81a95..157f7ac8c29 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -834,9 +834,16 @@ file_exists_in_directory(const char *dir, const char *filename)
struct stat st;
char buf[MAXPGPATH];
- if (snprintf(buf, MAXPGPATH, "%s/%s", dir, filename) >= MAXPGPATH)
+ if (strlen(dir) >= MAXPGPATH)
pg_fatal("directory name too long: \"%s\"", dir);
+ if (strlen(filename) >= MAXPGPATH)
+ pg_fatal("file name too long: \"%s\"", filename);
+
+ /* Now check path length of dir/filename */
+ if (snprintf(buf, MAXPGPATH, "%s/%s", dir, filename) >= MAXPGPATH)
+ pg_fatal("combined name of directory:\"%s\" and file:\"%s\" is too long", filename, dir);
+
return (stat(buf, &st) == 0 && S_ISREG(st.st_mode));
}
@@ -1190,6 +1197,10 @@ restore_all_databases(PGconn *conn, const char *dumpdirpath,
opts->cparams.override_dbname = NULL;
}
+ /*
+ * No need to check dumpdirpath/databases path as
+ * dumpdirpath/global.dat was OK.
+ */
snprintf(subdirdbpath, MAXPGPATH, "%s/databases", dumpdirpath);
/*
@@ -1197,17 +1208,23 @@ restore_all_databases(PGconn *conn, const char *dumpdirpath,
* {oid}.dmp file, use it. Otherwise try to use a directory called
* {oid}
*/
- snprintf(dbfilename, MAXPGPATH, "%u.tar", db_cell->oid);
+ snprintf(dbfilename, MAXPGPATH, "%u", db_cell->oid); /* path length is OK */
+
if (file_exists_in_directory(subdirdbpath, dbfilename))
- snprintf(subdirpath, MAXPGPATH, "%s/databases/%u.tar", dumpdirpath, db_cell->oid);
+ snprintf(subdirpath, MAXPGPATH, "%s/databases/%s", dumpdirpath, dbfilename);
else
{
- snprintf(dbfilename, MAXPGPATH, "%u.dmp", db_cell->oid);
-
+ snprintf(dbfilename, MAXPGPATH, "%u.tar", db_cell->oid);
if (file_exists_in_directory(subdirdbpath, dbfilename))
- snprintf(subdirpath, MAXPGPATH, "%s/databases/%u.dmp", dumpdirpath, db_cell->oid);
+ snprintf(subdirpath, MAXPGPATH, "%s/databases/%u.tar", dumpdirpath, db_cell->oid);
else
- snprintf(subdirpath, MAXPGPATH, "%s/databases/%u", dumpdirpath, db_cell->oid);
+ {
+ snprintf(dbfilename, MAXPGPATH, "%u.dmp", db_cell->oid);
+ if (file_exists_in_directory(subdirdbpath, dbfilename))
+ snprintf(subdirpath, MAXPGPATH, "%s/databases/%u.dmp", dumpdirpath, db_cell->oid);
+ else
+ snprintf(subdirpath, MAXPGPATH, "%s/databases/%u", dumpdirpath, db_cell->oid);
+ }
}
pg_log_info("restoring database \"%s\"", db_cell->str);
@@ -1364,6 +1381,9 @@ copy_or_print_global_file(const char *outfile, FILE *pfile)
OPF = stdout;
else
{
+ if (strlen(outfile) >= MAXPGPATH)
+ pg_fatal("out file name:\"%s\" is too long", outfile);
+
snprintf(out_file_path, MAXPGPATH, "%s", outfile);
OPF = fopen(out_file_path, PG_BINARY_W);
--
2.39.3
On 11 Apr 2025, at 14:26, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
Hi,
In another thread[1], Álvaro gave some feedback for _fileExistsInDirectory function for "too long name" error.
Basically, in _fileExistsInDirectory function, we pass dirname and filename but we were checking only the combined length of these two names.
My interpretation of the original problem in the other thread is that the
errormessage isn't applicable for a generic function as it only mention
directory, not that checking the combination is inherently wrong.
Here, I am attaching a patch which will check lengths of dirname and filename separately and will report errors if the name is too long.
Since we only care about the combination of directory and filename, do we
really gain much by using separate checks? A proposed filename exceeding
MAXPGPATH should be pretty rare in production I'd hope.
+ if (snprintf(buf, MAXPGPATH, "%s/%s", dir, filename) >= MAXPGPATH)
+ pg_fatal("combined name of directory:\"%s\" and file:\"%s\" is too long", filename, dir);snprintf() will return a negative value in case of an error so if we really
want to clamp down on path generation we should probably check that as well.
--
Daniel Gustafsson