Collected issues related to @INC hooks.

[demerphq]

Description
Perl supports "hooks" in the @INC array. The original idea was to allow for source filters, but because they were exposed as sub/method calls to the user the API opens up a number of troublesome issues which unfortunately the code does not account for. We have had bugs related to this previously, but we only fixed them superficially, we never addressed the fundamental issues involved.

The main problem is that @INC hooks can and do modify the @INC array, and the behavior when they do so is pretty much accidental as opposed to intended, and can even result in accesses freed memory.

So the first issue is that code like this:

perl -le'BEGIN { push @INC, sub { warn "overwriting INC array in glob"; *INC=[sub{warn "C"}]; }, sub{ warn "in B" } }; eval "require Frobnitz" or die $@'

will access the original @INC array after it is freed. This just happens to work out to just not do anything as it sees the freed array as having 0 elements and stops processing entirely. (It may be that the sub itself is also destroyed before it finishes executing, i havent checked) Arguably this code should never access freed memory and should output this:

$ ./perl -le'BEGIN { push @INC, sub { warn "overwriting INC array in glob"; *INC=[sub{warn "in C"}]; }, sub{ warn "in B" } }; eval "require Frobnitz" or die $@' 
overwriting INC array in glob at -e line 1.
in C at -e line 1.
Can't locate Frobnitz.pm in @INC (you may need to install the Frobnitz module) (@INC contains: CODE(0x556edbddb900)) at (eval 1) line 1.

Another interesting case is this:

perl -le'my $counter; BEGIN { push @INC, sub { warn "unshifting INC"; unshift @INC, sub{warn "in shift ",++$counter}; }, sub{ warn "in B" } }; eval "require Frobnitz" or die $@' 2>&1 | head

which outputs:

perl -le'my $counter; BEGIN { push @INC, sub { warn "unshifting INC"; unshift @INC, sub{warn "in shift ",++$counter}; }, sub{ warn "in B" } }; eval "require Frobnitz" or die $@' 2>&1 | head 
unshifting INC at -e line 1.
unshifting INC at -e line 1.
unshifting INC at -e line 1.
unshifting INC at -e line 1.
unshifting INC at -e line 1.
unshifting INC at -e line 1.
unshifting INC at -e line 1.
unshifting INC at -e line 1.
unshifting INC at -e line 1.
unshifting INC at -e line 1.
....

eg, it infinite loops because the array keeps growing and the callback keeps getting called over and over. IMO it would be saner for all if this output this:

$ ./perl -le'my $counter; BEGIN { push @INC, sub { warn "A unshifting INC"; unshift @INC, sub{warn "C in shift ",++$counter}; }, sub{ warn "in B" } }; eval "require Frobnitz" or die $@' 2>&1 | head 
A unshifting INC at -e line 1.
C in shift 1 at -e line 1.
in B at -e line 1.
Can't locate Frobnitz.pm in @INC (you may need to install the Frobnitz module) (@INC contains: CODE(0x559a13e94638) /home/yorton/perl5/perlbrew/perls/latest_blead/lib/site_perl/5.37.7/x86_64-linux-thread-multi /home/yorton/perl5/perlbrew/perls/latest_blead/lib/site_perl/5.37.7 /home/yorton/perl5/perlbrew/perls/latest_blead/lib/5.37.7/x86_64-linux-thread-multi /home/yorton/perl5/perlbrew/perls/latest_blead/lib/5.37.7 CODE(0x559a13e85910) CODE(0x559a13e857c0)) at (eval 1) line 1.

or with the reverse order of the "B" and "C" lines.

Another bug is this:

norole:yorton@oncidium:blead:~/git_tree/perl$ perl -le'my $counter; BEGIN { push @INC, bless(sub { warn "A"; }), sub{ warn "B" } }; eval "require Frobnitz" or die $@' 2>&1 | head 
Can't locate object method "INC" via package "main" at (eval 1) line 1.

why is it calling the INC method on an object that does not support it, and why doesnt the error message say "in @inc hook" or something to that effect? IMO this should output the following:

