Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Perl: porters
NWCLARK TPF grant June report
 

Index | Next | Previous | View Flat


nick at ccl4

Jul 26, 2012, 4:19 AM


Views: 118
Permalink
NWCLARK TPF grant June report

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
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:

$ 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 C<stat>,
intending that to call File::stat::stat(). However the refactoring of the
test script (all part of the same commit) from C<use File::stat;> to
C<use_ok( 'File::stat' );> (not in a BEGIN block) intentionally eliminated
the export of &File::stat::stat. This meant that the plain C<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
RT #... is a bug in 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/

[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

Nicholas Clark

Subject User Time
NWCLARK TPF grant June report nick at ccl4 Jul 26, 2012, 4:19 AM
    Re: NWCLARK TPF grant June report perl.p5p at rjbs Jul 26, 2012, 8:00 PM
        Re: NWCLARK TPF grant June report mhx-perl at gmx Jul 28, 2012, 10:32 AM

  Index | Next | Previous | View Flat
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.