pjf / ipc-system-simple

Perl module to make running system commands and capturing errors as simple as possible.
http://search.cpan.org/perldoc?IPC::System::Simple
Other
19 stars 20 forks source link

skip taint tests when perl was built without taint support #43

Open DrHyde opened 1 year ago

DrHyde commented 1 year ago

If perl is built without taint support (ie using -Accflags=-DSILENT_NO_TAINT_SUPPORT or -Accflags=-DNO_TAINT_SUPPORT) the existing tests fail. This patch makes the taint tests skip if taint-mode isn't supported.

shlomif commented 1 year ago

:+1: looks great

jkeenan commented 1 year ago

If perl is built without taint support (ie using -Accflags=-DSILENT_NO_TAINT_SUPPORT or -Accflags=-DNO_TAINT_SUPPORT) the existing tests fail. This patch makes the taint tests skip if taint-mode isn't supported.

I built a perl from blead with NO_TAINT_SUPPORT:

$ ./bin/perl -Ilib -v | head -2 |tail -1
This is perl 5, version 37, subversion 12 (v5.37.12 (v5.37.11-5-gcf4572e56a)) built for x86_64-linux
$ ./bin/perl -Ilib -V:config_args
config_args='-des -Dusedevel -Accflags=-DNO_TAINT_SUPPORT -Dprefix=/home/jkeenan/testing/blead -Uversiononly -Dman1dir=none -Dman3dir=none';
$ ./bin/perl -Ilib -V | grep -i taint
    config_args='-des -Dusedevel -Accflags=-DNO_TAINT_SUPPORT -Dprefix=/home/jkeenan/testing/blead -Uversiononly -Dman1dir=none -Dman3dir=none'
    ccflags ='-DNO_TAINT_SUPPORT -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'
    cppflags='-DNO_TAINT_SUPPORT -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
    NO_TAINT_SUPPORT

I then fetched your pull request onto my local machine and ran it against this taint-less perl.

commit ae5c5abc40c4709d7405387d9bae684cc7910c7e (HEAD -> drhyde-taint-support)
Author:     David Cantrell <david@cantrell.org.uk>
AuthorDate: Sat Apr 22 18:26:51 2023 +0100
Commit:     David Cantrell <david@cantrell.org.uk>
CommitDate: Sat Apr 22 18:26:51 2023 +0100

    skip taint tests when perl was built without taint support
...

$ bleadperl -V:config_args
config_args='-des -Dusedevel -Accflags=-DNO_TAINT_SUPPORT -Dprefix=/home/jkeenan/testing/blead -Uversiononly -Dman1dir=none -Dman3dir=none';

$ bleadperl Makefile.PL && make
Checking if your kit is complete...
Looks good
Generating a Unix-style Makefile
Writing Makefile for IPC::System::Simple
Writing MYMETA.yml and MYMETA.json
cp lib/IPC/System/Simple.pm blib/lib/IPC/System/Simple.pm

$ make test
PERL_DL_NONLAZY=1 "/home/jkeenan/testing/blead/bin/perl" "-MExtUtils::Command::MM" "-MTest::Harness" "-e" "undef *Test::Harness::Switches; test_harness(0, 'blib/lib', 'blib/arch')" t/*.t
t/01_load.t ........... ok   
t/02_exit.t ........... ok     
t/03_signal.t ......... ok   
t/04_capture.t ........ ok     
t/05_multi_capture.t .. ok     
t/06_fail.t ........... ok   
t/07_taint.t .......... This perl was compiled without taint support. Cowardly refusing to run with -t or -T flags.
t/07_taint.t .......... Dubious, test returned 29 (wstat 7424, 0x1d00)
No subtests run 
t/08_core.t ........... skipped: BSD::Resource required for coredump tests
t/09_system.t ......... ok     
t/10_formatting.t ..... This perl was compiled without taint support. Cowardly refusing to run with -t or -T flags.
t/10_formatting.t ..... Dubious, test returned 29 (wstat 7424, 0x1d00)
No subtests run 
t/11_newlines.t ....... ok   
t/12_systemx.t ........ ok   
t/13_exports.t ........ ok   
t/14_uninitialised.t .. skipped: Test::NoWarnings required for testing undef warnings
t/args.t .............. ok     
t/internal.t .......... ok   
t/win32.t ............. skipped: Win32 only tests

Test Summary Report
-------------------
t/07_taint.t        (Wstat: 7424 (exited 29) Tests: 0 Failed: 0)
  Non-zero exit status: 29
  Parse errors: No plan found in TAP output
t/10_formatting.t   (Wstat: 7424 (exited 29) Tests: 0 Failed: 0)
  Non-zero exit status: 29
  Parse errors: No plan found in TAP output
Files=17, Tests=163,  1 wallclock secs ( 0.04 usr  0.01 sys +  0.50 cusr  0.11 csys =  0.66 CPU)
Result: FAIL
Failed 2/17 test programs. 0/163 subtests failed.
make: *** [Makefile:887: test_dynamic] Error 255

Your commit message stated, This patch makes the taint tests skip if taint-mode isn't supported. But these tests printed out a message indicating skipping but two test files nonetheless FAILed. That's confusing.

Additional note: This time last year @neilb had submitted https://github.com/pjf/ipc-system-simple/pull/39 on the same problem. We closed it then because it ran into other problems. We'll need to evaluate this p.r. in light of those problems.

DrHyde commented 1 year ago

That's odd, I thought I'd tested it with both -DSILENT_NO_TAINT_SUPPORT and -DNO_TAINT_SUPPORT. Let me try again ...

DrHyde commented 1 year ago

Ah-ha, I had two builds with SILENT_NO_TAINT_SUPPORT! I'll update the PR to also work with the noisy version of NO_TAINT_SUPPORT

DrHyde commented 1 year ago

IIRC @neilb's work in blead was about adding a question in Configure to ask whether to build perl without taint support. That was reverted. Then more recently some patches went in to at least make perl pass its own tests without taint support, even though there's still no question for it in Configure, and also for Config.pm to know about it - see https://github.com/Perl/perl5/pull/20983.

Now that perl passes its own tests if you disable taint support via -Accflags=..., I'm running a CPAN smoker with it, and slowly working my way through all the "up river" modules that fail on it - hence this PR.

jkeenan commented 1 year ago

On 4/23/23 10:24, David Cantrell wrote:

[snip]

Now that perl passes its own tests if you disable taint support via |-Accflags=...|, I'm running a CPAN smoker with it, and slowly working my way through all the "up river" modules that fail on it - hence this PR.

My initial reaction to your second commit was, "Wow, that's an awfully big lot of new code just to maintain the same functionality! And now a diligent CPAN maintainer has to test any taint-related code against one (or possibly two) different Perl builds!"

Thanks for undertaking this investigation. I will hope to get to studying it for this distro soon. I just wonder how many CPAN maintainers will be willing to undertake similar adaptive work in their distros.

DrHyde commented 1 year ago

It's actually very little code, the contents of 07_taint.t and 10_formatting.t have been moved into non-.t files so that a perl built with the noisy version of NO_TAINT_SUPPORT won't ever see them and so won't blow chunks. The new versions of 07_taint.t and 10_formatting.t just check to see if taint-mode is supported and either exec() those renamed files or say "nope, skipping".