Re: 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.

Edmund Horner <ejrh00(at)gmail(dot)com> 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

On 12 June 2012 14:23, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Edmund Horner <ejrh00(at)gmail(dot)com> 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.

Edmund Horner <ejrh00(at)gmail(dot)com> writes:
> On 12 June 2012 14:23, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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

On 12 June 2012 14:51, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Edmund Horner <ejrh00(at)gmail(dot)com> writes:
>> On 12 June 2012 14:23, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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=0x00000000, int leftjust=0, int
minlen=1, int maxwidth=0, int pointflag=0, PrintfTarget *
target=0x00000000) Line 779 + 0x6 bytes C
pg_ctl.exe!dopr(PrintfTarget * target=0x0081f72c, const char *
format=0x0040b948, char * args=0x00000000) Line 715 + 0x1c bytes C
pg_ctl.exe!pg_snprintf(char * str=0x0081fb5c, unsigned int
count=1024, const char * fmt=0x0040b948, ...) Line 171 + 0x35 bytes C
> 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 */
1901 snprintf(filename, sizeof(filename), "%s/postgresql.conf", pg_config);
1902 if ((fd = fopen(filename, "r")) == NULL)

>> 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.
*/
DWORD x;

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-owner(at)postgresql(dot)org
[mailto:pgsql-bugs-owner(at)postgresql(dot)org] On Behalf Of Tom Lane
Sent: Tuesday, June 12, 2012 7:53 AM
To: Edmund Horner
Cc: pgsql-bugs(at)postgresql(dot)org; Bruce Momjian
Subject: Re: [BUGS] 9.2 beta2 - pg_ctl crashes on Win32 when neither PGDATA
nor -D specified

Edmund Horner <ejrh00(at)gmail(dot)com> 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

On 13 June 2012 00:54, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> 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.

> 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:ejrh00(at)gmail(dot)com]
Sent: Wednesday, June 13, 2012 6:26 AM
To: Amit Kapila
Cc: Tom Lane; pgsql-bugs(at)postgresql(dot)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 <amit(dot)kapila(at)huawei(dot)com> 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.

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 <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Wed, Jun 13, 2012 at 3:51 PM, Bruce Momjian <bruce(at)momjian(dot)us> 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

On Wed, Jun 13, 2012 at 04:08:20PM +0100, Dave Page wrote:
> On Wed, Jun 13, 2012 at 3:51 PM, Bruce Momjian <bruce(at)momjian(dot)us> 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 <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

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 <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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-owner(at)postgresql(dot)org [mailto:pgsql-bugs-owner(at)postgresql(dot)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 <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support