Обсуждение: 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

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

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

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. +