Обсуждение: BUG #16080: pg_ctl is failed if a fake cmd.exe exist in the current directory.

The following bug has been logged on the website:

Bug reference:      16080
Logged by:          cili
Email address:      cilizili@protonmail.com
PostgreSQL version: 12.0
Operating system:   Microsoft Windows [Version 10.0.18362.418]
Description:

If a fake cmd.exe exits in the current directory, pg_ctl is failed to
start.

Instructions:
# cd %TEMP%
# "c:\Program Files\PostgreSQL\12\bin\pg_ctl.exe" initdb -D test
# copy %windir%\system32\calc.exe cmd.exe
# "c:\Program Files\PostgreSQL\12\bin\pg_ctl.exe" start -D test
waiting for server to start.... stopped waiting
pg_ctl: could not start server
Examine the log output.

Expected:
The PostgreSQL is started.

Actual:
The Windows calculator instead of PostgreSQL is started.

PG Bug reporting form <noreply@postgresql.org> writes:
> If a fake cmd.exe exits in the current directory, pg_ctl is failed to
> start.

I do not think this is a bug.  There are probably thousands of ways to
break Postgres by misconfiguring your system, and this is one of 'em.

(Likewise for your identical complaint about pg_upgrade.)

            regards, tom lane

=?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <juanjo.santamaria@gmail.com> writes:
> There is a difference in the behaviour for the WIN32 port. In other
> platforms execl() is called from "src/bin/pg_ctl/pc_ctl" with the full path
> ("/bin/sh"), but for WIN32 the function CreateProcessAsUser() calls the CMD
> command without a path, and this function has a search logic of its own [1].

Right, but does cmd.exe have a well-defined location in Windows?
I don't think we can know which drive it's on, for starters.

Ultimately this seems like a problem of insecure search path,
which is not our responsibility to fix (and people would not
appreciate us trying, in many cases).

            regards, tom lane

=?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <juanjo.santamaria@gmail.com> writes:
> On Sat, Oct 26, 2019 at 5:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Right, but does cmd.exe have a well-defined location in Windows?
>> I don't think we can know which drive it's on, for starters.

> The environment variable COMSPEC [1] should point to the right location.

Hm.  I don't have any objection to using COMSPEC if it's set, but
of course that changes nothing from a security perspective.  It's
just a different route by which pg_ctl, pg_upgrade, etc can be
misled.

            regards, tom lane

=?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <juanjo.santamaria@gmail.com> writes:
> On Sat, Oct 26, 2019 at 7:44 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hm.  I don't have any objection to using COMSPEC if it's set, but
>> of course that changes nothing from a security perspective.  It's
>> just a different route by which pg_ctl, pg_upgrade, etc can be
>> misled.

> Please find attached a patch with this simple modification.

I poked around a bit for other references to cmd.exe.  It looks
like psql's do_shell() is handling this correctly already, but should
we not also fix spawn_process() in src/test/regress/pg_regress.c ?

There are also a couple of references in pg_upgrade's test.sh,
but I don't feel a need to change those.

Another point that could be raised here: seeing that psql honors the
SHELL variable to substitute for /bin/sh, should these other programs
do likewise?  I'm inclined to think not, because what psql is doing is
launching an interactive shell, so the user's shell preference should be
honored.  In these other cases we want plain old Bourne shell thank you,
so ignoring SHELL seems correct.  But it's worth thinking about, and
perhaps adding a comment about.

            regards, tom lane

=?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <juanjo.santamaria@gmail.com> writes:
> On Sun, Oct 27, 2019 at 4:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I poked around a bit for other references to cmd.exe.  It looks
>> like psql's do_shell() is handling this correctly already, but should
>> we not also fix spawn_process() in src/test/regress/pg_regress.c ?

> Agreed, so please find attached an updated patch.

Looks OK now, so pushed.  (I don't have the ability to test this
locally, but the buildfarm will soon tell us if it's wrong.)

            regards, tom lane