$ perl -le'my $counter; BEGIN { push @INC, sub { warn "A"; }, sub{ warn "B" } }; eval "require Frobnitz" or die $@' 2>&1 | head 
A at -e line 1.
B at -e line 1.
Can't locate Frobnitz.pm in @INC (you may need to install the Frobnitz module) (@INC contains: /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/x86_64-linux /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1 /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/5.34.1/x86_64-linux /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/5.34.1 CODE(0x5649a78a39d8) CODE(0x5649a78a3a68)) at (eval 1) line 1.

Another issue is that @INC hooks and the errors that are generated from them are not "crazy proofed", in the sense that you can tie @INC, and then have it return a list of places to look, but then when the error message is constructed show a totally different set of places, that might not even have been inspected. I am not showing an example, but the way the code is structured it is entirely possible. It is somewhat debatable if they should be crazy proofed, but personally i think they should be. A simple example without tie is this:

perl -le'my $counter; BEGIN { push @INC, sub { @INC=()}, }; require Frobnitz;' 2>&1 | head 
Can't locate Frobnitz.pm in @INC (you may need to install the Frobnitz module) (@INC contains:) at -e line 1.

I would argue that the error message should show where we checked, not an empty list.

IMO the @INC hooks should ensure that any given ref is looked at only once during traversal of the @INC array, and that after an @INC returns we should either a) restart processing from the beginning, ignoring anything we have already seen, or b) continue and wrap around to do a full traversal back to the most recently executed @inc hook, assuming that index is still reachable. the former is easier to implement, but the latter might be more efficient. We also need to assume that the old @INC array no longer exists after any @INC hook callback.

Hopefully it is possibly to clean up these issues without imposing any noticeable costs on people doing lots of require statements.

Steps to Reproduce
Repro steps are above.

Expected behavior

• @INC hooks should never access freed memory.
• @INC hooks should never fire more than once per require statement.
• Modifications to @INC by hooks should be visible regardless as to where they are in the array.
• Error message should show where we checked, and maybe info about the callbacks used, not the state of @INC at the end of the require.
• Blessed subs with no INC methods should be called as a subroutine, not as an object which would produce a method call error.

Perl configuration
Not relevant. This ticket is correct to the best of knowledge at 5.37.7

[haarg]

There is currently code in core which relies on @INC traversal being essentially by index. It is a hook which will splice itself out of @INC and expects that the following entry (which now has the same index the hook did originally) to be skipped.

https://github.com/Perl/perl5/blob/blead/dist/base/lib/base.pm#L108

[demerphq]

@haarg: yeah, ill check up on that. but at a quick glance that is exactly the kind of hokey code that I am trying to make redundant. The INC logic is basically broken. People doing interesting things with it have had to work around the brokenness, and maybe in some cases exploited it. Ideally we can make it sane without too much fallout, but i suspect we will see a bit.

[demerphq]

@haarg, i need to play with it more (and im on the way out to dinner) but a closer read suggests that this code might even work unchanged if I were to do the "wrap around approach". Maybe im wrong, tho, my assumption is that any hook in @inc at the end of a failed require should have been called once, including those pushed into the start of the array.

But this is part of the problem. IMO if I have an @inc with only a single callback in it, which populates @inc its first run, removing itself in the process, the stuff it pushes in should be looked up, including the first element. Similarly, if I had a hook at the end, which cleaned up non-existent paths, and maybe added newly discovered ones, id say the newly added ones should be visible before the require finishes.

I think there will be some tension here between "useful behavior" and "current implementation warts".

Frankly im getting to the point when I think exposing our directory lookups as an array was fairly unfortunate. It opens up way too many cans of worms IMO. Ties. Hooks. (That arent even evaled correctly). Blah. Its ridiculously complex.

[demerphq]

An example of the tied case messing up the error message. It says we checked 11, but if did an strace you would see we checked 10.

