perl-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Damon Buckwalter <buck...@gmail.com>
Subject Re: ACL filesystem incompatibility and potential race condition
Date Thu, 03 Mar 2005 19:06:10 GMT
On Thu, 03 Mar 2005 08:28:26 -0500, Stas Bekman <stas@stason.org> wrote:
> Damon Buckwalter wrote:
> > 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:
> 
[patch snipped]

The patch works nicely, but every error code returns FORBIDDEN still. 
I have a tweaked version attached that I think works better.

> sure, let's give them best info we can. with the patch above you will get
> the exact rc.
> 

I had to dig into $@->{rc} to get the exact return code.  $@ ==
APR::EACCES seemed to always evaluate true, even though the error log
showed different error codes for file not found versus permission
denied:

[Thu Mar 03 10:22:14 2005] [error] slurp_filename('/home/damon/test/apache2/htdo
cs/mp/notexist.cgi') / opening: (2) No such file or directory at /home/damon/tes
t/perl/lib/site_perl/5.9.1/i686-linux-thread-multi-64int-ld/ModPerl/RegistryCook
er.pm line 540
[Thu Mar 03 10:22:14 2005] [error] rc = 2 EACCESS = 13 ENOENT = 2
[Thu Mar 03 10:41:08 2005] [error] slurp_filename('/home/damon/test/apache2/htdo
cs/mp/unreadable.cgi') / opening: (13) Permission denied at /home/damon/test/per
l/lib/site_perl/5.9.1/i686-linux-thread-multi-64int-ld/ModPerl/RegistryCooker.pm
 line 540
[Thu Mar 03 10:41:08 2005] [error] rc = 13 EACCESS = 13 ENOENT = 2


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

See my patch below, which includes your changes.  I believe that this
matches mod_cgi behavior pretty closely, returning FORBIDDEN on
EACCES,  NOT_FOUND on ENOENT, and SERVER_ERROR by default if there is
an error in $@.


Index: src/modules/perl/modperl_util.c
===================================================================
--- src/modules/perl/modperl_util.c	(revision 156065)
+++ 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)
Index: ModPerl-Registry/lib/ModPerl/RegistryCooker.pm
===================================================================
--- ModPerl-Registry/lib/ModPerl/RegistryCooker.pm	(revision 156065)
+++ ModPerl-Registry/lib/ModPerl/RegistryCooker.pm	(working copy)
@@ -41,6 +41,7 @@
 use File::Spec::Functions ();
 use File::Basename;
 
+use APR::Const     -compile => qw(EACCES ENOENT);
 use Apache::Const  -compile => qw(:common &OPT_EXECCGI);
 use ModPerl::Const -compile => 'EXIT';
 
@@ -254,21 +255,10 @@
     my $self = shift;
     my $r = $self->{REQ};
 
-    unless (-r $r->my_finfo && -s _) {
-        $self->log_error("$self->{FILENAME} not found or unable to stat");
-        return Apache::NOT_FOUND;
-    }
+    return Apache::DECLINED if -d $r->my_finfo;
 
-    return Apache::DECLINED if -d _;
-
     $self->{MTIME} = -M _;
 
-    unless (-x _ or IS_WIN32) {
-        $r->log_error("file permissions deny server execution",
-                       $self->{FILENAME});
-        return Apache::FORBIDDEN;
-    }
-
     if (!($r->allow_options & Apache::OPT_EXECCGI)) {
         $r->log_error("Options ExecCGI is off in this directory",
                        $self->{FILENAME});
@@ -372,10 +362,13 @@
 sub convert_script_to_compiled_handler {
     my $self = shift;
 
+    my $rc = Apache::OK;
+
     $self->debug("Adding package $self->{PACKAGE}") if DEBUG & D_NOISE;
 
     # get the script's source
-    $self->read_script;
+    $rc = $self->read_script;
+    return $rc unless $rc == Apache::OK;
 
     # convert the shebang line opts into perl code
     $self->rewrite_shebang;
@@ -408,7 +401,7 @@
                     ${ $self->{CODE} },
                     "\n}"; # last line comment without newline?
 
-    my $rc = $self->compile(\$eval);
+    $rc = $self->compile(\$eval);
     return $rc unless $rc == Apache::OK;
     $self->debug(qq{compiled package \"$self->{PACKAGE}\"}) if DEBUG & D_NOISE;
 
@@ -534,16 +527,33 @@
 # dflt: read_script
 # desc: reads the script in
 # args: $self - registry blessed object
-# rtrn: nothing
+# rtrn: Apache::OK on success, some other code on failure
 # efct: initializes the CODE field with the source script
 #########################################################################
 
 # reads the contents of the file
 sub read_script {
     my $self = shift;
+    my $rc = Apache::OK;
 
     $self->debug("reading $self->{FILENAME}") if DEBUG & D_NOISE;
-    $self->{CODE} = $self->{REQ}->slurp_filename(0); # untainted
+    $self->{CODE} = eval { $self->{REQ}->slurp_filename(0) }; # untainted
+
+    if ($@) {
+        $self->log_error("$@");
+    
+        if (ref $@ eq 'APR::Error') {
+            if ($@->{rc} == APR::EACCES) {
+                $rc = Apache::FORBIDDEN;
+            } elsif ($@->{rc} == APR::ENOENT) {
+                $rc = Apache::NOT_FOUND;
+            } else {
+                $rc = Apache::SERVER_ERROR;
+            }
+        }
+    }
+
+    return $rc;
 }
 
 #########################################################################

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


Mime
View raw message