perl-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stas Bekman <s...@stason.org>
Subject Re: ACL filesystem incompatibility and potential race condition
Date Thu, 03 Mar 2005 13:28:26 GMT
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:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org   http://ticketmaster.com

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org


Mime
View raw message