perl -MTie::Scalar -wle'use strict; my $counter= 10; sub R::FETCH { warn "in R::FETCH"; return $counter++ }; @R::ISA=qw(Tie::StdScalar); tie $INC[-1], "R"; eval "require Frobnitz" or print $@;'
in R::FETCH at -e line 1.
in R::FETCH at -e line 1.
Can't locate Frobnitz.pm in @INC (you may need to install the Frobnitz module) (@INC contains: /home/yorton/perl5/perlbrew/perls/latest_blead/lib/site_perl/5.37.7/x86_64-linux-thread-multi /home/yorton/perl5/perlbrew/perls/latest_blead/lib/site_perl/5.37.7 /home/yorton/perl5/perlbrew/perls/latest_blead/lib/5.37.7/x86_64-linux-thread-multi 11) at (eval 5) line 1.

[demerphq]

Sigh. So far @haarg the code you pointed at defeated me. @ap has come up with a devilishly clever solution there, and I don't see how i can do what that code does with the semantics I wished were in place. My fallback is to provide a way to workaround the old malbehavior with a new var. So when an @inc hook is called the var $INC is set to its index. When the hook returns the value of the iterator is set to its value. Setting it to undef or -1 would trigger a new pass over the @inc array contents and setting it to X-1 would cause it to process item X next (or exit if X was larger than the array). That way we can use an @inc hook to replumb @inc if we wish (which I think you and I agree is not an unreasonable thing to want to do) without breaking existing code. We would still ensure that any given in the hook in the array is called only once during traversing the list.

I was also thinking of supporting a new API for a blessed hook, so that the method INCDIR (as opposed to INC) would return a directory name to search, or undef which would be ignored, so that you could have a hook that can decide if it wants to return something (like "."). (I want to make the code ignore undef in the @inc array anyway.) It is possible such a hook could replace the logic in base.pm, but im not certain and it wouldnt be backwards compatible.

[ap]

For base.pm both the $INC variable and the INCDIR method would allow making the second hook 100% reliable by making it a stand-in for the dot, rather than the current situation where it exists in @INC alongside the dot. But base.pm is somewhat of a unique case as far as @INC hooks because of the requirement to only hide the dot from the top-level require run by base.pm itself, not from nested requires run by the loaded code. So the hooks it places in @INC are not meant to stay there and provide some functionality on an ongoing basis, they want to disappear as soon as possible – and between those two options, $INC would be more useful for that than INCDIR: the second hook would poke an actual dot into @INC at $INC to replace itself and then increment $INC before returning.

[demerphq]

@ap - I was planning on doing both (as in a have a patch working already, although not really release worthy just yet). I see a lot of utility in a smart INCDIR callback. And I see a lot of utility in having some control over the order things get searched. If you can use either to make base more robust then I think that bodes well for the patch.

My I congratulate you again, the logic in base.pm is devilishly clever. Bravo!

[demerphq]

I have pushed #20547 to address the issues in this ticket.

[ap]

Thank you, thank you. Mind you it took months of staring at each iteration of the code and thinking through all the edge cases and reasoning out the exact requirements of base.pm to arrive at this devilry. Just writing it up in the comment took days of iterating. Happy with the result though.

Case in point re trickiness of working out all the consequences:

making the second hook 100% reliable by making it a stand-in for the dot, rather than the current situation where it exists in @INC alongside the dot

Nope, because other code looking for the dot to remove it will not know that this hook is a concealed dot. The literal dot must be left in @INC to avoid breaking other @INC-manipulating code.

The next best thing I can think of (but this is off the top of my head again) is using $INC in the hook to check whether the next element is a dot before causing it to be skipped. Disturbing the arrangement would then deactivate the hook instead of leaving it to quietly skip the wrong thing.

[demerphq]

Nope, because other code looking for the dot to remove it will not know that this hook is a concealed dot.

@ap: Couldn't it use the object form and test on that with isa()? I was thinking something like:

sub HiddenDot::INCDIR { ${$_[0]}++ ? "." : () } # uses blessed scalar ref as object
sub HiddenDot::new { my ($class,$value)= @_; return bless \$value, $class }

# and in base.pm:
if (@INC and $INC[-1] eq ".") {
  $INC[-1] = HiddenDot->new();
} else if (@INC and UNIVERSAL::ISA($INC[-1],"HiddenDot")) {
  ${$INC[-1]} = 0;
}