Thread: 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 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. Indeed, that's definitly a bug. Quick patch attached. It does appear to work, but there may be a better way? //Magnus
Attachment
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
> > > 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?) 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
Attachment
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. 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. :)
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
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 automaticallydeleted 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
"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
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
"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
"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? A look at http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vclib/html/_crt__tempnam.2c_._wtempnam.2c_.tmpnam.2c_._wtmpnam.asp 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.asp > > 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? 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
"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
Attachment
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. +