as per commit 643a1a61985bef2590496, move create/open dir code to function using switch case of pg_backup_directory.c file also
Hi,
In commit 643a1a61985bef2590496, we did some cleanup and we replaced
if-else with switch case.
Basically, we made a function to open a directory in pg_dumpall.
In pg_backup_directory.c file also, we are opening a directory with if-else.
Here, I am attaching a patch which makes both the files similar.
We can move this similar function into one common file also but as of now,
I made a static function same as pg_dumpall.c.
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v01_move-create-dir-code-to-the-switch-in-pg_backup_dir.patchapplication/octet-stream; name=v01_move-create-dir-code-to-the-switch-in-pg_backup_dir.patchDownload
From 0a2222476df6eea6f4ffba176395b589d42b22be Mon Sep 17 00:00:00 2001
From: Mahendra Singh Thalor <mahi6run@gmail.com>
Date: Mon, 7 Apr 2025 17:43:05 +0530
Subject: [PATCH] move create dir code to the switch as per commit in
pg_dumpall
in commit 643a1a61985bef2590496, we moved code of create/open directory
into switch case.
Same code we are using in InitArchiveFmt_Directory also.
(file: src/bin/pg_dump/pg_backup_directory.c)
As per pg_dumpall, we can make same here.
If we want, we can move common code to other common file also.
---
src/bin/pg_dump/pg_backup_directory.c | 71 ++++++++++++++-------------
1 file changed, 37 insertions(+), 34 deletions(-)
diff --git a/src/bin/pg_dump/pg_backup_directory.c b/src/bin/pg_dump/pg_backup_directory.c
index b2a841bb0ff..7db1bae334b 100644
--- a/src/bin/pg_dump/pg_backup_directory.c
+++ b/src/bin/pg_dump/pg_backup_directory.c
@@ -39,6 +39,7 @@
#include <dirent.h>
#include <sys/stat.h>
+#include "common/file_perm.h"
#include "common/file_utils.h"
#include "compress_io.h"
#include "parallel.h"
@@ -94,6 +95,7 @@ static int _WorkerJobDumpDirectory(ArchiveHandle *AH, TocEntry *te);
static void setFilePath(ArchiveHandle *AH, char *buf,
const char *relativeFilename);
+static void create_or_open_dir(const char *dirname);
/*
* Init routine required by ALL formats. This is a global routine
@@ -156,41 +158,8 @@ InitArchiveFmt_Directory(ArchiveHandle *AH)
if (AH->mode == archModeWrite)
{
- struct stat st;
- bool is_empty = false;
-
/* we accept an empty existing directory */
- if (stat(ctx->directory, &st) == 0 && S_ISDIR(st.st_mode))
- {
- DIR *dir = opendir(ctx->directory);
-
- if (dir)
- {
- struct dirent *d;
-
- is_empty = true;
- while (errno = 0, (d = readdir(dir)))
- {
- if (strcmp(d->d_name, ".") != 0 && strcmp(d->d_name, "..") != 0)
- {
- is_empty = false;
- break;
- }
- }
-
- if (errno)
- pg_fatal("could not read directory \"%s\": %m",
- ctx->directory);
-
- if (closedir(dir))
- pg_fatal("could not close directory \"%s\": %m",
- ctx->directory);
- }
- }
-
- if (!is_empty && mkdir(ctx->directory, 0700) < 0)
- pg_fatal("could not create directory \"%s\": %m",
- ctx->directory);
+ create_or_open_dir(ctx->directory);
}
else
{ /* Read Mode */
@@ -875,3 +844,37 @@ _WorkerJobRestoreDirectory(ArchiveHandle *AH, TocEntry *te)
{
return parallel_restore(AH, te);
}
+
+/*
+ * create_or_open_dir
+ *
+ * This will create a new directory with the given dirname. If there is
+ * already an empty directory with that name, then use it.
+ */
+void
+create_or_open_dir(const char *dirname)
+{
+ int ret;
+
+ switch ((ret = pg_check_dir(dirname)))
+ {
+ case -1:
+ /* opendir failed but not with ENOENT */
+ pg_fatal("could not open directory \"%s\": %m", dirname);
+ break;
+ case 0:
+ /* directory does not exist */
+ if (mkdir(dirname, pg_dir_create_mode) < 0)
+ pg_fatal("could not create directory \"%s\": %m", dirname);
+ break;
+ case 1:
+ /* exists and is empty, fix perms */
+ if (chmod(dirname, pg_dir_create_mode) != 0)
+ pg_fatal("could not change permissions of directory \"%s\": %m",
+ dirname);
+ break;
+ default:
+ /* exists and is not empty */
+ pg_fatal("directory \"%s\" is not empty", dirname);
+ }
+}
--
2.39.3
On 2025-04-07 Mo 8:25 AM, Mahendra Singh Thalor wrote:
Hi,
In commit 643a1a61985bef2590496, we did some cleanup and we replaced
if-else with switch case.
Basically, we made a function to open a directory in pg_dumpall.
In pg_backup_directory.c file also, we are opening a directory with
if-else.
Here, I am attaching a patch which makes both the files similar.We can move this similar function into one common file also but as of
now, I made a static function same as pg_dumpall.c.
Yeah, let's put it in a common file. There's no point in duplicating it.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Mon, 7 Apr 2025 at 18:52, Andrew Dunstan <andrew@dunslane.net> wrote:
On 2025-04-07 Mo 8:25 AM, Mahendra Singh Thalor wrote:
Hi,
In commit 643a1a61985bef2590496, we did some cleanup and we replaced
if-else with switch case.
Basically, we made a function to open a directory in pg_dumpall.
In pg_backup_directory.c file also, we are opening a directory with
if-else.
Here, I am attaching a patch which makes both the files similar.We can move this similar function into one common file also but as of
now, I made a static function same as pg_dumpall.c.Yeah, let's put it in a common file. There's no point in duplicating it.
Thanks Andrew.
I moved this common function dumputils.c file. Here, I am attaching a
patch for the same.
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v02_move-create-dir-code-to-the-switch-in-pg_backup_dir.patchapplication/octet-stream; name=v02_move-create-dir-code-to-the-switch-in-pg_backup_dir.patchDownload
From 5554ac118a137aa9808f20c02f5a185d637749ec Mon Sep 17 00:00:00 2001
From: Mahendra Singh Thalor <mahi6run@gmail.com>
Date: Tue, 8 Apr 2025 00:25:02 +0530
Subject: [PATCH] Move code of create/open dir to the switch as per commit in
pg_dumpall and move it to commom file (dumputils.c)
in commit 643a1a61985bef2590496, we moved code of create/open directory
into switch case.
Same code we are using in InitArchiveFmt_Directory also.
(file: src/bin/pg_dump/pg_backup_directory.c)
Removing duplicate code by moving function into dumputils.c file.
---
src/bin/pg_dump/dumputils.c | 36 ++++++++++++++++++++++++++
src/bin/pg_dump/dumputils.h | 1 +
src/bin/pg_dump/pg_backup_directory.c | 36 ++------------------------
src/bin/pg_dump/pg_dumpall.c | 37 ---------------------------
4 files changed, 39 insertions(+), 71 deletions(-)
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index ab0e9e6da3c..1d8ffe363e7 100644
--- a/src/bin/pg_dump/dumputils.c
+++ b/src/bin/pg_dump/dumputils.c
@@ -16,6 +16,8 @@
#include <ctype.h>
+#include "common/file_perm.h"
+#include "common/logging.h"
#include "dumputils.h"
#include "fe_utils/string_utils.h"
@@ -884,3 +886,37 @@ makeAlterConfigCommand(PGconn *conn, const char *configitem,
pg_free(mine);
}
+
+/*
+ * create_or_open_dir
+ *
+ * This will create a new directory with the given dirname. If there is
+ * already an empty directory with that name, then use it.
+ */
+void
+create_or_open_dir(const char *dirname)
+{
+ int ret;
+
+ switch ((ret = pg_check_dir(dirname)))
+ {
+ case -1:
+ /* opendir failed but not with ENOENT */
+ pg_fatal("could not open directory \"%s\": %m", dirname);
+ break;
+ case 0:
+ /* directory does not exist */
+ if (mkdir(dirname, pg_dir_create_mode) < 0)
+ pg_fatal("could not create directory \"%s\": %m", dirname);
+ break;
+ case 1:
+ /* exists and is empty, fix perms */
+ if (chmod(dirname, pg_dir_create_mode) != 0)
+ pg_fatal("could not change permissions of directory \"%s\": %m",
+ dirname);
+ break;
+ default:
+ /* exists and is not empty */
+ pg_fatal("directory \"%s\" is not empty", dirname);
+ }
+}
diff --git a/src/bin/pg_dump/dumputils.h b/src/bin/pg_dump/dumputils.h
index a4bd16857ce..91c6e612e28 100644
--- a/src/bin/pg_dump/dumputils.h
+++ b/src/bin/pg_dump/dumputils.h
@@ -62,5 +62,6 @@ extern void makeAlterConfigCommand(PGconn *conn, const char *configitem,
const char *type, const char *name,
const char *type2, const char *name2,
PQExpBuffer buf);
+extern void create_or_open_dir(const char *dirname);
#endif /* DUMPUTILS_H */
diff --git a/src/bin/pg_dump/pg_backup_directory.c b/src/bin/pg_dump/pg_backup_directory.c
index b2a841bb0ff..21b00792a8a 100644
--- a/src/bin/pg_dump/pg_backup_directory.c
+++ b/src/bin/pg_dump/pg_backup_directory.c
@@ -41,6 +41,7 @@
#include "common/file_utils.h"
#include "compress_io.h"
+#include "dumputils.h"
#include "parallel.h"
#include "pg_backup_utils.h"
@@ -156,41 +157,8 @@ InitArchiveFmt_Directory(ArchiveHandle *AH)
if (AH->mode == archModeWrite)
{
- struct stat st;
- bool is_empty = false;
-
/* we accept an empty existing directory */
- if (stat(ctx->directory, &st) == 0 && S_ISDIR(st.st_mode))
- {
- DIR *dir = opendir(ctx->directory);
-
- if (dir)
- {
- struct dirent *d;
-
- is_empty = true;
- while (errno = 0, (d = readdir(dir)))
- {
- if (strcmp(d->d_name, ".") != 0 && strcmp(d->d_name, "..") != 0)
- {
- is_empty = false;
- break;
- }
- }
-
- if (errno)
- pg_fatal("could not read directory \"%s\": %m",
- ctx->directory);
-
- if (closedir(dir))
- pg_fatal("could not close directory \"%s\": %m",
- ctx->directory);
- }
- }
-
- if (!is_empty && mkdir(ctx->directory, 0700) < 0)
- pg_fatal("could not create directory \"%s\": %m",
- ctx->directory);
+ create_or_open_dir(ctx->directory);
}
else
{ /* Read Mode */
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 18b544b0214..1f272b7525b 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -15,7 +15,6 @@
#include "postgres_fe.h"
-#include <sys/stat.h>
#include <time.h>
#include <unistd.h>
@@ -78,7 +77,6 @@ static void executeCommand(PGconn *conn, const char *query);
static void expand_dbname_patterns(PGconn *conn, SimpleStringList *patterns,
SimpleStringList *names);
static void read_dumpall_filters(const char *filename, SimpleStringList *pattern);
-static void create_or_open_dir(const char *dirname);
static ArchiveFormat parseDumpFormat(const char *format);
static char pg_dump_bin[MAXPGPATH];
@@ -1946,41 +1944,6 @@ read_dumpall_filters(const char *filename, SimpleStringList *pattern)
filter_free(&fstate);
}
-/*
- * create_or_open_dir
- *
- * This will create a new directory with the given dirname. If there is
- * already an empty directory with that name, then use it.
- */
-static void
-create_or_open_dir(const char *dirname)
-{
- int ret;
-
- switch ((ret = pg_check_dir(dirname)))
- {
- case -1:
- /* opendir failed but not with ENOENT */
- pg_fatal("could not open directory \"%s\": %m", dirname);
- break;
- case 0:
- /* directory does not exist */
- if (mkdir(dirname, pg_dir_create_mode) < 0)
- pg_fatal("could not create directory \"%s\": %m", dirname);
- break;
- case 1:
- /* exists and is empty, fix perms */
- if (chmod(dirname, pg_dir_create_mode) != 0)
- pg_fatal("could not change permissions of directory \"%s\": %m",
- dirname);
-
- break;
- default:
- /* exists and is not empty */
- pg_fatal("directory \"%s\" is not empty", dirname);
- }
-}
-
/*
* parseDumpFormat
*
--
2.39.3
On 2025-04-07 Mo 2:59 PM, Mahendra Singh Thalor wrote:
On Mon, 7 Apr 2025 at 18:52, Andrew Dunstan <andrew@dunslane.net> wrote:
On 2025-04-07 Mo 8:25 AM, Mahendra Singh Thalor wrote:
Hi,
In commit 643a1a61985bef2590496, we did some cleanup and we replaced
if-else with switch case.
Basically, we made a function to open a directory in pg_dumpall.
In pg_backup_directory.c file also, we are opening a directory with
if-else.
Here, I am attaching a patch which makes both the files similar.We can move this similar function into one common file also but as of
now, I made a static function same as pg_dumpall.c.Yeah, let's put it in a common file. There's no point in duplicating it.
Thanks Andrew.
I moved this common function dumputils.c file. Here, I am attaching a
patch for the same.
Pushed with a slight tweaking.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Thu, 10 Apr 2025 at 21:48, Andrew Dunstan <andrew@dunslane.net> wrote:
On 2025-04-07 Mo 2:59 PM, Mahendra Singh Thalor wrote:
On Mon, 7 Apr 2025 at 18:52, Andrew Dunstan <andrew@dunslane.net> wrote:
On 2025-04-07 Mo 8:25 AM, Mahendra Singh Thalor wrote:
Hi,
In commit 643a1a61985bef2590496, we did some cleanup and we replaced
if-else with switch case.
Basically, we made a function to open a directory in pg_dumpall.
In pg_backup_directory.c file also, we are opening a directory with
if-else.
Here, I am attaching a patch which makes both the files similar.We can move this similar function into one common file also but as of
now, I made a static function same as pg_dumpall.c.Yeah, let's put it in a common file. There's no point in duplicating it.
Thanks Andrew.
I moved this common function dumputils.c file. Here, I am attaching a
patch for the same.Pushed with a slight tweaking.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Thanks Andrew for committing this.
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
I don't understand why the routine is called "create_or_open_dir". In
what sense does this open the directory? I think "check_or_create_dir"
would be closer to what this seem to be doing.
Is there no TOCTTOU bug in pg_dumpall because of the way this code is
written? A malicious user that can create an empty directory that
pg_dumpall is going to use as output destination could remove it after
the opendir(), then replace it with another directory with a symlink
called "global.dat" that causes some other file to be overwritten with
the privileges of the user running pg_dumpall. Maybe there's no problem
here, but I don't see what the explanation for that is.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/