Damon Buckwalter wrote:
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.
Right, because what I said was bogus, slurp_filename didn't throw an APR::Error object. Please try with this patch:
Index: src/modules/perl/modperl_util.c
===================================================================
--- src/modules/perl/modperl_util.c (revision 155373)
+++ src/modules/perl/modperl_util.c (working copy)
@@ -597,11 +597,13 @@
return (svp && *svp != &PL_sv_undef) ? 1 : 0;
}-#define SLURP_SUCCESS(action) \
- if (rc != APR_SUCCESS) { \
- SvREFCNT_dec(sv); \
- Perl_croak(aTHX_ "Error " action " '%s': %s ", r->filename, \
- modperl_error_strerror(aTHX_ rc)); \
+#define SLURP_SUCCESS(action) \
+ if (rc != APR_SUCCESS) { \
+ SvREFCNT_dec(sv); \
+ modperl_croak(aTHX_ rc, \
+ apr_psprintf(r->pool, \
+ "slurp_filename('%s') / " action, \
+ r->filename)); \
}MP_INLINE SV *modperl_slurp_filename(pTHX_ request_rec *r, int tainted)
+ } 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.
sure, let's give them best info we can. with the patch above you will get the exact rc.
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.
user-wise it doesn't make any difference. developers get a detailed error message in the logs. May be I'm biased, since when I develop my code I always first watch the error_log and only then the browser :)
but as I said above if we can match mod_cgi behavior, that would be perfect.
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.
That's correct.
-- __________________________________________________________________ Stas Bekman JAm_pH ------> Just Another mod_perl Hacker http://stason.org/ mod_perl Guide ---> http://perl.apache.org mailto:[EMAIL PROTECTED] http://use.perl.org http://apacheweek.com http://modperlbook.org http://apache.org http://ticketmaster.com
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
