Config.pm - add taint_disabled and taint_support to %Config #20983
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
demerphq
force-pushed
the
yves/taint_disabled
branch
from
626ba2e to
1923a28
Compare
Contributor
Collaborator
Author
Good idea. Ill add it shortly. |
Collaborator
Author
|
@DrHyde : added as suggested, thanks. |
Contributor
|
I think this should be wired the other way around. There should be a |
Collaborator
Author
That breaks backwards compatibility with the people who have been creating taint-free perl with -Accflags=-DNO_TAINT_SUPPORT or -Accflags=-DSILENT_NO_TAINT_SUPPORT. Those defines have existed for 10 years or so. The configure support for it still doesn't exist properly. Part of the reason the Configure support was retracted was that it didn't support setting the define directly, so I think that idea has already been proved broken. As a general observation we need to get out of the business of assuming the only thing that setup a config variable is Configure. It puts the horse before the cart. We add new defines to control subsystems in the perl codebase all the time, we rarely add support to Configure to set these defines directly. We should not have to coordinate two repositories to have code be able to introspect these things through the easy API of %Config. |
Collaborator
Author
|
IMO there is a bigger issue here. We have a variety of defines which control how things work which Configure does not know about. Currently we report those things only via Internals::V(). So what do we do when something gets Configure support? IMO it is a regression if we stop supporting the "old way" of setting a define directly. Which seems to support the notion that Configure should just set the define, and we should follow the pattern in this patch. Overall it would be backwards compatible, and we could rollout Config.pm support for these things without worrying about the Configure repo. There are other options of course. We could introduce a new setting that is "owned" by Configure, which then causes the perl defines to be set up internally if they have not been set explicitly. For instance we could introduce d_taint_disabled, which would then cause SILENT_NO_TAINT_SUPPORT or NO_TAINT_SUPPORT to be set, which in turn would set 'taint_disabled' and 'taint_support' We could also make it so that if Configure sets taint_disabled, then we set SILENT_NO_TAINT_SUPPORT or NO_TAINT_SUPPORT based on it, but if it is not set we set it depending on the state of those defines. That would be the most complex approach IMO. Personally I favour the the first option, where for things like this Configure sets the define, which then makes things Just Work regardless if you use the -Accflags=... or if you use a Configure Q&A support for it. Conceptually it is the simplest, and it sets a precedent which we can use for many of the other define based controls. For instance the controls for what hash function we support, or if your perl uses a fixed seed, or whatnot. |
Contributor
|
When I got started on what became #20972 my use case was that I have code on the CPAN that uses taint-mode and I'd like to be able to test it to make sure it copes OK when taint-mode is not available, while still using a perl that passes all its own tests. I'm also one of the CPAN-testers, and am planning on running a tester regularly like what I already do with the various options for float- and int-size - and again, I want to use a perl that passes all its own tests. This scratches that itch. |
DrHyde
approved these changes
Mar 30, 2023
demerphq
force-pushed
the
yves/taint_disabled
branch
4 times, most recently
from
4c53223 to
4bc59e3
Compare
Collaborator
Author
|
Without this patch we fail a lot of tests with -DSILENT_NO_TAINT_SUPPORT: |
Collaborator
Author
|
These are the test failures without this branch, but compiled with -DNO_TAINT_SUPPORT |
tonycoz
reviewed
Mar 30, 2023
demerphq
force-pushed
the
yves/taint_disabled
branch
from
4bc59e3 to
8bf72b4
Compare
Contributor
|
Maybe add a CONFIGURE_ARGS matrix to the automated Windows tests so that the gotcha that @tonycoz pointed out is tested. Having a matrix of CONFIGURE_ARGS there in the workflow would hopefully remind people in the future that build options like this need testing per-platform, not just per-option. |
Collaborator
Author
I am a bit reluctant to do that, although I did think about it. We don't run the win32 build and test in parallel mode, so they take a long time, and apparently we consume a lot of resources with our Win32 tests already. So I am not sure if it would be a good thing to add yet more win32 tests. I wonder if there is some way to have our cake and eat it too tho, maybe run a reduced test of tests only. |
demerphq
force-pushed
the
yves/taint_disabled
branch
from
8bf72b4 to
87c7f60
Compare
Collaborator
Author
|
I just added a patch to disable or skip the tests that break under -DNO_TAINT_SUPPORT. |
Contributor
|
Latest patches look good. Perhaps excessively pedantically, the commit message for 77c56f9 might have its first paragraph better phrased as "This patch uses a collection of heuristics to skip test files which would die on a perl compiled with -DNO_TAINT_SUPPORT but without -DSILENT_NO_TAINT_SUPPORT", for as we discussed elsewhere, SILENT_NO_TAINT_SUPPORT also turns on NO_TAINT_SUPPORT, it just makes it shut the hell up. Given that we're OK with editing history and force-pushing I think it's worth updating. |
demerphq
force-pushed
the
yves/taint_disabled
branch
from
87c7f60 to
11cafc0
Compare
Collaborator
Author
Done. Thank you for the suggestion. |
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.
Add an entry for -DSILENT_NO_TAINT_SUPPORT to our linux based test matrix. Currently we cannot do the same for plain -DNO_TAINT_SUPPORT as it chokes on -t and -T on the command line. [Committers note: edited commit message to add detail]
This patch uses a collection of heuristics to skip test files which would die on a perl compiled with -DNO_TAINT_SUPPORT but without -DSILENT_NO_TAINT_SUPPORT. -DNO_TAINT_SUPPORT disables taint support in a "safe" way, such that if you try to use taint mode with the -t or -T options an exception will be thrown informing you that the perl you are using does not support taint. (The related setting -DSILENT_NO_TAINT_SUPPORT disables taint support but causes the -t and -T options to be silently ignored.) The error from using -t and -T is thrown very early in the process startup and there is no way to "gracefully" handle it and convert it into something else, for instance to skip a test file which contains it. This patch generally fixes our code to skip these tests. * Make t/TEST and t/harness check shebang lines and use filename checks to filter out tests that use -t or -T. Primarily this is the result of checking their shebang line, but some cpan/ files are excluded by name, either from a very short list of exclusions, or because their file name contains the word "taint". Non-cpan test files were fixed individually as noted below. * test.pl - make run_multiple_progs() skip test cases based on the switches that are part of the test definition. This function is used in a great deal of our internal tests, so it fixes a lot of tests in one go. * XS-APITest/t/call.t, t/run/switchDX.t, lib/B/Deparse.t - Skip a small set of tests in each file.
Test that we can pass test with -DNO_TAINT_SUPPORT but without -DSILENT_NO_TAINT_SUPPORT. Both disable taint mode, but the latter causes -t and -T to be silently ignored, whereas the former by itself causes use of -t and -T to throw fatal exceptions during process startup.
demerphq
force-pushed
the
yves/taint_disabled
branch
from
11cafc0 to
5b2d453
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@DrHyde - this was motivated by your recent efforts.