perl-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stas Bekman <>
Subject Re: ACL filesystem incompatibility and potential race condition
Date Thu, 03 Mar 2005 02:06:18 GMT
Damon Buckwalter wrote:
> On Wed, 02 Mar 2005 18:32:40 -0500, Stas Bekman <> wrote:
>>failing to open a file will cover the -r check. What will cover the -x
>>check? Or do you suggest that we shouldn't require cgi scripts to be
>>executable? At least mod_cgi requires so when doing exec, but we don't
>>exec. So we should probably keep it in, no?
> I'd say no, because we're not really exec'ing anything.  It will be a
> user education issue, that PerlRun/Registry scripts will run if you
> tell Apache to handle then and ExecCGI is enabled in that context. 
> Plus it breaks my whole scheme to make mod_perl ACL compatible...

That's fine with me. Unless other developers disagree we can go with it.

> I managed to come up with something very similar to this, minus the
> last bit.  I like it, but I did manage to (partially) implement some
> code on the C/XS side to return different statuses based on whether
> the file is unreadable or non-existent.  It is broken because I can't
> figure out how to return the script from modperl_slurp_filehandle() to
> $self->{CODE}.  I just started learning XS today ;^)...  I was able to
> make missing or unreadable files return a proper status code at least.

You don't need to do any XS changes. Just the following will do:

     $self->{CODE} = eval { $self->{REQ}->slurp_filename(0) }; # untainted
     if ($@) {
         $rc = $@ == APR::EACCES ? Apache::FORBIDDEN : Apache::NOT_FOUND;

$@ is a magic object under mod_perl 2. Take a look at:

Your case is certainly faulty:

+    if ($rc == APR::EACCES) {
+        return Apache::DECLINED;

why declined and not forbidden?

+    } elsif ($rc == APR::ENOENT) {
+        return Apache::NOT_FOUND;
+    } else {
+        return Apache::OK;

why default to Apache::OK, which may miss some other errors you didn't 
encounter for. We should either default to Apache::NOT_FOUND or 500, but 
see below.

Further APR::EACCES will be given no matter whether the file exists or its 
perms are not right. So you will never end up with Apache::NOT_FOUND.

You can see that tests like t/404.t are failing with it, when using this 
version in addition to my last patch:

sub read_script {
     my $self = shift;
     my $rc = Apache::OK;

     $self->debug("reading $self->{FILENAME}") if DEBUG & D_NOISE;
     $self->{CODE} = eval { $self->{REQ}->slurp_filename(0) }; # untainted
     if ($@) {
         $rc = $@ == APR::EACCES ? Apache::FORBIDDEN : Apache::NOT_FOUND;

     return $rc;

I'd rather always return Apache::NOT_FOUND, and log_error() logs the right 
error message. I don't think the error code makes any difference to the 
user, when they can't reach the page.

regarding changing the slurp_filename() API: passing a buffer by reference 
is ugly and not perlish (though we do it sometimes where we have no 
choice). Here $@ gives us the exact error code and stringified error 
message if wanted. Ideally it should have been $!, but unfortunately it's 
impossible to extend perl-core functionality on this one. Hence the $@ object.

Stas Bekman            JAm_pH ------> Just Another mod_perl Hacker     mod_perl Guide --->

To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message