Improving Perl 5: Grant Report for Month 10
Thu, 26-Jul-2012 by
Karen Pauley
edit post
<![CDATA[
_Nicholas Clark writes:_
The most significant activity for the month was discovering, investigating and iterating fixes for the various bugs now fixed on the branch smoke-me/require. This is the cause of the delay on making a 5.16.1 release, as Ricardo explained in "http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2012-07/msg00954.html":http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2012-07/msg00954.html and hence this report, as the issue only went public two days ago.
There are two partly related problems. Both affect code which takes untrusted user data and passes it to routines that load code (a risky thing to do at the best of times), but doesn't perform sufficiently strong validation. The called routines could detect that the passed in string is crazy, but don't, and instead proceed to treat it in surprising ways.
Note, this only affects programs that
1) are "brave" enough to load *code* from disk using package names provided by user data
2) don't validate that user data thoroughly
(ie a generally bad idea, and a definitely bad implementation)
The first is accessible from Perl-space:
bc. $ cat /tmp/foo.pm
print STDERR __FILE__ . " was not the file you expected to get loaded?\n";
$ ~/Sandpit/5000/bin/perl -e 'require ::tmp::foo;'
/tmp/foo.pm was not the file you expected to get loaded?
$ echo $?
0
and is present for every version of Perl 5 ever. (Perl 4 is safe)
Of course, to be bitten by that you have to pass *user data* to a *string eval* containing require, which is already a risky thing to be doing, and this *only* increases the attack space from "all readable files in @INC ending .pm" to "all readable files on disk ending .pm"
Plus if you're already passing user data to string eval, it's usually much easier to do direct nasties - eg strings such as "less; system 'sudo rm -rf /'"
So this probably doesn't expose any holes in programs that weren't already completely unseaworthy.
It's also rather hard to search CPAN to work out if there's any code which is unwise enough to be at risk from this.
The second is accessible only to C and XS code that call the APIs function Perl_load_module() or Perl_vload_module(). These are documented as taking a module name (ie "Foo::Bar"), not a file name (ie not "Foo/Bar.pm"). However, they don't validate what they are passed, so module names such as "Foo::..::..::..::Bar" are subject to the standard s!::!/!g transformation, which obviously can be used to produce "interesting"
filenames.
Perl_load_module() has been available since 5.6.0.
Note, again this only affects programs that
1) are "brave" enough to load *code* from disk using package names provided by user data
2) don't validate that user data thoroughly
(ie a generally bad idea, and a definitely bad implementation)
and again this *only* increases the attack space from all readable files in @INC to all readable files on disk.
Nothing on CPAN uses Perl_vload_module(), and (as best I can tell from searching) only two modules on CPAN use Perl_load_module() on values that aren't constants or tightly controlled at the C level. The authors of both modules are aware, and have checked their code.
While investigating the problem described last month, of testing as root but the file tree owned by a different user, I discovered a different problem with File::stat's tests when running as root. This had prompted me to look at its regression test, to get a feel for why this wasn't spotted before. At which point the scale of the job escalated, because the test didn't look that robust or as complete as it could be. So at the start of the month I started on refactoring it, to give increase my confidence in it actually being able to spot bugs. In the process I discovered that just like t/op/filetest.t it had accumulated quite a lot of layers of cruft, each attempting to solve one of the various problems that had emerged over the years, but none looking closely at what the *intent* other work arounds were, and whether combination could be simplified.
In this case, again the problems related to "which is our victim file?" Initially the test used t/TEST. But (again), that file might be a symlink (if built with -Dmksymlinks) so the setup code has to check for that, and if so, chase the symlink back to the original file. All good so far. But, it gets worse: the test checks the output of stat, which includes the last access time, and with parallel testing, other files can read that. So the script was changed to use itself as the victim. However, the original code made the "golden result" stat call in a BEGIN block, which can still go wrong because BEGIN blocks run during compilation, and it may be that the interpreter hasn't finished reading the entire file yet, in which case the atime will get updated.
The simple solution is to use a tempfile.
This also has the benefit of being able to change the file's mode, and hence test many more permutations of the various file operators. This seemed like a quick job, so I pushed another iteration of File::stat test improvements a smoke-me branch, so that George Greer's Win32 smoker could take it for a spin.
Sigh. These things always take longer than you think. So, the aim was get the code to check filetest operators with various different file modes, not just the mode the tempfile happens to be created with. But, while refactoring the code to make this work, I realised that some of the other tests weren't actually doing what they thought that they did. (Inconceivable!)
Specifically, the end of the test script contains code intended to test that bug ID 20011110.104 (RT #7896) is fixed. The bug was indeed fixed by the changes to File::stat made by commit 2f173a711df6278f in Nov 2001, but the test that commit added never actually tested this.
The initial problem was that the new code, as written, used @stat@, intending that to call File::stat::stat(). However the refactoring of the test script (all part of the same commit) from @use File::stat;@ to @use_ok( 'File::stat' );@ (not in a BEGIN block) intentionally eliminated the export of &File::stat::stat. This meant that the plain @stat@ the added code used was the core builtin. The core builtin never exhibited the bug. :-)
Fixing this as-is to File::stat::stat() won't help, as tests have subsequently been added earlier in the script that trigger the autoloading of Symbol by File::stat (commit 83716b1ec25b41f2 in Feb 2002). Moving the tests earlier won't help now that the test uses File::Temp, as that loads IO::Seekable, which loads IO::Handle, and *that* unconditionally loads Symbol.
So the simplest solution seemed to be to move the test to its own file, which I did. Now, the bug is actually tested for, and regressions should not be possible. (But make it foolproof, and they build a better fool.)
So, finally, I got to the point where I could fix a long-standing bug in File::stat's handling of -x and -X for directories and executable files when running as root, which I'd spotted last month.
Previously File::stat's overloaded -x and -X operators gave the correct results only for regular files when running as root. However, they had been treating executable permissions for root just like for any other user, performing group membership tests etc. for files not owned by root. They now follow the correct Unix behaviour - for a directory they are always true, and for a file if any of the three execute permission bits are set then they report that root can execute the file. Perl's builtin -x and -X operators, added in Perl 2, have always been correct.
Thinking I was home and dry, I pushed a smoke-me branch to the repository and waited for the testers on various platforms to check it out and report back. Result - *nearly* everything worked, except for a mysterious failure on OS X. So I tried the test on OS X locally, and obviously I couldn't replicate the failure. Which resulted in a careful scouring of the smoker logs (exactly which test did fail?), trying to replicate the problem on various other operating systems (no success), and eventually realising that the problem was a combination of two things. Firstly that darwin defaults mounts to POSIXly-correct atime semantics - atime is always updated when a file is read. Secondly, when tests run in parallel, the atime of $^X will update whenever another perl process starts. The result is a race condition between the stat calls in this test, and any other test that runs. Life is always fun when reads trigger write actions - and it doesn't just happen with perl's scalar types.
It also turned out that my refactoring of the test exposed a latent bug present in it, which I failed to spot. Fortunately Father Chrysostomos diagnosed that the test was always using stat(), even when testing -l, and fixed it to use lstat() in that case.
I spotted some code in Class::Struct related to 5.002 and 5.003 compatibility. Obviously that's long dead, so it should come out, and this will be a nice quick one-hit tidy up. Of course, the world doesn't work like this. The code in question affects an error message, and it turns out that there are no tests for any of the error conditions in Class::Struct. It felt wrong to add just one test for the error check whose code I was refactoring, leaving all the rest naked, so the simple fix acquired a not-so-simple pre-requisite. And then that turned out to reveal another hole in the testing - initialiser lists had insufficient tests. So, before testing the error paths in initialiser lists, we probably should test the success paths. And so some time later, I removed (net) 5 lines of dead code. Collateral damage - 69 regression tests, where previously there were only 26.
Config::Perl::V is a module written by H. Merijn Brand to provide structured access to the output of perl -V, used by most of the CPAN testing infrastructure. The reason that it exists is that historically some configuration information about the running perl had only been available in the output from running perl -V. This gets "interesting" if the running perl program wants the values, as not only does one have to parse the output robustly and cope with the obscure case, but in the general case re-running the correct perl interpreter binary is not a trivial task - possibly it's not installed so library paths need overriding, or possibly not the first perl in the PATH. The module abstracts all this away nicely.
The plan is to add it to the core distribution, because it simplifies the CPAN toolchain to have it always available. However, the "historically" above is pertinent - the implementation of Config::Perl::V predates changes I made for 5.14.0, which has functions in the Config module which expose to Perl space all the information previously only available by running perl -V. Hence I spent some time updating Config::Perl::V to use the routines in Config where available, and adding a test that cross checks that the old and new approaches give the same answers.
Blead had been failing two tests when built "outside" the source tree using a symlink forest generated by -Dmksymlinks. In both cases, they related to how various porting tests that require a git repository work under -Dmksymlinks. To enable the tests to run instead of simply skipping, the setup routine would detect the symlink situation, and that they pointed to a real checkout with a .git directory, and change directory to it, so that git commands would work.
However, running the tests *outside* of the build tree turns out also to cause problems. t/porting/cmp_version.t was failing because it could see that ext/DynaLoader/dl_vms.xs has been modified since the last tag, but was unable to work out which was the related .pm file, to validate that $VERSION had been increased. It turned out that this was working just find *inside* a build tree, because ext/DynaLoader/DynaLoader.pm exists there, having been created by ExtUtils::MakeMaker from ext/DynaLoader/DynaLoader_pm.PL However, in a clean tree, it doesn't exist, hence the confusion. So I improved Porting/cmpVERSION.pl (the underlying driver script) to teach it about _pm.PL files.
t/porting/utils.t had recently started failing, because Ricardo changed it to avoid false positive failures, by only running if it found itself in a git checkout. However, it assumes that it can find many files generated by the build process (to syntax check them), whilst the "are we in a git checkout?" code assumes that it should change directory to the checkout, if one is building symlinked from it. Result - various files that "should" be there are now missing. The solution was to change the "are we in a git checkout?" code to set GIT_DIR instead of changing directory, and return the path to the git checkout. That way, tests that really need to be in the git checkout (ie t/porting/cmp_version.t) can change directory to it, but by default tests continue to run from the build tree. This has two benefits
a) fewer surprises
b) for a couple of tests, we can drop the code that converts @INC to absolute
paths, because they no longer change directory
(The tests by default run, and have always run, with the relative path of the build lib/ in @INC. Converting it to an absolute path relies on fairly complex platform specific Perl code, and how are you going to safely test that works before you're even sure that simple Perl code works? When testing an interpreter from scratch, you need to bootstrap your tests too, because you initially can't assume anything works.)
A more detailed breakdown summarised from the weekly reports. In these:
16 hex digits refer to commits in "http://perl5.git.perl.org/perl.git":http://perl5.git.perl.org/perl.git
RT #... is a bug in "https://rt.perl.org/rt3/":https://rt.perl.org/rt3/
CPAN #... is a bug in https://rt.cpan.org/Public/
BBC is "bleadperl breaks CPAN" - Andreas König's test reports for CPAN modules
ID YYYYMMDD.### is an bug number in the old bug system. The RT # is given
afterwards. You can look up the old IDs at "https://rt.perl.org/perlbug/":https://rt.perl.org/perlbug/
|Hours| Activity|
| 3.00 | Class::Struct |
| 3.00 | Config::Perl::V |
| 5.75 | File::stat |
| 0.50 | File:DosGlob |
| 0.75 | HP-UX -Duse64bitall |
| 0.50 | ID 20020306.011 (RT #8800) |
| 0.25 | IO::Socket::IP |
| 0.50 | Locale::Codes |
| 0.50 | Module::Build failures on VMS |
| 3.00 | Porting tests with -Dmksymlinks |
| 0.50 | RT #113094 |
| 0.25 | RT #113464 |
| 0.75 | RT #113472 |
| 0.25 | RT #113620 |
| 0.25 | RT #16249 |
| 0.25 | Ubuntu link fail with non-English locales |
| 4.00 | VMS |
| 0.25 | applying patches (with massaging) |
| 0.75 | bisect.pl (Debian multiarch) |
| 0.50 | chip/magicflags4 |
| 9.75 | lib/File/stat.t |
| 1.25 | lib/locale.t |
| 0.75 | make_ext.pl |
| 0.25 | mktables memory usage |
| 0.75 | named arguments in prototypes |
| 0.25 | perldelta updates |
| 0.25 | perlsecret |
| 8.25 | pp_require |
| 1.00 | process, scalability, mentoring |
| 1.75 | re/eval on VMS |
| 23.25 | reading/responding to list mail |
| 0.25 | regexp set syntax |
| 1.00 | smoke-me branches |
| 0.75 | smoke-me/cmpVERSION |
| 0.25 | smoke-me/make_ext |
| 14.75 | smoke-me/require |
| 2.00 | smoker logs |
| 4.75 | t/op/arith.t |
| | t/op/arith.t tryeq_sloppy |
| 2.50 | t/op/getppid.t |
| 3.25 | t/re/reg_posixcc.t |
**102.50 hours total**
]]>
Comments (0)
