Re: Supporting TAP tests with MSVC and Windows

On Sat, Jul 25, 2015 at 4:14 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Fri, Jul 24, 2015 at 08:27:42PM +0300, Heikki Linnakangas wrote:
>> On 06/25/2015 07:40 AM, Michael Paquier wrote:
>> >Attached is v7, rebased on 0b157a0.
>>
>> Thanks! I fiddled with this a bit more, to centralize more of the
>> platform-dependent stuff to RewindTest.pm. Also, Windows doesn't have "cat"
>> and "touch" if you haven't installed MinGW, so I replaced those calls with
>> built-in perl code.
>
> My main priority for this patch is to not reintroduce CVE-2014-0067. The
> patch is operating in a security minefield:

Thanks!

>> --- a/src/bin/pg_ctl/t/001_start_stop.pl
>> +++ b/src/bin/pg_ctl/t/001_start_stop.pl
>> @@ -15,13 +15,9 @@ command_exit_is([ 'pg_ctl', 'start', '-D', "$tempdir/nonexistent" ],
>>
>> command_ok([ 'pg_ctl', 'initdb', '-D', "$tempdir/data" ], 'pg_ctl initdb');
>> command_ok(
>> - [ "$ENV{top_builddir}/src/test/regress/pg_regress", '--config-auth',
>> + [ $ENV{PG_REGRESS}, '--config-auth',
>> "$tempdir/data" ],
>> 'configure authentication');
>> -open CONF, ">>$tempdir/data/postgresql.conf";
>> -print CONF "listen_addresses = ''\n";
>> -print CONF "unix_socket_directories = '$tempdir_short'\n";
>> -close CONF;
>> command_ok([ 'pg_ctl', 'start', '-D', "$tempdir/data", '-w' ],
>> 'pg_ctl start -w');
>
> Since this series of tests doesn't use standard_initdb, the deleted lines
> remain necessary.

Added with a switch on $config{osname}.

>> @@ -303,7 +297,6 @@ sub run_pg_rewind
>> }
>> else
>> {
>> -
>> # Cannot come here normally
>
> Unrelated change.

Removed.

>> sub standard_initdb
>> {
>> my $pgdata = shift;
>> system_or_bail('initdb', '-D', "$pgdata", '-A' , 'trust', '-N');
>> - system_or_bail("$ENV{top_builddir}/src/test/regress/pg_regress",
>> - '--config-auth', $pgdata);
>> + system_or_bail($ENV{PG_REGRESS}, '--config-auth', $pgdata);
>> +
>> + my $tempdir_short = tempdir_short;
>> +
>> + open CONF, ">>$pgdata/postgresql.conf";
>> + print CONF "\n# Added by TestLib.pm)\n";
>> + if ($Config{osname} eq "MSWin32")
>> + {
>> + print CONF "listen_addresses = '127.0.0.1'\n";
>> + }
>> + else
>> + {
>> + print CONF "unix_socket_directories = '$tempdir_short'\n";
>
> This second branch needs listen_addresses=''; it doesn't help to secure the
> socket directory if the server still listens on localhost.

Yep. Agreed.

>> +sub configure_hba_for_replication
>> +{
>> + my $pgdata = shift;
>> +
>> + open HBA, ">>$pgdata/pg_hba.conf";
>> + print HBA "\n# Allow replication (set up by TestLib.pm)\n";
>> + if ($Config{osname} ne "MSWin32")
>> + {
>> + print HBA "local replication all trust\n";
>> + }
>> + else
>> + {
>> + print HBA "host replication all 127.0.0.1/32 sspi include_realm=1 map=regress\n";
>> + print HBA "host replication all ::1/128 sspi include_realm=1 map=regress\n";
>
> The second line will make the server fail to start on non-IPv6 systems. You
> shouldn't need it if the clients always connect to 127.0.0.1, not "localhost".

Done.

>> + configure_for_replication("$tempdir/pgdata");
>
> That function name appears nowhere else.

This looks like dead code to me. Hence removed.

>> +sub tapcheck
>> +{
>> + InstallTemp();
>> +
>> + my @args = ( "prove", "--verbose", "t/*.pl");
>> +
>> + $ENV{PATH} = "$tmp_installdir/bin;$ENV{PATH}";
>> + $ENV{PERL5LIB} = "$topdir/src/test/perl;$ENV{PERL5LIB}";
>> + $ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
>> +
>> + # Find out all the existing TAP tests by simply looking for t/
>> + # in the tree.
>
> This target shall not run the SSL suite, for the same reason check-world shall
> not run it. We could offer a distinct vcregress.pl target for TAP suites
> excluded from check-world.

OK, for the sake of duplicating what GNU does, let's simply ignore it then.

Also I added an equivalent of --enable-tap-tests for this MSVC stuff,
removed afterwards by Heikki. Do we want it or not?

An updated patch is attached.
--
Michael