Fix tests when -DSILENT_NO_TAINT_SUPPORT is defined #20972

DrHyde · 2023-03-26T19:22:56Z

This PR adds automatic testing with this option enabled, and updates the tests that fail in the lib/ and t/ directories. There are also currently some failures in cpan/ - I have raised PRs for those. You can see in the history for this branch that I fixed them in this repo to make sure the fix worked then reverted when I raised the PRs upstream. Once they've been merged upstream I will edit history to make them disappear from here:

This PR shouldn't be merged - and will fail its tests - until those have been merged upstream and then also updated in perl5-blead.

The fix in the tests is quite straightforward. Checking $Config{taint_support} is insufficient for -DSILENT_NO_TAINT_SUPPORT, so it adds checks that look for that in $Config{ccflags}.

Revert this before creating PR!

…SUPPORT is turned on

This should be reverted before merge, as Encode is dual-life CPAN-first

This should be reverted before merge, as Scalar-List-Utils is dual-life CPAN-first

This should be reverted before merge, as Test-Harness is dual-life CPAN-first

This should be reverted before merge, as Text-ParseWords is dual-life CPAN-first

This reverts commit 1e7ac3b. A PR has been created for upstream: dankogai/p5-encode#172

This reverts commit bca1c72. A PR has been created upstream: Dual-Life/Scalar-List-Utils#124

This reverts commit d4fa19e. A PR has been created for upstream: Perl-Toolchain-Gang/Test-Harness#118

This reverts commit c61fb35. A PR has been created upstream: neilb/Text-ParseWords#2

Leont · 2023-03-26T21:32:19Z

The fix in the tests is quite straightforward. Checking $Config{taint_support} is insufficient for -DSILENT_NO_TAINT_SUPPORT, so it adds checks that look for that in $Config{ccflags}.

I'm not sure that's the right conclusion. The whole point of the taint_support option was that we don't have to dive into ccflags.

DrHyde · 2023-03-26T22:08:33Z

That's the only trace I could find of the option that's available to perl after it's been built. taint_support doesn't seem to get set by anything at all - but I assume I'm missing something.

JRaspass · 2023-03-26T22:17:18Z

lib/overload.t


-my $no_taint_support = exists($Config::Config{taint_support})
-                     && !$Config::Config{taint_support};
+my $NoTaintSupport =


Why the change of naming style? All the rest of this file is snake case.

For consistency with the $NoTaintSupport in other files

Leont · 2023-03-26T22:29:29Z

That's the only trace I could find of the option that's available to perl after it's been built. taint_support doesn't seem to get set by anything at all - but I assume I'm missing something.

I think that question is somewhat answered by 28003a9

demerphq · 2023-03-27T23:44:06Z

taint_support doesn't seem to get set by anything at all - but I assume I'm missing something.

@DrHyde, @Leont: Hopefully when that patch gets resurrected, next dev cycle I guess (we/I forgot about it this dev cycle), the var wont be named "taint_support", but rather "taint_disabled".

It should never have been expressed as a positive config value given the history that we always supported taint, and that we historically had no setting to say so. Thus introducing a new setting that says we do have it now, when we always had it makes code awkward. Having two types of disablement just makes it even worse.

I believe the thinking was "in the future we will have no taint support at all", but that is an unforced error where the false state of the config value has a different meaning depending on the version of perl (eg it introduces the notion that exists-and-false != not-exists). It also makes the interim logic where there are two forms of disablement more painful, as proved to be the case from the get-go, as part of the reason the patch was not merged was if you did not use Configure's Q&A to set the define and instead set it via ccflags with something like '-Accflags=-DNO_TAINT_SUPPORT', the taint_support config value would not be set correctly. Much better to keep the setting negtive (as in taint_disabled) and when/if we remove taint entirely we just hardwire that setting to "taint_disabled=yes".

For this reason I don't think these patches should check "taint_support" at all. If anything I would check "taint_disabled" and if it is not set then I would check the ccflags. In the future 'taint_disabled' /might/ get set up without updating ccflags, but it is also conceivable it is set up AND the ccflags are setup, and historical perls will not have either "taint_support" nor "taint_disabled" and will only have the ccflags set up, so checking ccflags as a fallback makes sense no matter what.

FWIW, The taint configure patch was released fairly late in the 35.x cycle, and we didn't have a lot of time to find and fix the conceptual or implementation issues in the patch sequence until it was released. FWIW, I had meant to follow up on it in 5.37.x but it slipped my mind until I saw this PR.

demerphq · 2023-03-27T23:48:17Z

lib/locale.t

 # Configure now lets you build a perl that silently ignores taint features
-my $NoTaintSupport = exists($Config{taint_support}) && !$Config{taint_support};
+my $NoTaintSupport =
+    (exists($Config{taint_support}) && !$Config{taint_support}) ||


I would lose these checks entirely. I am very hopeful that we will never have a config variable called "taint_support".

demerphq · 2023-03-27T23:49:04Z

lib/locale.t

-my $NoTaintSupport = exists($Config{taint_support}) && !$Config{taint_support};
+my $NoTaintSupport =
+    (exists($Config{taint_support}) && !$Config{taint_support}) ||
+    $Config{ccflags} =~ /-DSILENT_NO_TAINT_SUPPORT/;


