Horton

Thread

  1. merge file_exists_in_directory and _fileExistsInDirectory functions and move into common file dumputils.c

    Mahendra Singh Thalor <mahi6run@gmail.com> — 2025-04-10T17:11:33Z

    Hi,
We have file_exists_in_directory function in pg_restore.c and same
code we are using in _fileExistsInDirectory function in pg_backup_archiver.c
also.
Here, I am attaching a patch to move these duplicate functions into
dumputils.c file

-- 
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

  2. Re: merge file_exists_in_directory and _fileExistsInDirectory functions and move into common file dumputils.c

    Michael Paquier <michael@paquier.xyz> — 2025-04-11T04:51:08Z

    On Thu, Apr 10, 2025 at 10:41:33PM +0530, Mahendra Singh Thalor wrote:
> We have file_exists_in_directory function in pg_restore.c and same
> code we are using in _fileExistsInDirectory function in pg_backup_archiver.c
> also.
> Here, I am attaching a patch to move these duplicate functions into
> dumputils.c file

Indeed.  I don't quite see a reason not to remove this duplication,
and both routines in pg_restore.c and the pg_dump code are the same.

dumputils.h is only used by pg_dump and pg_dumpall, and its top
comment documents exactly that, so using this location for a routine
that would be used by a pg_restore path is a bit strange to me for
something that is qualified as a "dump" routine in your patch.

Perhaps we should just use a more centralized place, like file_utils.c
so as all frontends could benefit of it?

Please make sure to add it to the next commit fest that will begin in
July, this refactoring proposal is too late to be considered for v18.
--
Michael

  3. Re: merge file_exists_in_directory and _fileExistsInDirectory functions and move into common file dumputils.c

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-04-11T09:38:12Z

    On 2025-Apr-11, Michael Paquier wrote:

> Perhaps we should just use a more centralized place, like file_utils.c
> so as all frontends could benefit of it?

I'm not sure about that.  This code looks to be making too many
assumptions that aren't acceptable for a general routine, such as
complaining only that the directory name is long without the possibility
that the culprit is the file name.  It's more or less okay in current
uses because they're all using harcoded short names, but that would not
hold in general.  At the same time, isn't every call of this routine a
potential TOCTTOU bug?  Again it's probably fine for the current code,
but I wouldn't be too sure about making this generally available as-is.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Oh, great altar of passive entertainment, bestow upon me thy discordant images
at such speed as to render linear thought impossible" (Calvin a la TV)

  4. Re: merge file_exists_in_directory and _fileExistsInDirectory functions and move into common file dumputils.c

    Mahendra Singh Thalor <mahi6run@gmail.com> — 2025-04-11T12:30:02Z

    On Fri, 11 Apr 2025 at 10:21, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Apr 10, 2025 at 10:41:33PM +0530, Mahendra Singh Thalor wrote:
> > We have file_exists_in_directory function in pg_restore.c and same
> > code we are using in _fileExistsInDirectory function in
pg_backup_archiver.c
> > also.
> > Here, I am attaching a patch to move these duplicate functions into
> > dumputils.c file
>
> Indeed.  I don't quite see a reason not to remove this duplication,
> and both routines in pg_restore.c and the pg_dump code are the same.

Thanks Michael for the feedback.

>
> dumputils.h is only used by pg_dump and pg_dumpall, and its top
> comment documents exactly that, so using this location for a routine
> that would be used by a pg_restore path is a bit strange to me for
> something that is qualified as a "dump" routine in your patch.
>
> Perhaps we should just use a more centralized place, like file_utils.c
> so as all frontends could benefit of it?

I tried to add it into file_utils.c but I was getting many "symbols not
found errors"  so I moved this common function into dumputils.h as we have
another common function in that file. (Ex; create_or_open_dir)

If we want to move this function into file_utils.c, then I can try to
rewrite the patch.

>
> Please make sure to add it to the next commit fest that will begin in
> July, this refactoring proposal is too late to be considered for v18.
> --
> Michael

Okay. Thank you. I will add it.

On Fri, 11 Apr 2025 at 15:08, Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> On 2025-Apr-11, Michael Paquier wrote:
>
> > Perhaps we should just use a more centralized place, like file_utils.c
> > so as all frontends could benefit of it?
>
> I'm not sure about that.  This code looks to be making too many
> assumptions that aren't acceptable for a general routine, such as
> complaining only that the directory name is long without the possibility
> that the culprit is the file name.  It's more or less okay in current
> uses because they're all using harcoded short names, but that would not
> hold in general.  At the same time, isn't every call of this routine a
> potential TOCTTOU bug?  Again it's probably fine for the current code,
> but I wouldn't be too sure about making this generally available as-is.
>
> --
> Álvaro Herrera               48°01'N 7°57'E  —
https://www.EnterpriseDB.com/
> "Oh, great altar of passive entertainment, bestow upon me thy discordant
images
> at such speed as to render linear thought impossible" (Calvin a la TV)

Thanks Álvaro for the feedback.

/*
>  * file_exists_in_directory
>  *
>  * Returns true if the file exists in the given directory.
>  */
> static bool
> file_exists_in_directory(const char *dir, const char *filename)
> {
>     struct stat st;
>     char        buf[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));
> }
>

I did changes as per above code and added some checks in code to give an
error for "too long" name. I started a new thread[1] for "too long names"
check and later I will post an updated patch here to move
duplicate functions into one common file.

[1] :
https://www.postgresql.org/message-id/CAKYtNApPmWmU9rdf__D=cA7ivL6H_UrPc=w0CHW74P2acxJJ-g@mail.gmail.com

-- 
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com