pg_dump -Ft failed on Windows XP
Hi,
I got the following message when I ran pg_dump with -Ft option on
Windows XP.
pg_dump -V
pg_dump (PostgreSQL) 8.1.2
pg_dump -Ft test > C:\backup\xxx.out
pg_dump: [tar archiver] could not generate temporary file name: Permission denied
pg_dump calls tmpfile() in pg_backup_tar.c:tarOpen(). Win32's
tmpfile() creates the file into root folder. But non-administrator
users can't create files into root folder. So, I think it fails that
non-administrator users run pg_dump with -Ft option.
Regards,
--
Yoshiyuki Asaba
y-asaba@sraoss.co.jp
I got the following message when I ran pg_dump with -Ft
option on Windows XP.pg_dump -V
pg_dump (PostgreSQL) 8.1.2
pg_dump -Ft test > C:\backup\xxx.out
pg_dump: [tar archiver] could not generate temporary file
name: Permission deniedpg_dump calls tmpfile() in pg_backup_tar.c:tarOpen(). Win32's
tmpfile() creates the file into root folder. But
non-administrator users can't create files into root folder.
So, I think it fails that non-administrator users run pg_dump
with -Ft option.
Indeed, that's definitly a bug. Quick patch attached. It does appear to
work, but there may be a better way?
//Magnus
Attachments:
pg_dump_tempfile.patchapplication/octet-stream; name=pg_dump_tempfile.patchDownload
Index: bin/pg_dump/pg_backup_tar.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/pg_dump/pg_backup_tar.c,v
retrieving revision 1.50
diff -c -r1.50 pg_backup_tar.c
*** bin/pg_dump/pg_backup_tar.c 12 Feb 2006 06:11:50 -0000 1.50
--- bin/pg_dump/pg_backup_tar.c 20 Apr 2006 08:45:03 -0000
***************
*** 68,73 ****
--- 68,74 ----
FILE *tarFH;
FILE *tmpFH;
char *targetFile;
+ char *tempFile;
char mode;
off_t pos;
off_t fileLen;
***************
*** 360,370 ****
}
else
{
tm = calloc(1, sizeof(TAR_MEMBER));
! tm->tmpFH = tmpfile();
! if (tm->tmpFH == NULL)
die_horribly(AH, modulename, "could not generate temporary file name: %s\n", strerror(errno));
#ifdef HAVE_LIBZ
--- 361,378 ----
}
else
{
+ char *tmpname;
tm = calloc(1, sizeof(TAR_MEMBER));
! tmpname = tempnam(NULL, NULL);
! if (tmpname != NULL)
! {
! tm->tempFile = strdup(tmpname);
! if (tm->tempFile != NULL)
! tm->tmpFH = fopen(tm->tempFile , "w+b");
! }
! if (tmpname == NULL || tm->tempFile == NULL || tm->tmpFH == NULL)
die_horribly(AH, modulename, "could not generate temporary file name: %s\n", strerror(errno));
#ifdef HAVE_LIBZ
***************
*** 1040,1047 ****
len += res;
}
! if (fclose(tmp) != 0) /* This *should* delete it... */
die_horribly(AH, modulename, "could not close tar member: %s\n", strerror(errno));
if (len != th->fileLen)
{
--- 1048,1060 ----
len += res;
}
! if (fclose(tmp) != 0)
die_horribly(AH, modulename, "could not close tar member: %s\n", strerror(errno));
+ if (th->tempFile != NULL)
+ {
+ unlink(th->tempFile);
+ free(th->tempFile);
+ }
if (len != th->fileLen)
{
Import Notes
Resolved by subject fallback
Am Donnerstag, 20. April 2006 10:47 schrieb Magnus Hagander:
Indeed, that's definitly a bug. Quick patch attached. It does appear to
work, but there may be a better way?
This patch introduces a security hole because an attacker could create, say, a
suitable symlink between the time the name is generated and the file is
opened.
--
Peter Eisentraut
http://developer.postgresql.org/~petere/
Indeed, that's definitly a bug. Quick patch attached. It
does appear
to work, but there may be a better way?
This patch introduces a security hole because an attacker
could create, say, a suitable symlink between the time the
name is generated and the file is opened.
Good point. I guess what I need to do is use open() specifying O_CREATE,
and then fdopen() that file.
Question: Is the use of O_TEMPORARY to open() portable? (my win32 docs
say it will make the file automatically deleted when the last descriptor
is closed, which I didn't know before. That would make the patch much
simpler, but might require #ifdefs?)
//Magnus
Import Notes
Resolved by subject fallback
Indeed, that's definitly a bug. Quick patch attached. It
does appear
to work, but there may be a better way?
This patch introduces a security hole because an attacker could
create, say, a suitable symlink between the time the nameis generated
and the file is opened.
Good point. I guess what I need to do is use open()
specifying O_CREATE, and then fdopen() that file.Question: Is the use of O_TEMPORARY to open() portable? (my
win32 docs say it will make the file automatically deleted
when the last descriptor is closed, which I didn't know
before. That would make the patch much simpler, but might
require #ifdefs?)
Actually, since I'm running out the door, here is a new attempt that
changes behaviour only on win32. And that also appears to work, but may
be wrong ;-)
//Magnus
Attachments:
pg_dump_tempfile.patchapplication/octet-stream; name=pg_dump_tempfile.patchDownload
Index: bin/pg_dump/pg_backup_tar.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/pg_dump/pg_backup_tar.c,v
retrieving revision 1.50
diff -c -r1.50 pg_backup_tar.c
*** bin/pg_dump/pg_backup_tar.c 12 Feb 2006 06:11:50 -0000 1.50
--- bin/pg_dump/pg_backup_tar.c 20 Apr 2006 11:17:42 -0000
***************
*** 362,368 ****
--- 362,388 ----
{
tm = calloc(1, sizeof(TAR_MEMBER));
+ #ifndef WIN32
tm->tmpFH = tmpfile();
+ #else
+ /* on win32, tmpfile() generates a filename in the root directory, which requires
+ * administrative permissions to write to. */
+ while (1)
+ {
+ char *tmpname;
+ int fd;
+
+ tmpname = tempnam(NULL, NULL);
+ if (tmpname == NULL)
+ break;
+ fd = _open(tmpname, _O_RDWR | _O_CREAT | _O_EXCL | _O_TEMPORARY, 0);
+ if (fd == -1 && errno == EEXIST)
+ continue; /* Try again with a new name if file exists */
+ if (fd != -1)
+ tm->tmpFH = fdopen(fd, "w+b");
+ break;
+ }
+ #endif
if (tm->tmpFH == NULL)
die_horribly(AH, modulename, "could not generate temporary file name: %s\n", strerror(errno));
Import Notes
Resolved by subject fallback
Am Donnerstag, 20. April 2006 13:03 schrieb Magnus Hagander:
Question: Is the use of O_TEMPORARY to open() portable? (my win32 docs
say it will make the file automatically deleted when the last descriptor
is closed, which I didn't know before. That would make the patch much
simpler, but might require #ifdefs?)
I think it would be more elegant if you wrote a replacement implementation of
tmpfile() for pgport and did not change pg_dump at all. And/or write a bug
to Microsoft about a buggy C library. :)
--
Peter Eisentraut
http://developer.postgresql.org/~petere/
Question: Is the use of O_TEMPORARY to open() portable? (my
win32 docs
say it will make the file automatically deleted when the last
descriptor is closed, which I didn't know before. Thatwould make the
patch much simpler, but might require #ifdefs?)
I think it would be more elegant if you wrote a replacement
implementation of
tmpfile() for pgport and did not change pg_dump at all.
And/or write a bug to Microsoft about a buggy C library. :)
It's not buggy. It's well documented behaviour,and per my linux manpage
for the file it's also OK per spec:
The standard does not specify the directory that tmpfile()
will use. Glibc will try the path prefix P_tmpdir defined
in <stdio.h>, and if that fails the directory /tmp.
It's certainly *stupid*, but not buggy ;-)
//Magnus
Import Notes
Resolved by subject fallback
Am Donnerstag, 20. April 2006 13:21 schrieb Magnus Hagander:
It's not buggy. It's well documented behaviour,and per my linux manpage
for the file it's also OK per spec:The standard does not specify the directory that tmpfile()
will use. Glibc will try the path prefix P_tmpdir defined
in <stdio.h>, and if that fails the directory /tmp.
The spec says
The tmpfile() function shall create a temporary file and open a
corresponding stream. The file shall be automatically deleted when all
references to the file are closed. The file is opened as in fopen() for
update (w+).
If the implementation is such that it tries to create the file in a directory
that the user does not have write permission to, it's a bug.
--
Peter Eisentraut
http://developer.postgresql.org/~petere/
It's not buggy. It's well documented behaviour,and per my linux
manpage for the file it's also OK per spec:The standard does not specify the directory that tmpfile()
will use. Glibc will try the path prefix P_tmpdir defined
in <stdio.h>, and if that fails the directory /tmp.The spec says
The tmpfile() function shall create a temporary file and open a
corresponding stream. The file shall be automatically
deleted when all
references to the file are closed. The file is opened as
in fopen() for
update (w+).If the implementation is such that it tries to create the
file in a directory that the user does not have write
permission to, it's a bug.
Well, you're never gonig to convince MS of that :-)
And either way, the runtime we're usnig now isn't especially current
(MSVC6), so even in the unlikely event that they did fix it, it wouldn't
help us. So we'll definitly need to fix it ourselves. Did the code I
sent the last time look reasonable?
//Magnus
Import Notes
Resolved by subject fallback
"Magnus Hagander" <mha@sollentuna.net> writes:
Win32's tmpfile() creates the file into root folder. But
non-administrator users can't create files into root folder.
In other words, tmpfile() doesn't work at all on Win32? Seems like
the appropriate place to be filing a bug report is at microsoft.com.
regards, tom lane
Win32's tmpfile() creates the file into root folder. But
non-administrator users can't create files into root folder.In other words, tmpfile() doesn't work at all on Win32?
Seems like the appropriate place to be filing a bug report is
at microsoft.com.
If works if you're an administrator or power user. Sure, we can file a
report, but if they bother to fix it, that will be for Visual Studio
2005 (if we're lucky, more likely for the next version after that). So
we can forget all about mingw then, since it uses an old version of the
runtime...
Though I doubt they'd fix it anyway, since their documentation alreadyu
tells you what to do to get around it.
//Magnus
Import Notes
Resolved by subject fallback
Peter Eisentraut <peter_e@gmx.net> writes:
If the implementation is such that it tries to create the file in a directory
that the user does not have write permission to, it's a bug.
Well, I think it would be a valid implementation on Unix to always try
to create the file in /tmp, which'd likely fail if someone had revoked
world write on /tmp --- but doing the latter is administrator error,
not a library fault.
OTOH, if / is *supposed* to be non world writable on Win32, then trying
to create temp files there indicates a seriously brain-damaged library.
It should be trying to create the file in a place where the user is
expected to have permission to do it.
Has anyone looked to see with tmpfile() actually does though? I'm a bit
surprised that it doesn't create stuff in the same directory as tmpnam().
I wonder if Magnus and Yoshiyuki are testing under different conditions.
regards, tom lane
From: Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: [HACKERS] pg_dump -Ft failed on Windows XP
Date: Thu, 20 Apr 2006 10:00:48 -0400
"Magnus Hagander" <mha@sollentuna.net> writes:
Win32's tmpfile() creates the file into root folder. But
non-administrator users can't create files into root folder.In other words, tmpfile() doesn't work at all on Win32? Seems like
the appropriate place to be filing a bug report is at microsoft.com.
Yes. There is the description of Win32's tmpfile() at the following URL.
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vclib/html/_crt_tmpfile.asp
--
Yoshiyuki Asaba
y-asaba@sraoss.co.jp
If the implementation is such that it tries to create the file in a
directory that the user does not have write permission to,it's a bug.
Well, I think it would be a valid implementation on Unix to
always try to create the file in /tmp, which'd likely fail if
someone had revoked world write on /tmp --- but doing the
latter is administrator error, not a library fault.OTOH, if / is *supposed* to be non world writable on Win32,
then trying to create temp files there indicates a seriously
brain-damaged library.
It should be trying to create the file in a place where the
user is expected to have permission to do it.
Prior to Windows XP, users had write permissions in the root IIRC. They
definitly had in NT4, but I think they did in 2000 as well.
Has anyone looked to see with tmpfile() actually does though?
I'm a bit surprised that it doesn't create stuff in the same
directory as tmpnam().
I wonder if Magnus and Yoshiyuki are testing under different
conditions.
I have repeated the problem with CVS head on XP SP2. It *does* create it
there (or rather, it tries to).
tmpnam() returns a file in the current dir per documentation, but I see
it generating one in the root instead.
tempnam() uses TMP environment variable.
tmpfile() and tmpnam() both use the same function to generate the
filename.
//Magnus
Import Notes
Resolved by subject fallback
"Magnus Hagander" <mha@sollentuna.net> writes:
I have repeated the problem with CVS head on XP SP2. It *does* create it
there (or rather, it tries to).
tmpnam() returns a file in the current dir per documentation, but I see
it generating one in the root instead.
tempnam() uses TMP environment variable.
tmpfile() and tmpnam() both use the same function to generate the
filename.
Hm. I guess I concur with Peter's conclusion: the cleanest fix is to
put our own implementation of tmpfile() into src/port/.
regards, tom lane
I have repeated the problem with CVS head on XP SP2. It
*does* create
it there (or rather, it tries to).
tmpnam() returns a file in the current dir per documentation, but I
see it generating one in the root instead.
tempnam() uses TMP environment variable.tmpfile() and tmpnam() both use the same function to generate the
filename.Hm. I guess I concur with Peter's conclusion: the cleanest
fix is to put our own implementation of tmpfile() into src/port/.
Ok. Should be easy enough once the code is fine - can you comment on the
patch as sent, if the code itself looks right provided i wrap it up in a
function in port/?
//Magnus
Import Notes
Resolved by subject fallback
"Magnus Hagander" <mha@sollentuna.net> writes:
Ok. Should be easy enough once the code is fine - can you comment on the
patch as sent, if the code itself looks right provided i wrap it up in a
function in port/?
Not sure if the error handling is adequate --- are there any cases
besides EEXIST that should loop?
suggests that tempnam() is also pretty fragile, esp. if you're passing
NULLs. Apparently it won't work at all if TMP isn't set?
regards, tom lane
Ok. Should be easy enough once the code is fine - can you
comment on
the patch as sent, if the code itself looks right provided
i wrap it
up in a function in port/?
Not sure if the error handling is adequate --- are there any
cases besides EEXIST that should loop?
Well, per docs I can get:
EACCES (will certainly fail again since we're still working the same
directory)
EEXIST (covered)
EINVAL = invalid oflag or pmode, which should never happen
EMFILE = too many open files (that would be the one, but I doubd it'd
help that much)
ENOENT = path ont found (would happen agai non retry)
A look at
http://msdn.microsoft.com/library/default.asp?url=/library/en-
us/vclib/html/_crt__tempnam.2c_._wtempnam.2c_.tmpnam.2c_._wtmpnam.aspsuggests that tempnam() is also pretty fragile, esp. if
you're passing NULLs. Apparently it won't work at all if TMP
isn't set?
I'm not *too* concerned about that, since TMP is normally set by the OS
itself. There's one set in the "system environment" (to c:\windows\temp
or whatrever) and then it's overridden by one set by the OS when it
loads a user profile.
Also to the point, what would you fall back to?
//Magnus
Import Notes
Resolved by subject fallback
"Magnus Hagander" <mha@sollentuna.net> writes:
Apparently it won't work at all if TMP
isn't set?
I'm not *too* concerned about that, since TMP is normally set by the OS
itself. There's one set in the "system environment" (to c:\windows\temp
or whatrever) and then it's overridden by one set by the OS when it
loads a user profile.
OK, then maybe not having it would be equivalent to /tmp-not-writable
on Unix, ie, admin error.
Also to the point, what would you fall back to?
Current directory maybe?
Actually, the thing that struck me about the man page was that first
it said current dir was a fallback, and then it said it wasn't. Is this
just typical Microsoft brain damage, or is there a reason to avoid using
current dir for this?
regards, tom lane
Apparently it won't work at all if TMP isn't set?
I'm not *too* concerned about that, since TMP is normally set by the
OS
itself. There's one set in the "system environment" (to
c:\windows\temp
or whatrever) and then it's overridden by one set by the OS when it
loads a user profile.OK, then maybe not having it would be equivalent to /tmp-not-writable
on Unix, ie, admin error.Also to the point, what would you fall back to?
Current directory maybe?
It tries \ (tested on Win 2000), if the dir argument is NULL and TMP is
not set.
But TMP is usually set.
Attached is a working version not yet adapted to port/.
- memoryleak fixed
- use _tmpname and _fdopen not the compatibility tmpname and fdopen
(imho only cosmetic)
- EACCES fixed (Win2000 needs _S_IREAD | _S_IWRITE or fails with EACCES,
even as Admin)
- I suggest adding a prefix pg_temp_ (for leftover temp files after
crash,
the name I get is then usually pg_temp_2)
Andreas
Attachments:
pg_dump_tempfile.patch.txttext/plain; name=pg_dump_tempfile.patch.txtDownload
Index: bin/pg_dump/pg_backup_tar.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/pg_dump/pg_backup_tar.c,v
retrieving revision 1.50
diff -c -r1.50 pg_backup_tar.c
*** bin/pg_dump/pg_backup_tar.c 12 Feb 2006 06:11:50 -0000 1.50
--- bin/pg_dump/pg_backup_tar.c 21 Apr 2006 09:22:00 -0000
***************
*** 362,368 ****
--- 362,388 ----
{
tm = calloc(1, sizeof(TAR_MEMBER));
+ #ifndef WIN32
tm->tmpFH = tmpfile();
+ #else
+ /* on win32, tmpfile() generates a filename in the root directory, which requires
+ * administrative permissions to write to. */
+ while (1)
+ {
+ char *tmpname;
+ int fd;
+
+ tmpname = _tempnam(NULL, "pg_temp_");
+ if (tmpname == NULL)
+ break;
+ fd = _open(tmpname, _O_RDWR | _O_CREAT | _O_EXCL | _O_BINARY | _O_TEMPORARY, _S_IREAD | _S_IWRITE);
+ free(tmpname);
+ if (fd == -1 && errno == EEXIST)
+ continue; /* Try again with a new name if file exists */
+ if (fd != -1)
+ tm->tmpFH = _fdopen(fd, "w+b");
+ break;
+ }
+ #endif
if (tm->tmpFH == NULL)
die_horribly(AH, modulename, "could not generate temporary file name: %s\n", strerror(errno));
Import Notes
Resolved by subject fallback
Someday we can move this to /port, but for now, let's get it into CVS.
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.
---------------------------------------------------------------------------
Zeugswetter Andreas DCP SD wrote:
Apparently it won't work at all if TMP isn't set?
I'm not *too* concerned about that, since TMP is normally set by the
OS
itself. There's one set in the "system environment" (to
c:\windows\temp
or whatrever) and then it's overridden by one set by the OS when it
loads a user profile.OK, then maybe not having it would be equivalent to /tmp-not-writable
on Unix, ie, admin error.Also to the point, what would you fall back to?
Current directory maybe?
It tries \ (tested on Win 2000), if the dir argument is NULL and TMP is
not set.
But TMP is usually set.Attached is a working version not yet adapted to port/.
- memoryleak fixed
- use _tmpname and _fdopen not the compatibility tmpname and fdopen
(imho only cosmetic)
- EACCES fixed (Win2000 needs _S_IREAD | _S_IWRITE or fails with EACCES,
even as Admin)
- I suggest adding a prefix pg_temp_ (for leftover temp files after
crash,
the name I get is then usually pg_temp_2)Andreas
Content-Description: pg_dump_tempfile.patch.txt
[ Attachment, skipping... ]
---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster
--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Modified patch attached and applied to HEAD and 8.1.X.
I restructured the loop exit, and used the symbols without the leading
underscores. I didn't see any Win32 underscore symbol usage in our
existing code.
Thanks.
-------------------------------------------------------------------------
Zeugswetter Andreas DCP SD wrote:
Apparently it won't work at all if TMP isn't set?
I'm not *too* concerned about that, since TMP is normally set by the
OS
itself. There's one set in the "system environment" (to
c:\windows\temp
or whatrever) and then it's overridden by one set by the OS when it
loads a user profile.OK, then maybe not having it would be equivalent to /tmp-not-writable
on Unix, ie, admin error.Also to the point, what would you fall back to?
Current directory maybe?
It tries \ (tested on Win 2000), if the dir argument is NULL and TMP is
not set.
But TMP is usually set.Attached is a working version not yet adapted to port/.
- memoryleak fixed
- use _tmpname and _fdopen not the compatibility tmpname and fdopen
(imho only cosmetic)
- EACCES fixed (Win2000 needs _S_IREAD | _S_IWRITE or fails with EACCES,
even as Admin)
- I suggest adding a prefix pg_temp_ (for leftover temp files after
crash,
the name I get is then usually pg_temp_2)Andreas
Content-Description: pg_dump_tempfile.patch.txt
[ Attachment, skipping... ]
---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster
--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Attachments:
/rtmp/difftext/x-diffDownload
Index: src/bin/pg_dump/pg_backup_tar.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/pg_dump/pg_backup_tar.c,v
retrieving revision 1.52
diff -c -c -r1.52 pg_backup_tar.c
*** src/bin/pg_dump/pg_backup_tar.c 7 Jun 2006 22:24:44 -0000 1.52
--- src/bin/pg_dump/pg_backup_tar.c 27 Jun 2006 00:32:06 -0000
***************
*** 359,365 ****
--- 359,393 ----
{
tm = calloc(1, sizeof(TAR_MEMBER));
+ #ifndef WIN32
tm->tmpFH = tmpfile();
+ #else
+ /*
+ * On WIN32, tmpfile() generates a filename in the root directory,
+ * which requires administrative permissions on certain systems.
+ * Loop until we find a unique file name we can create.
+ */
+ while (1)
+ {
+ char *name;
+ int fd;
+
+ name = _tempnam(NULL, "pg_temp_");
+ if (name == NULL)
+ break;
+ fd = open(name, O_RDWR | O_CREAT | O_EXCL | O_BINARY |
+ O_TEMPORARY, S_IREAD | S_IWRITE);
+ free(name);
+
+ if (fd != -1) /* created a file */
+ {
+ tm->tmpFH = fdopen(fd, "w+b");
+ break;
+ }
+ else if (errno != EEXIST) /* failure other than file exists */
+ break;
+ }
+ #endif
if (tm->tmpFH == NULL)
die_horribly(AH, modulename, "could not generate temporary file name: %s\n", strerror(errno));