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