Обсуждение: Problem with pg_upgrade's directory write check on Windows
Pg_upgrade writes temporary files (e.g. _dumpall output) into the
current directory, rather than a temporary directory or the user's home
directory. (This was decided by community discussion.)
I have a check in pg_upgrade 9.1 to make sure pg_upgrade has write
permission in the current directory:
if (access(".", R_OK | W_OK#ifndef WIN32 /* * Do a directory execute check only on Unix because execute
permissionon * NTFS means "can execute scripts", which we don't care about. Also, X_OK * is not defined in the
WindowsAPI. */ | X_OK#endif ) != 0) pg_log(PG_FATAL, "You must have
readand write access in the current directory.\n");
Unfortunately, I have received a bug report from EnterpriseDB testing
that this does not trigger the FATAL exit on Windows even if the user
does not have write permission in the current directory, e.g. C:\.
I think I see the cause of the problem. access() on Windows is
described here:
http://msdn.microsoft.com/en-us/library/1w06ktdy%28v=vs.80%29.aspx
It specifically says:
When used with directories, _access determines only whether thespecified directory exists; in Windows NT 4.0 and
Windows2000, alldirectories have read and write access....This function only checks whether the file and directory are
read-onlyornot, it does not check the filesystem security settings. Forthat you need an access token.
We do use access() in a few other places in our code, but not for
directory permission checks.
Any ideas on a solution? Will checking stat() work? Do I have to try
creating a dummy file and delete it?
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ It's impossible for everything to be true. +
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ It's impossible for everything to be true. +
On 07/23/2011 08:45 AM, Bruce Momjian wrote:
> Pg_upgrade writes temporary files (e.g. _dumpall output) into the
> current directory, rather than a temporary directory or the user's home
> directory. (This was decided by community discussion.)
>
> I have a check in pg_upgrade 9.1 to make sure pg_upgrade has write
> permission in the current directory:
>
> if (access(".", R_OK | W_OK
> #ifndef WIN32
>
> /*
> * Do a directory execute check only on Unix because execute permission on
> * NTFS means "can execute scripts", which we don't care about. Also, X_OK
> * is not defined in the Windows API.
> */
> | X_OK
> #endif
> ) != 0)
> pg_log(PG_FATAL,
> "You must have read and write access in the current directory.\n");
>
> Unfortunately, I have received a bug report from EnterpriseDB testing
> that this does not trigger the FATAL exit on Windows even if the user
> does not have write permission in the current directory, e.g. C:\.
>
> I think I see the cause of the problem. access() on Windows is
> described here:
>
> http://msdn.microsoft.com/en-us/library/1w06ktdy%28v=vs.80%29.aspx
>
> It specifically says:
>
> When used with directories, _access determines only whether the
> specified directory exists; in Windows NT 4.0 and Windows 2000, all
> directories have read and write access.
> ...
> This function only checks whether the file and directory are read-only
> or not, it does not check the filesystem security settings. For
> that you need an access token.
>
> We do use access() in a few other places in our code, but not for
> directory permission checks.
>
> Any ideas on a solution? Will checking stat() work? Do I have to try
> creating a dummy file and delete it?
That looks like the obvious solution - it's what came to my mind even
before reading this sentence.
cheers
andrew
Andrew Dunstan wrote:
> > We do use access() in a few other places in our code, but not for
> > directory permission checks.
> >
> > Any ideas on a solution? Will checking stat() work? Do I have to try
> > creating a dummy file and delete it?
>
> That looks like the obvious solution - it's what came to my mind even
> before reading this sentence.
Well, the easy fix is to see if ALL_DUMP_FILE
("pg_upgrade_dump_all.sql") exists, and if so remove it, and if not,
create it and remove it.
Should I fix this in pg_upgrade 9.1 for Windows or just in 9.2? The
check works fine on non-Windows.
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ It's impossible for everything to be true. +
On Sat, Jul 23, 2011 at 9:14 AM, Bruce Momjian <bruce@momjian.us> wrote:
> Andrew Dunstan wrote:
>> > We do use access() in a few other places in our code, but not for
>> > directory permission checks.
>> >
>> > Any ideas on a solution? Will checking stat() work? Do I have to try
>> > creating a dummy file and delete it?
>>
>> That looks like the obvious solution - it's what came to my mind even
>> before reading this sentence.
>
> Well, the easy fix is to see if ALL_DUMP_FILE
> ("pg_upgrade_dump_all.sql") exists, and if so remove it, and if not,
> create it and remove it.
>
> Should I fix this in pg_upgrade 9.1 for Windows or just in 9.2? The
> check works fine on non-Windows.
Seems worth back-patching to me.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas wrote:
> On Sat, Jul 23, 2011 at 9:14 AM, Bruce Momjian <bruce@momjian.us> wrote:
> > Andrew Dunstan wrote:
> >> > We do use access() in a few other places in our code, but not for
> >> > directory permission checks.
> >> >
> >> > Any ideas on a solution? ?Will checking stat() work? ?Do I have to try
> >> > creating a dummy file and delete it?
> >>
> >> That looks like the obvious solution - it's what came to my mind even
> >> before reading this sentence.
> >
> > Well, the easy fix is to see if ALL_DUMP_FILE
> > ("pg_upgrade_dump_all.sql") exists, and if so remove it, and if not,
> > create it and remove it.
> >
> > Should I fix this in pg_upgrade 9.1 for Windows or just in 9.2? ?The
> > check works fine on non-Windows.
>
> Seems worth back-patching to me.
Attached patch applied and backpatched to 9.1. I was able to test both
code paths on my BSD machine by modifying the ifdefs. I will have
EnterpriseDB do further testing.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/exec.c b/contrib/pg_upgrade/exec.c
new file mode 100644
index a76c06e..3493696
*** a/contrib/pg_upgrade/exec.c
--- b/contrib/pg_upgrade/exec.c
***************
*** 16,21 ****
--- 16,24 ----
static void check_data_dir(const char *pg_data);
static void check_bin_dir(ClusterInfo *cluster);
static void validate_exec(const char *dir, const char *cmdName);
+ #ifdef WIN32
+ static int win32_check_directory_write_permissions(void);
+ #endif
/*
*************** verify_directories(void)
*** 97,113 ****
prep_status("Checking current, bin, and data directories");
- if (access(".", R_OK | W_OK
#ifndef WIN32
!
! /*
! * Do a directory execute check only on Unix because execute permission on
! * NTFS means "can execute scripts", which we don't care about. Also, X_OK
! * is not defined in the Windows API.
! */
! | X_OK
#endif
- ) != 0)
pg_log(PG_FATAL,
"You must have read and write access in the current directory.\n");
--- 100,110 ----
prep_status("Checking current, bin, and data directories");
#ifndef WIN32
! if (access(".", R_OK | W_OK | X_OK) != 0)
! #else
! if (win32_check_directory_write_permissions() != 0)
#endif
pg_log(PG_FATAL,
"You must have read and write access in the current directory.\n");
*************** verify_directories(void)
*** 119,124 ****
--- 116,147 ----
}
+ #ifdef WIN32
+ /*
+ * win32_check_directory_write_permissions()
+ *
+ * access() on WIN32 can't check directory permissions, so we have to
+ * optionally create, then delete a file to check.
+ * http://msdn.microsoft.com/en-us/library/1w06ktdy%28v=vs.80%29.aspx
+ */
+ static int
+ win32_check_directory_write_permissions(void)
+ {
+ int fd;
+
+ /*
+ * We open a file we would normally create anyway. We do this even in
+ * 'check' mode, which isn't ideal, but this is the best we can do.
+ */
+ if ((fd = open(GLOBALS_DUMP_FILE, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR)) < 0)
+ return -1;
+ close(fd);
+
+ return unlink(GLOBALS_DUMP_FILE);
+ }
+ #endif
+
+
/*
* check_data_dir()
*
Excerpts from Bruce Momjian's message of dom jul 24 01:46:08 -0400 2011: > Robert Haas wrote: > > > Should I fix this in pg_upgrade 9.1 for Windows or just in 9.2? ?The > > > check works fine on non-Windows. > > > > Seems worth back-patching to me. > > Attached patch applied and backpatched to 9.1. I was able to test both > code paths on my BSD machine by modifying the ifdefs. I will have > EnterpriseDB do further testing. Err, why not 9.0? -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera wrote: > Excerpts from Bruce Momjian's message of dom jul 24 01:46:08 -0400 2011: > > Robert Haas wrote: > > > > > Should I fix this in pg_upgrade 9.1 for Windows or just in 9.2? ?The > > > > check works fine on non-Windows. > > > > > > Seems worth back-patching to me. > > > > Attached patch applied and backpatched to 9.1. I was able to test both > > code paths on my BSD machine by modifying the ifdefs. I will have > > EnterpriseDB do further testing. > > Err, why not 9.0? The check did not exist in 9.0 -- I mentioned that in the commit message. I could add the check into 9.0, but we usually don't backpatch such things. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Sun, Jul 24, 2011 at 5:27 PM, Bruce Momjian <bruce@momjian.us> wrote: > Alvaro Herrera wrote: >> Excerpts from Bruce Momjian's message of dom jul 24 01:46:08 -0400 2011: >> > Robert Haas wrote: >> >> > > > Should I fix this in pg_upgrade 9.1 for Windows or just in 9.2? ?The >> > > > check works fine on non-Windows. >> > > >> > > Seems worth back-patching to me. >> > >> > Attached patch applied and backpatched to 9.1. I was able to test both >> > code paths on my BSD machine by modifying the ifdefs. I will have >> > EnterpriseDB do further testing. >> >> Err, why not 9.0? > > The check did not exist in 9.0 -- I mentioned that in the commit > message. I could add the check into 9.0, but we usually don't backpatch > such things. What do you mean by "such things"? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas wrote: > On Sun, Jul 24, 2011 at 5:27 PM, Bruce Momjian <bruce@momjian.us> wrote: > > Alvaro Herrera wrote: > >> Excerpts from Bruce Momjian's message of dom jul 24 01:46:08 -0400 2011: > >> > Robert Haas wrote: > >> > >> > > > Should I fix this in pg_upgrade 9.1 for Windows or just in 9.2? ?The > >> > > > check works fine on non-Windows. > >> > > > >> > > Seems worth back-patching to me. > >> > > >> > Attached patch applied and backpatched to 9.1. ?I was able to test both > >> > code paths on my BSD machine by modifying the ifdefs. ?I will have > >> > EnterpriseDB do further testing. > >> > >> Err, why not 9.0? > > > > The check did not exist in 9.0 -- I mentioned that in the commit > > message. ?I could add the check into 9.0, but we usually don't backpatch > > such things. > > What do you mean by "such things"? The check to see if the directory is writable was only in 9.1+ --- this is a fix for that check. The check was not backpatched because we don't ordinarly backpatch sanity checks for uncommon problems. We certainly can't backpatch just the fix. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +