Less hostile workaround for dot-in-inc #10
Conversation
The original local()-based implementation would mask away any @inc manipulation within a require()d chain. *PLEASE* do not ship the maint-perls without implementing this or a similar fix.
|
Similar changes ( |
|
This is a read only mirror, see http://perldoc.perl.org/perlhack.html |
|
Just to confirm my reading is correct: Basically, you're removing the trailing '.', and then only putting it back at scope exit, and then, only if @inc[-1] is in the same state as it was before hand. The idea being if somebody unshifts @inc, 'thing', that suppresses adding a '.' as well. But I don't understand exactly why this solution is different/better than the existing one. |
|
Ah, right, because with just a naive local, any manipulations made during the |
|
its just this logic I don't quite get: if ($INC[-1] eq '.') {
pop @INC;
my $localized_tail = $INC[-1];
...
push @INC, '.' if $localized_tail eq $INC[-1]||'';If somebody modifies @inc via |
But it won’t, because the code checks whether Off the cuff I’m not sure that that condition makes sense. Otherwise the approach looks good. |
|
Possibly it should be |
|
Shed-painting notes: I’d prefer to write this with a non-generic scope guard. Also, since it’s just the @@ -6,6 +6,13 @@ use vars qw($VERSION);
$VERSION = '2.24';
$VERSION =~ tr/_//d;
+sub base::__scope_guard::DESTROY { push @INC, '.' if ${$_[0]} eq $INC[-1]||'' }
+
# constant.pm is slow
sub SUCCESS () { 1 }
@@ -91,14 +98,22 @@ sub import {
next if grep $_->isa($base), ($inheritor, @bases);
- # Following blocks help isolate $SIG{__DIE__} changes
+ # Following blocks help isolate $SIG{__DIE__} and @INC changes
{
my $sigdie;
{
local $SIG{__DIE__};
my $fn = _module_to_filename($base);
- local @INC = @INC;
pop @INC if my $dotty = $INC[-1] eq '.';
+ $dotty &&= bless \$INC[-1], 'base::__scope_guard';(If the carefully-conditional |
|
@ribasushi This is not an official mirror of Perl and the organization is not owned or used by p5p. It is equivalent to a fork someone has made. While it can serve a good place for you to hold a public fork and a visible patch, it is not the appropriate place to have p5p discuss it and review it for merge. I have linked several people to it who have worked or reviewed CVE-2016-1238 so they could assess it, but if you want this discussed properly, I urge you to send it to p5p mailing list or to rt.perl.org. |
@ap this would preserve someones intent of "I want this coderef to be the last-most thing in No objection on the rest of the stylistic changes. |
|
@xsawyerx I am sorry, given the current climate on If any of the participants of this thread believes the raised concerns are important enough - feel free to move further discussion (as if any is needed) to an approved venue. Alternatively if the patch dies because a specific bureaucratic dance wasn't followed - it is what it is. |
|
@ribasushi I don't know what "climate" you are referring to, and the only one I can imagine is when I warned you about being offensive towards others. I still stand by that. I've reached out to you in multiple ways to understand if this is about something else and to resolve it, but so far it resulted in /dev/null. This current situation boils down to you having a reasonable and useful patch for a core module that affects many users. We need to be able to discuss it with a larger group, so any suggested changes or questions about it can be addressed. This forum does not provide one and you refuse to use the existing forum. It's a shame. Instead, we now need to open a ticket with your patch, discuss it, and go back and forth with this ticket on any questions or comments. |
|
Can we not have a meta-conversation on this thread? That typically doesn't go anywhere. Because you're going to be opinion swapping at this rate and its all downhill from there. Stick with the immediate bug and take politics somewhere else. |
|
I've manually relayed this myself to perl5porters to ensure it gets more eyes before we're stuck with the maint-release that's due in 3 days. |
|
@xsawyerx relaying where I left things last week on (yet another) closed list: There has been no discussion of this since that I am aware of |
|
@ribasushi We have run the same tests against your code and we don't see any breakage coming from it. Another suggestion, coming from @toddr, is to leave I would be happy to have your opinion on this. |
|
I would be happy to add more information about the possible risks, but I cannot do this publicly because it isn't disclosed yet. If you cannot do this privately, I won't be able to share it with you. :/ |
|
If the option is to leave "There's still a potential trap here" is not great, but if we can't be sure we're not breaking code, then we shouldn't be doing it, least of all in a stable point-release. At least, if we leave it as-is there's still a possible future where we can route around it, if we decide to. Its only by prematurely committing to this that we cut ourselves off ( because we'll be dealing with the consequences of that mistake for the next 5 years at least ) I mean, in a sense, fencing Because making every We're not ready to do the first of these, so we shouldn't be doing the latter either, its simply not self-consistent. Similarly, everything that mimics "require foo" semantics that is user facing should probably not be fixed in this way, because it all is prone to the same "breaking how require works" problem. the correct place for existing code that is vulnerable to this in Perl core is in things that do use base Fooso those statements should probably be augmented to BEGIN {
local @INC = @INC;
pop @INC if $INC[-1] eq '.';
require base;
base->import("Foo");
}But at that point, the reason you were using BEGIN {
local @INC = @INC;
pop @INC if $INC[-1] eq '.';
require Foo;
@ISA = ("Foo");
}I only hope |
[DELTA] 2.096 31 July 2020 * Add Zip support for Zstd 508258baeeec51ba49c3c07d2dda7c19e3194985 * Add support for Zip/Unzip with XZ compression 6d240d3b3514d627a751ec82fe71f2e236301e19 3c0046e8bc65ef467b9153722609654d3ccc5bbd 2.095 19 July 2020 * Add Support for Zstandard in AnyUncompress 2.094 13 July 2020 * bin/zipdetails version 2 7acb49ff4ca67051deaffd7f988556dae0dd884b small update f5988eebc21a4d0b96e0b094e6e9bf8d3dcb1763 Better error messages for missing Zip64 records d224dcc321dd1ff120345ac3a19286ecdc79776f Add note about warning output 4caa0e5117c4c214f457d90f9a87d00772a79622 Add --version option 6c045c859d2b6bab0398833f207d7f9b803bbbab Version 2 df97743ffa1da816936e8ef504c9d561d66bb0ed Beef up some error cases 073129c4f44ebd3cc2c5381ffa824fc09b474c29 Rename a couple of unused signatures 72568c7d9edfd3e2fb6647dce6ea511e9caa186c update comment 1088199809cabb9c565ac23f065988683aacd323 Merge branch 'master' of https://github.com/pmqs/IO-Compress ad987ab95e3f3fa02fcf526736ad2da78d327460 Merge pull request Perl#10 from fabiensanglard/master ac76d1b3d3f23077b1700778226edd68c50d81a8 fix typo 5950d7e724479f0eceffe68ae515ac117ff6a5ef Don't output "Extra Payload" if length is zero dbd3160decd9b761dbad7aaae2ec46c0173125ef Merge pull request Perl#12 from fabiensanglard/extra 7ae4a98124c9195ca5286e3ac7d2cbe37fa2b644 Recover from bad extra subfield 3e12e62916da31c003a7273293bc32bb9a31f85f Fix typo f3a0a4717433d32743f17d40adc30e11bea60868 Fix wrong START offset 6f078dca715473276556afb0b8582bb69efa7230 Typo for Implode string "Shannon-Fano Trees" 4e25fed1a8e29518fa38f0610a5ca33ca41e9d89 some small documentation updates. 1be04bf4bd5fb023ad276ecabdbc170823bac465 Add decoder for 'Open Packaging Growth Hint' 2da58735bdbd1149863014dd08a7cea0334f52d5 update compression method 16 82a9612676ae192747b8bcbf586b09408c3b72ce Add extra fields 0x20-0x23 from APPNOTE 6.3.5 bc5e2ffbc560b236bc3be0f977ce744f2a2afbfb remove trailing whitespace 3f70119190671b00eb432e36904aa9dbb2fb8f69 minor documentation changes
[DELTA] 2.096 31 July 2020 * Add Zip support for Zstd 508258baeeec51ba49c3c07d2dda7c19e3194985 * Add support for Zip/Unzip with XZ compression 6d240d3b3514d627a751ec82fe71f2e236301e19 3c0046e8bc65ef467b9153722609654d3ccc5bbd 2.095 19 July 2020 * Add Support for Zstandard in AnyUncompress 2.094 13 July 2020 * bin/zipdetails version 2 7acb49ff4ca67051deaffd7f988556dae0dd884b small update f5988eebc21a4d0b96e0b094e6e9bf8d3dcb1763 Better error messages for missing Zip64 records d224dcc321dd1ff120345ac3a19286ecdc79776f Add note about warning output 4caa0e5117c4c214f457d90f9a87d00772a79622 Add --version option 6c045c859d2b6bab0398833f207d7f9b803bbbab Version 2 df97743ffa1da816936e8ef504c9d561d66bb0ed Beef up some error cases 073129c4f44ebd3cc2c5381ffa824fc09b474c29 Rename a couple of unused signatures 72568c7d9edfd3e2fb6647dce6ea511e9caa186c update comment 1088199809cabb9c565ac23f065988683aacd323 Merge branch 'master' of https://github.com/pmqs/IO-Compress ad987ab95e3f3fa02fcf526736ad2da78d327460 Merge pull request #10 from fabiensanglard/master ac76d1b3d3f23077b1700778226edd68c50d81a8 fix typo 5950d7e724479f0eceffe68ae515ac117ff6a5ef Don't output "Extra Payload" if length is zero dbd3160decd9b761dbad7aaae2ec46c0173125ef Merge pull request #12 from fabiensanglard/extra 7ae4a98124c9195ca5286e3ac7d2cbe37fa2b644 Recover from bad extra subfield 3e12e62916da31c003a7273293bc32bb9a31f85f Fix typo f3a0a4717433d32743f17d40adc30e11bea60868 Fix wrong START offset 6f078dca715473276556afb0b8582bb69efa7230 Typo for Implode string "Shannon-Fano Trees" 4e25fed1a8e29518fa38f0610a5ca33ca41e9d89 some small documentation updates. 1be04bf4bd5fb023ad276ecabdbc170823bac465 Add decoder for 'Open Packaging Growth Hint' 2da58735bdbd1149863014dd08a7cea0334f52d5 update compression method 16 82a9612676ae192747b8bcbf586b09408c3b72ce Add extra fields 0x20-0x23 from APPNOTE 6.3.5 bc5e2ffbc560b236bc3be0f977ce744f2a2afbfb remove trailing whitespace 3f70119190671b00eb432e36904aa9dbb2fb8f69 minor documentation changes
Use warnings::enabled function to check if uninitialized warnings are…
NOTE - this is a workaround for the current version of
base.pmin blead. I do not endorse, agree or condone the original changeset from 362f3f7 / bca5527 / 8901dde. Simply popping '.' is shortsighted and wrongWith that said - the current implementation has a pretty serious regression, hence this PR as an attempt at damage control. The original local()-based implementation would mask away any
@INCmanipulation within a require()d chain.
@xsawyerx PLEASE do not ship the maint-perls without implementing this or a similar fix.
@haarg, @kentfredric, @ap, @Leont, @karenetheridge : I wrote this in the past hour literally in a moving vehicle, with a mild headache. Please scrutinize the changeset. I would be very surprised if it doesn't contain problems.