Why are you only checking SILENT_NO_TAINT_SUPPORT? Shouldn't this be

$Config{ccflags} =~ /-D(?SILENT_)?NO_TAINT_SUPPORT/;

DrHyde · 2023-03-28T07:38:23Z

@DrHyde, @Leont: Hopefully when that patch gets resurrected, next dev cycle ...

In that case I'll close this PR for now. @demerphq would you like a reminder after 5.38 is out so it doesn't get forgotten again? :-)

demerphq · 2023-03-28T09:31:49Z

would you like a reminder after 5.38 is out so it doesn't get forgotten again? :-)

Sure, can't hurt.

yves

This adds 'taint_disabled' and 'taint_support' to Config.pm and %Config. This way people can use them while we decide what to do about the changes to Configure. We shouldn't need to have Configure changed to export status variables like this in Config.pm See: Perl-Toolchain-Gang/Test-Harness#118 and: #20972 for related work that is stalled because we have not decided what to do about these variables.

This adds 'taint_disabled' and 'taint_support' to Config.pm and %Config. This way people can use them while we decide what to do about the changes to Configure. We shouldn't need to have Configure changed to export status variables like this in Config.pm See: Perl-Toolchain-Gang/Test-Harness#118 and: Perl#20972 for related work that is stalled because we have not decided what to do about these variables.

DrHyde added 12 commits March 25, 2023 22:46

Automatically test without taint support

988dc01

Force Github to run the Linux tests.

cb0a8c7

Revert this before creating PR!

Fix tests to skip those that require tainting when -DSILENT_NO_TAINT_…

f7ef137

…SUPPORT is turned on

Skip taint tests when -DSILENT_NO_TAINT_SUPPORT is enabled.

1e7ac3b

This should be reverted before merge, as Encode is dual-life CPAN-first

Skip taint tests when -DSILENT_NO_TAINT_SUPPORT is enabled.

bca1c72

This should be reverted before merge, as Scalar-List-Utils is dual-life CPAN-first

Skip taint tests when -DSILENT_NO_TAINT_SUPPORT is enabled.

d4fa19e

This should be reverted before merge, as Test-Harness is dual-life CPAN-first

Skip taint tests when -DSILENT_NO_TAINT_SUPPORT is enabled.

c61fb35

This should be reverted before merge, as Text-ParseWords is dual-life CPAN-first

Revert "Skip taint tests when -DSILENT_NO_TAINT_SUPPORT is enabled."

e4213b9

This reverts commit 1e7ac3b. A PR has been created for upstream: dankogai/p5-encode#172

Revert "Skip taint tests when -DSILENT_NO_TAINT_SUPPORT is enabled."

5d6a7e4

This reverts commit bca1c72. A PR has been created upstream: Dual-Life/Scalar-List-Utils#124

Revert "Skip taint tests when -DSILENT_NO_TAINT_SUPPORT is enabled."

e5fbeee

This reverts commit d4fa19e. A PR has been created for upstream: Perl-Toolchain-Gang/Test-Harness#118

Revert "Skip taint tests when -DSILENT_NO_TAINT_SUPPORT is enabled."

8353261

This reverts commit c61fb35. A PR has been created upstream: neilb/Text-ParseWords#2

Stop forcing tests to run, rely on the environment to configure that

407081d

JRaspass reviewed Mar 26, 2023

View reviewed changes

demerphq requested changes Mar 27, 2023

View reviewed changes

DrHyde closed this Mar 28, 2023

demerphq mentioned this pull request Mar 29, 2023

Config.pm - add taint_disabled and taint_support to %Config #20983

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix tests when -DSILENT_NO_TAINT_SUPPORT is defined #20972

Fix tests when -DSILENT_NO_TAINT_SUPPORT is defined #20972

Uh oh!

DrHyde commented Mar 26, 2023 •

edited

Loading

Uh oh!

Leont commented Mar 26, 2023

Uh oh!

DrHyde commented Mar 26, 2023 •

edited

Loading

Uh oh!

JRaspass Mar 26, 2023

Uh oh!

DrHyde Mar 26, 2023

Uh oh!

Leont commented Mar 26, 2023

Uh oh!

demerphq commented Mar 27, 2023 •

edited

Loading

Uh oh!

demerphq Mar 27, 2023

Uh oh!

demerphq Mar 27, 2023

Uh oh!

DrHyde commented Mar 28, 2023

Uh oh!

demerphq commented Mar 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix tests when -DSILENT_NO_TAINT_SUPPORT is defined #20972

Fix tests when -DSILENT_NO_TAINT_SUPPORT is defined #20972

Uh oh!

Conversation

DrHyde commented Mar 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Leont commented Mar 26, 2023

Uh oh!

DrHyde commented Mar 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JRaspass Mar 26, 2023

Choose a reason for hiding this comment

Uh oh!

DrHyde Mar 26, 2023

Choose a reason for hiding this comment

Uh oh!

Leont commented Mar 26, 2023

Uh oh!

demerphq commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

demerphq Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

demerphq Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

DrHyde commented Mar 28, 2023

Uh oh!

demerphq commented Mar 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DrHyde commented Mar 26, 2023 •

edited

Loading

DrHyde commented Mar 26, 2023 •

edited

Loading

demerphq commented Mar 27, 2023 •

edited

Loading