Adds a Configure question for selecting whether you want taint supported #19541
Conversation
This adds a Configure question for whether you want taint support. It defaults to "yes", so that ./Configure -des will build a perl which supports taint in the usual way. If you say "no", then perl is compiled with -DSILENT_NO_TAINT_SUPPORT so that taint features silently do nothing. I've submitted a separate pull request on perl/metaconfig, which adds the underlying metaconfig unit for this question, which was used to build this Configure script.
The central doc change is in perlsec.pod. This not only explains that you can build a perl that doesn't support taint, but shows how you can check whether your perl supports taint or not. The other doc changes are mainly to note that taint might not be supported, and to refer the reader to perlsec for more details.
| @@ -34,6 +35,9 @@ use warnings; | |||
| my $is_ebcdic = ord("A") == 193; | |||
| my $os = lc $^O; | |||
|
|
|||
| # Configure now lets you build a perl that silently ignores taint features | |||
| my $NoTaintSupport = exists($Config{taint_support}) && !$Config{taint_support}; | |||
There was a problem hiding this comment.
This exists check shouldn't be necessary, as this test is core-only
There was a problem hiding this comment.
While not necessary in core, I think it's worth leaving as-is, in case people look in core files to see how to check for taint support in their modules. I've documented it in perlsec, and will blog about it, but I reckon some people will just just look for examples in the perl dist.
|
It feels to me that instead of having a configure key 'taint_support' that it should have been 'taint_disabled', which would be a much simpler check to implement and wouldn't need the exists check. Not saying this is a show stopper, but it feels backwards to introduce a new variable that you have to test for with exists to see if its defined. Is there a reason you did it this way? |
That is what I initially created, but on reviewing with @Tux, he asked me to switch the sense, saying it fit better with Configure precedent, and was less likely to confuse people. I bought his argument. |
|
The four CPAN upstream distributions (Encode, Scalar-List-Utils, Text-ParseWords, and Test-Harness) have now all had their PRs merged, so this can now be merged, ignoring the four commits marked "DO NOT MERGE". I'd like to make the case for this to be merged in time for 5.35.11 so it can be included in 5.36.0:
|
|
I just looked at the Configure related changes, and those LGTM. |
|
I think this is probably ready to go in now. |
|
With Rik's help I just dropped the commits for the four CPAN-upstream dual-life modules, so this can now be merged as-is. |
|
On Thu, 14 Apr 2022 at 17:33, Neil Bowers ***@***.***> wrote:
It feels to me that instead of having a configure key 'taint_support' that
it should have been 'taint_disabled', which would be a much simpler check
to implement and wouldn't need the exists check. [...] Is there a reason
you did it this way?
That is what I initially created, but on reviewing with @Tux
<https://github.com/Tux>, he asked me to switch the sense, saying it fit
better with Configure precedent, and was less likely to confuse people. I
bought his argument.
I think it is very much the opposite. This is disabling something that
used to always be on. Adding a new property that says it is on just means
you have the complexity of telling apart two falses, key exists and it is
false and key doesn't exist (which is also a false) which actually means it
is true. It will make everyone /else/ have to deal with the complexity,
instead of dealing with it in one place neatly.
Also it is confusing and basically a lie. I see "taint_support" => "define"
in Configure, and I go to look what defined it, and it turns out nothing
did, and in fact there is no such setting, and I have to define something
else to turn it off, something that isn't mentioned in Configure, and when
I define it, it doesn't show up in %Config instead the flag that doesn't
exist says its no longer defined. That seems pretty backwards don't you
think?
Consider,
Configure -des -Accflags='-DNO_TAINT_SUPPORT'
I will not see 'no_taint_support' in configure, instead I will see
'taint_support' => ''. Doesn't that strike you as odd? Especially as
NO_TAINT_SUPPORT will show up in the list of compile time options in perl
-V.
We shouldnt lie to our users about what is defined or not when we build.
That is super confusing.
What is the precedent where we say a setting that doesn't exist is defined
when a setting that does exist is not defined? And or vice versa, what is
the precedent where we say a setting that doesn't exist is not defined when
a setting that does exist is defined? (This paragraph shows how confusing
this would be.)
Dealing with this in metaconfig/Configure/Build time is a one time
investment, if this gets merged as is we will have to live with it forever.
I think that would be a mistake.
Maybe you can recover without having to redo the Configure stuff in
metaconfig, and add a new key to the configure template in perl that is
called no_taint_support that shows the true (opposite) value of
'taint_support'. That is my first thought to fix this.
cheers,
Yves
…--
perl -Mre=debug -e "/just|another|perl|hacker/"
|
Yeah, the current solution optimizes for |
This adds a Configure question for whether you want to build perl with or without taint support.
It defaults to "yes", but you can build a perl without taint support by running:
This results in perl being compiled with the previously added
SILENT_NO_TAINT_SUPPORTsymbol, which means that taint features silently do nothing.Previously though, if you compiled with that option, a bunch of tests would fail, and there was no easy way to tell from Perl land whether your perl supports taint. Now there's a Configure variable
taint_support, which you can query in%Config.This PR fixes all relevant test suites so that relevant tests are skipped under a taint-free perl. I've included commits for the CPAN-upstream dual-life modules, but they shouldn't be merged. I've submitted pull requests on the four affected distributions (Encode, Scalar-List-Utils, Text-ParseWords, and Test-Harness), so the changes should make it to blead via CPAN releases on those.
The change to Configure came from changes to metaconfig, which I've submitted a separate pull request on.