On Wed, 02 Mar 2005 21:06:18 -0500, Stas Bekman <[EMAIL PROTECTED]> wrote:
> Damon Buckwalter wrote:
> > On Wed, 02 Mar 2005 18:32:40 -0500, Stas Bekman <[EMAIL PROTECTED]> wrote:
> >
> > 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 ($@) {
> $self->log_error("$@");
> $rc = $@ == APR::EACCES ? Apache::FORBIDDEN : Apache::NOT_FOUND;
> }
>
> $@ is a magic object under mod_perl 2. Take a look at:
> http://perl.apache.org/docs/2.0/api/APR/Error.html
This produces the following for scripts that cannot be read/do not exist:
Software error:
Argument "Error opening '/home/www/foo.cgi...." isn't numeric in
numeric eq (==) at /usr/lib/perl5/Apache2/ModPerl/RegistryCooker.pm
line 543.
Looking at the APR::Error docs, I changed this to something like:
$rc = ref $@ eq 'APR::Error' && $@ == APR::EACCES ? Apache::FORBIDDEN
: Apache::NOT_FOUND
But I only ever get NOT_FOUND, even for files that exist but do not
have read permisson.
> Your case is certainly faulty:
>
> + if ($rc == APR::EACCES) {
> + return Apache::DECLINED;
>
> why declined and not forbidden?
This was just a guess on my part to try and make a functional proof-of-concept.
>
> + } 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.
Right, this is also broken. It would be nice to differentiate between
non-existence of a script versus bad permissions if we have that
information, but not really necessary.
>
> 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;
> $self->log_error("$@");
> }
>
> 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.
I think it is a bit confusing to return 404 if the script is there,
just not readable.
> 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.
I was just looking for a way to propagate the return code from
apr_file_open out to modperl, so I could see why it failed. If $@
achieves the same goal, it works for me.
Thanks for your work on this,
Damon
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
