Re: [BUGS] 9.2 beta2 - pg_ctl crashes on Win32 when neither PGDATA nor -D specified
Please Find the patch based on idea I have suggested attached with this mail. Let me know your comments regarding the same. -Original Message- From: pgsql-bugs-ow...@postgresql.org [mailto:pgsql-bugs-ow...@postgresql.org] On Behalf Of Alvaro Herrera Sent: Wednesday, June 13, 2012 9:23 PM To: Amit Kapila Cc: 'Edmund Horner'; Tom Lane; Pg Bugs; Bruce Momjian Subject: Re: [BUGS] 9.2 beta2 - pg_ctl crashes on Win32 when neither PGDATA nor -D specified Excerpts from Amit Kapila's message of mié jun 13 00:53:47 -0400 2012: > > Unfortunately in src/backend/main/main.c it only does a cursory check > > for --help and --version. So it would need to become a little more > > complicated to scan for -C options at that stage. It's not too much > > if you can assume -C always appears first like the other special > > options detected in that file. > > I am doubtful whether we should make such an exception for -C option, as > this will > be a change in behavior as compare to previous versions. > How to do in code is next step. > According to me the solution I have proposed is safer and already initdb > handles in same way. > > I am waiting for other people opinion on this issue. I agree with you. The fact that we drop privileges is not only a security measure; it's a robustness one as well. With the current setup, we can confidently say "it's not Postgres' fault" when the system crashes with some weird kernel error. A process running with administrative privs is capable of doing privileged stuff that may override safe interfaces provided by the operating system; a process without admin privs is more constrained and should not be able to cause the system to crash. Any kernel crash, then, is not our responsibility. If we allow -C to run with admin privs, we lose that. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs pgctldefectfix.patch Description: Binary data -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] 9.2 beta2 - pg_ctl crashes on Win32 when neither PGDATA nor -D specified
Excerpts from Amit Kapila's message of mié jun 13 00:53:47 -0400 2012: > > Unfortunately in src/backend/main/main.c it only does a cursory check > > for --help and --version. So it would need to become a little more > > complicated to scan for -C options at that stage. It's not too much > > if you can assume -C always appears first like the other special > > options detected in that file. > > I am doubtful whether we should make such an exception for -C option, as > this will > be a change in behavior as compare to previous versions. > How to do in code is next step. > According to me the solution I have proposed is safer and already initdb > handles in same way. > > I am waiting for other people opinion on this issue. I agree with you. The fact that we drop privileges is not only a security measure; it's a robustness one as well. With the current setup, we can confidently say "it's not Postgres' fault" when the system crashes with some weird kernel error. A process running with administrative privs is capable of doing privileged stuff that may override safe interfaces provided by the operating system; a process without admin privs is more constrained and should not be able to cause the system to crash. Any kernel crash, then, is not our responsibility. If we allow -C to run with admin privs, we lose that. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] 9.2 beta2 - pg_ctl crashes on Win32 when neither PGDATA nor -D specified
On Wed, Jun 13, 2012 at 04:08:20PM +0100, Dave Page wrote: > On Wed, Jun 13, 2012 at 3:51 PM, Bruce Momjian wrote: > > On Mon, Jun 11, 2012 at 10:23:00PM -0400, Tom Lane wrote: > >> Hm, that patch seems to be several bricks shy of a load. I will fix > >> two obvious bugs in it: > >> > >> (1) not dump core on boxes where printf("%s", NULL) dumps core; > >> > >> (2) not try to call adjust_data_dir before complaining for lack of > >> a -D switch; since adjust_data_dir does not do anything to the value > >> of pg_config, it's just silly to do things in that order. > >> > >> However, > >> > >> > I note that "postgres -C data_directory" will refuse to run on the > >> > command line because I've got admin privileges in Windows, and that > >> > pg_ctl normally starts postgres.exe using CreateRestrictedProcess. > >> > But it does not do so for the popen call in adjust_data_dir. > >> > >> if that actually is a third bug, as seems likely, somebody with access > >> to a windows environment will need to deal with it. > > > > I think you have to use RUNAS when using 'pg_ctl' or 'postgres' on Windows. > > pg_ctl should work as an administrative user, as it'll irrevocably > drop unwanted privileges. postgres won't though. The user was geting an error trying to start the postmaster, which I think matches your description. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] 9.2 beta2 - pg_ctl crashes on Win32 when neither PGDATA nor -D specified
On Wed, Jun 13, 2012 at 3:51 PM, Bruce Momjian wrote: > On Mon, Jun 11, 2012 at 10:23:00PM -0400, Tom Lane wrote: >> Hm, that patch seems to be several bricks shy of a load. I will fix >> two obvious bugs in it: >> >> (1) not dump core on boxes where printf("%s", NULL) dumps core; >> >> (2) not try to call adjust_data_dir before complaining for lack of >> a -D switch; since adjust_data_dir does not do anything to the value >> of pg_config, it's just silly to do things in that order. >> >> However, >> >> > I note that "postgres -C data_directory" will refuse to run on the >> > command line because I've got admin privileges in Windows, and that >> > pg_ctl normally starts postgres.exe using CreateRestrictedProcess. >> > But it does not do so for the popen call in adjust_data_dir. >> >> if that actually is a third bug, as seems likely, somebody with access >> to a windows environment will need to deal with it. > > I think you have to use RUNAS when using 'pg_ctl' or 'postgres' on Windows. pg_ctl should work as an administrative user, as it'll irrevocably drop unwanted privileges. postgres won't though. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] 9.2 beta2 - pg_ctl crashes on Win32 when neither PGDATA nor -D specified
On Mon, Jun 11, 2012 at 10:23:00PM -0400, Tom Lane wrote: > Hm, that patch seems to be several bricks shy of a load. I will fix > two obvious bugs in it: > > (1) not dump core on boxes where printf("%s", NULL) dumps core; > > (2) not try to call adjust_data_dir before complaining for lack of > a -D switch; since adjust_data_dir does not do anything to the value > of pg_config, it's just silly to do things in that order. > > However, > > > I note that "postgres -C data_directory" will refuse to run on the > > command line because I've got admin privileges in Windows, and that > > pg_ctl normally starts postgres.exe using CreateRestrictedProcess. > > But it does not do so for the popen call in adjust_data_dir. > > if that actually is a third bug, as seems likely, somebody with access > to a windows environment will need to deal with it. I think you have to use RUNAS when using 'pg_ctl' or 'postgres' on Windows. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] 9.2 beta2 - pg_ctl crashes on Win32 when neither PGDATA nor -D specified
> Unfortunately in src/backend/main/main.c it only does a cursory check > for --help and --version. So it would need to become a little more > complicated to scan for -C options at that stage. It's not too much > if you can assume -C always appears first like the other special > options detected in that file. I am doubtful whether we should make such an exception for -C option, as this will be a change in behavior as compare to previous versions. How to do in code is next step. According to me the solution I have proposed is safer and already initdb handles in same way. I am waiting for other people opinion on this issue. Please suggest whether this problem needs to be fixed and what is best way to fix it among below or it should be fixed in some other way. 1. pg_ctl invoke itself in a restricted mode, similar to initdb. 2. postgres to handle -C calls without checking if it's running as root. -Original Message- From: Edmund Horner [mailto:ejr...@gmail.com] Sent: Wednesday, June 13, 2012 6:26 AM To: Amit Kapila Cc: Tom Lane; pgsql-bugs@postgresql.org; Bruce Momjian Subject: Re: [BUGS] 9.2 beta2 - pg_ctl crashes on Win32 when neither PGDATA nor -D specified On 13 June 2012 00:54, Amit Kapila wrote: >>> I note that "postgres -C data_directory" will refuse to run on the >>> command line because I've got admin privileges in Windows, and that >>> pg_ctl normally starts postgres.exe using CreateRestrictedProcess. >>> But it does not do so for the popen call in adjust_data_dir. > >> if that actually is a third bug, as seems likely, somebody with access >> to a windows environment will need to deal with it. > > I am able to reproduce this problem, "that pg_ctl throws error for > administrative user in the mentioned code path". > > One solution to this problem is that pg_ctl invoke itself in a restricted > mode, similar to initdb. > This will allow popen call to be successful in pg_ctl code path. Perhaps an alternative solution is to get postgres to handle -C calls without checking if it's running as root. The command does not do much more than read the config file, print a value from it, and exit. Unfortunately in src/backend/main/main.c it only does a cursory check for --help and --version. So it would need to become a little more complicated to scan for -C options at that stage. It's not too much if you can assume -C always appears first like the other special options detected in that file. But you might not want to make an exception for -C. And I realise the check is a security feature, and it's best to err on the side of defensive programming and maintainability. Edmund. -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] 9.2 beta2 - pg_ctl crashes on Win32 when neither PGDATA nor -D specified
On 13 June 2012 00:54, Amit Kapila wrote: >>> I note that "postgres -C data_directory" will refuse to run on the >>> command line because I've got admin privileges in Windows, and that >>> pg_ctl normally starts postgres.exe using CreateRestrictedProcess. >>> But it does not do so for the popen call in adjust_data_dir. > >> if that actually is a third bug, as seems likely, somebody with access >> to a windows environment will need to deal with it. > > I am able to reproduce this problem, "that pg_ctl throws error for > administrative user in the mentioned code path". > > One solution to this problem is that pg_ctl invoke itself in a restricted > mode, similar to initdb. > This will allow popen call to be successful in pg_ctl code path. Perhaps an alternative solution is to get postgres to handle -C calls without checking if it's running as root. The command does not do much more than read the config file, print a value from it, and exit. Unfortunately in src/backend/main/main.c it only does a cursory check for --help and --version. So it would need to become a little more complicated to scan for -C options at that stage. It's not too much if you can assume -C always appears first like the other special options detected in that file. But you might not want to make an exception for -C. And I realise the check is a security feature, and it's best to err on the side of defensive programming and maintainability. Edmund. -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] 9.2 beta2 - pg_ctl crashes on Win32 when neither PGDATA nor -D specified
>> I note that "postgres -C data_directory" will refuse to run on the >> command line because I've got admin privileges in Windows, and that >> pg_ctl normally starts postgres.exe using CreateRestrictedProcess. >> But it does not do so for the popen call in adjust_data_dir. > if that actually is a third bug, as seems likely, somebody with access > to a windows environment will need to deal with it. I am able to reproduce this problem, "that pg_ctl throws error for administrative user in the mentioned code path". One solution to this problem is that pg_ctl invoke itself in a restricted mode, similar to initdb. This will allow popen call to be successful in pg_ctl code path. Please let me know if this solution is okay, I can create the patch for it. For Referrence initdb code is as below, we can have similar code for pg_ctl: #ifdef WIN32 /* * Before we execute another program, make sure that we are running with a * restricted token. If not, re-execute ourselves with one. */ if ((restrict_env = getenv("PG_RESTRICT_EXEC")) == NULL || strcmp(restrict_env, "1") != 0) { PROCESS_INFORMATION pi; char *cmdline; ZeroMemory(&pi, sizeof(pi)); cmdline = xstrdup(GetCommandLine()); putenv("PG_RESTRICT_EXEC=1"); if (!CreateRestrictedProcess(cmdline, &pi)) { fprintf(stderr, "Failed to re-exec with restricted token: %lu.\n", GetLastError()); } else { /* * Successfully re-execed. Now wait for child process to capture * exitcode. */ DWORDx; CloseHandle(pi.hThread); WaitForSingleObject(pi.hProcess, INFINITE); if (!GetExitCodeProcess(pi.hProcess, &x)) { fprintf(stderr, "Failed to get exit code from subprocess: %lu\n", GetLastError()); exit(1); } exit(x); } } #endif -Original Message- From: pgsql-bugs-ow...@postgresql.org [mailto:pgsql-bugs-ow...@postgresql.org] On Behalf Of Tom Lane Sent: Tuesday, June 12, 2012 7:53 AM To: Edmund Horner Cc: pgsql-bugs@postgresql.org; Bruce Momjian Subject: Re: [BUGS] 9.2 beta2 - pg_ctl crashes on Win32 when neither PGDATA nor -D specified Edmund Horner writes: > In 9.1, if I run "pg_ctl start" without providing way for it to find > the datadir, it prints the error: > C:\ehorner\pgsql-old\bin>pg_ctl start > pg_ctl: no database directory specified and environment variable > PGDATA unset > Try "pg_ctl --help" for more information. > In 9.2 (beta1 and beta2), it runs for a couple of seconds and then > Windows pops up an error box saying it has "encountered a problem". I > ... > I think it could be something in > http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=aaa6e1def2 92cdacb6b27088898793b1b879fedf#patch5 Hm, that patch seems to be several bricks shy of a load. I will fix two obvious bugs in it: (1) not dump core on boxes where printf("%s", NULL) dumps core; (2) not try to call adjust_data_dir before complaining for lack of a -D switch; since adjust_data_dir does not do anything to the value of pg_config, it's just silly to do things in that order. However, > I note that "postgres -C data_directory" will refuse to run on the > command line because I've got admin privileges in Windows, and that > pg_ctl normally starts postgres.exe using CreateRestrictedProcess. > But it does not do so for the popen call in adjust_data_dir. if that actually is a third bug, as seems likely, somebody with access to a windows environment will need to deal with it. regards, tom lane -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] 9.2 beta2 - pg_ctl crashes on Win32 when neither PGDATA nor -D specified
On 12 June 2012 14:51, Tom Lane wrote: > Edmund Horner writes: >> On 12 June 2012 14:23, Tom Lane wrote: >>> Hm, that patch seems to be several bricks shy of a load. I will fix >>> two obvious bugs in it: >>> (1) not dump core on boxes where printf("%s", NULL) dumps core; > >> I saw that, but I couldn't decide if it was the actual problem in my >> case. Later parts of adjust_data_dir appeared to be running (looking >> at file system activity). > > The weird thing is that if it's not dumping core, the first fopen would > presumably be trying to open "/postgresql.conf", which generally ought > to fail and thus mask any problems in the rest of adjust_data_dir(). > Have you by any chance got a postgresql.conf laying about in your root > directory? I don't, and I couldn't see any file system requests for postgresql.conf either (though I thought that might be because it's failing to find a bogus parent path first e.g. something like "(null)/postgresql.conf". But I think it's just a problem in the snprintf statements after all: I've managed to run it in the MSVC debugger, using the packaged pdb files. So it does seem to be crashing on the first snprintf statement. pg_ctl.exe!fmtstr(char * value=0x, int leftjust=0, int minlen=1, int maxwidth=0, int pointflag=0, PrintfTarget * target=0x) Line 779 + 0x6 bytesC pg_ctl.exe!dopr(PrintfTarget * target=0x0081f72c, const char * format=0x0040b948, char * args=0x) Line 715 + 0x1c bytes C pg_ctl.exe!pg_snprintf(char * str=0x0081fb5c, unsigned int count=1024, const char * fmt=0x0040b948, ...) Line 171 + 0x35 bytesC > pg_ctl.exe!adjust_data_dir() Line 1902 C pg_ctl.exe!main(int argc=2, char * * argv=0x00da4fe0) Line 2189 C pg_ctl.exe!__tmainCRTStartup() Line 555 + 0x17 bytes C kernel32.dll!7c817067() [Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll] I'm not sure I can find the exact source version pg_ctl.c that was compiled -- I get an off-by-one error around line 1902 using the source from a few days ago. 1900/* If there is no postgresql.conf, it can't be a config-only dir */ 1901snprintf(filename, sizeof(filename), "%s/postgresql.conf", pg_config); 1902if ((fd = fopen(filename, "r")) == NULL) -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] 9.2 beta2 - pg_ctl crashes on Win32 when neither PGDATA nor -D specified
Edmund Horner writes: > On 12 June 2012 14:23, Tom Lane wrote: >> Hm, that patch seems to be several bricks shy of a load. I will fix >> two obvious bugs in it: >> (1) not dump core on boxes where printf("%s", NULL) dumps core; > I saw that, but I couldn't decide if it was the actual problem in my > case. Later parts of adjust_data_dir appeared to be running (looking > at file system activity). The weird thing is that if it's not dumping core, the first fopen would presumably be trying to open "/postgresql.conf", which generally ought to fail and thus mask any problems in the rest of adjust_data_dir(). Have you by any chance got a postgresql.conf laying about in your root directory? regards, tom lane -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] 9.2 beta2 - pg_ctl crashes on Win32 when neither PGDATA nor -D specified
On 12 June 2012 14:23, Tom Lane wrote: > Edmund Horner writes: >> In 9.1, if I run "pg_ctl start" without providing way for it to find >> the datadir, it prints the error: > >> C:\ehorner\pgsql-old\bin>pg_ctl start >> pg_ctl: no database directory specified and environment variable >> PGDATA unset >> Try "pg_ctl --help" for more information. > >> In 9.2 (beta1 and beta2), it runs for a couple of seconds and then >> Windows pops up an error box saying it has "encountered a problem". I >> ... >> I think it could be something in >> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=aaa6e1def292cdacb6b27088898793b1b879fedf#patch5 > > Hm, that patch seems to be several bricks shy of a load. I will fix > two obvious bugs in it: > > (1) not dump core on boxes where printf("%s", NULL) dumps core; I saw that, but I couldn't decide if it was the actual problem in my case. Later parts of adjust_data_dir appeared to be running (looking at file system activity). Anyway I'm happy to test it again when there's a new binary. Edmund. -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] 9.2 beta2 - pg_ctl crashes on Win32 when neither PGDATA nor -D specified
Edmund Horner writes: > In 9.1, if I run "pg_ctl start" without providing way for it to find > the datadir, it prints the error: > C:\ehorner\pgsql-old\bin>pg_ctl start > pg_ctl: no database directory specified and environment variable > PGDATA unset > Try "pg_ctl --help" for more information. > In 9.2 (beta1 and beta2), it runs for a couple of seconds and then > Windows pops up an error box saying it has "encountered a problem". I > ... > I think it could be something in > http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=aaa6e1def292cdacb6b27088898793b1b879fedf#patch5 Hm, that patch seems to be several bricks shy of a load. I will fix two obvious bugs in it: (1) not dump core on boxes where printf("%s", NULL) dumps core; (2) not try to call adjust_data_dir before complaining for lack of a -D switch; since adjust_data_dir does not do anything to the value of pg_config, it's just silly to do things in that order. However, > I note that "postgres -C data_directory" will refuse to run on the > command line because I've got admin privileges in Windows, and that > pg_ctl normally starts postgres.exe using CreateRestrictedProcess. > But it does not do so for the popen call in adjust_data_dir. if that actually is a third bug, as seems likely, somebody with access to a windows environment will need to deal with it. regards, tom lane -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
[BUGS] 9.2 beta2 - pg_ctl crashes on Win32 when neither PGDATA nor -D specified
In 9.1, if I run "pg_ctl start" without providing way for it to find the datadir, it prints the error: C:\ehorner\pgsql-old\bin>pg_ctl start pg_ctl: no database directory specified and environment variable PGDATA unset Try "pg_ctl --help" for more information. In 9.2 (beta1 and beta2), it runs for a couple of seconds and then Windows pops up an error box saying it has "encountered a problem". I can get more detail from Windows but I'll wait 'till someone asks (since I'd be surprised if it was useful). The same crash occurs for any other subcommand, such as "stop" or "register", etc., except for "pg_ctl kill" which says it needs missing arguments; it crashes when they are provided. (Kill does not require a datadir and unregister does not either (according to the help, at least) and they still crash.) No crashes occur for plain old "pg_ctl --help" or similar. I think it could be something in http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=aaa6e1def292cdacb6b27088898793b1b879fedf#patch5 where pg_config is NULL (due to the env var not existing), but adjust_data_dir is called anyway. It detects that there "must" be a configuration directory. I can't follow exactly where it might be crashing but I can report that pg_ctl seems to iterate over all the EXEs in the bin directory (the call to "find_other_exec_or_die"), and that "postgres -C data_directory" does not appear to be executed. I note that "postgres -C data_directory" will refuse to run on the command line because I've got admin privileges in Windows, and that pg_ctl normally starts postgres.exe using CreateRestrictedProcess. But it does not do so for the popen call in adjust_data_dir. Anyway that's enough speculating from me. ;-) Let me know if you need me to test anything else. Cheers, Edmund. -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs