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 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?
--
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. +
Attachments:
/rtmp/pg_upgrade.difftext/x-diffDownload
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. +