Even when not evaling/cache-checking, we need to lock to prepend to

grisha · grisha · commit f8aaf8ee3ae2 · 2013-10-08T01:24:58.000-04:00
path.

Fix a bug where accessing an invalid attribute in hlist would segfault.

There is no need to do any directory-related stuff if we are in a
Location context. Added d_is_location flag to the hlist
structure, and added checks for it.

Skip map-to-storage phase when we are wrapped in a Location (it
attempts to do a directory_walk). The implication is that
Location trumps all Directory configs which have no effect. But
it's much faster this way.

Track down where in core.c the trailing slash is added as part of
the why-are-we-doing-this research with respect to appending
trailing slashes to all directories.
diff --git a/lib/python/mod_python/apache.py b/lib/python/mod_python/apache.py
@@ -205,8 +205,13 @@ def FilterDispatch(self, filter):
                 finally:
                     _path_cache_lock.release()
             else:
-                if filter.dir and (filter.dir not in sys.path):
-                    sys.path[:0] = [filter.dir]
+                if filter.dir:
+                    _path_cache_lock.acquire()
+                    try:
+                        if filter.dir not in sys.path:
+                            sys.path[:0] = [filter.dir]
+                    finally:
+                        _path_cache_lock.release()
 
             # import module
             autoreload = True
@@ -352,10 +357,15 @@ def HandlerDispatch(self, req):
                     finally:
                         _path_cache_lock.release()
                 else:
-                    dir = hlist.directory
-
-                    if dir and (dir not in sys.path):
-                        sys.path[:0] = [dir]
+                    if not hlist.is_location:
+                        directory = hlist.directory
+                        if directory:
+                            _path_cache_lock.acquire()
+                            try:
+                                if directory not in sys.path:
+                                    sys.path[:0] = [directory]
+                            finally:
+                                _path_cache_lock.release()
 
                 # import module
                 autoreload = True
diff --git a/src/hlistobject.c b/src/hlistobject.c
@@ -91,7 +91,7 @@ static void hlist_dealloc(hlistobject *self)
 
 static PyObject *hlist_getattr(hlistobject *self, char *name)
 {
-    PyObject *res;
+    PyObject *res, *md;
 
     res = Py_FindMethod(hlistmethods, (PyObject *)self, name);
     if (res != NULL)
@@ -106,8 +106,13 @@ static PyObject *hlist_getattr(hlistobject *self, char *name)
         return Py_None;
     }
 
-    return PyMember_GetOne((char*)self->head,
-                           find_memberdef(hlist_memberlist, name));
+    md = find_memberdef(hlist_memberlist, name);
+    if (!md) {
+        PyErr_SetString(PyExc_AttributeError, name);
+        return NULL;
+    }
+
+    return PyMember_GetOne((char*)self->head, md);
 }
 
 /**
diff --git a/src/mod_python.c b/src/mod_python.c
@@ -880,14 +880,8 @@ static py_config *python_create_config(apr_pool_t *p)
 
 static void *python_create_dir_config(apr_pool_t *p, char *dir)
 {
-
     py_config *conf = python_create_config(p);
-
-    /* make sure directory ends with a slash (TODO: Why?) */
-    if (dir && (dir[strlen(dir) - 1] != '/'))
-        conf->config_dir = apr_pstrcat(p, dir, "/", NULL);
-    else
-        conf->config_dir = apr_pstrdup(p, dir);
+    conf->config_dir = dir;
 
     return conf;
 }
@@ -1125,19 +1119,26 @@ static void determine_context(apr_pool_t *p, const cmd_parms* cmd,
      * this point if no pattern matching to be done at
      * a later time. */
 
-    if (!d_is_fnmatch && !regex) {
+    if (directory && !d_is_fnmatch && !regex && !d_is_location) {
 
-        char *newpath = 0;
+        char *newpath = NULL;
         apr_status_t rv;
 
+        /* NB: if directory is not absolute, CWD will be prepended to
+         * it. If directory is NULL (we make sure it is not NULL
+         * above), then newpath will become CWD */
         rv = apr_filepath_merge(&newpath, NULL, directory,
                                 APR_FILEPATH_TRUENAME, p);
 
-        /* Should be able to ignore a failure as any
-         * problem with path should have been flagged
-         * when configuration was read in. */
-
         if (rv == APR_SUCCESS || rv == APR_EPATHWILD) {
+            /*
+             * We are appending a trailing slash here to be consistent
+             * with what httpd's core.c does in dirsection(). I [GT]
+             * am not very clear why core.c does it, especially given
+             * that it appends the trailing slash even to things that
+             * aren't really directories, e.g. when directory is an
+             * fnmatch pattern such as "/foo/bar*"
+             */
             directory = newpath;
             if (directory[strlen(directory) - 1] != '/') {
                 directory = apr_pstrcat(p, directory, "/", NULL);
@@ -1196,10 +1197,10 @@ static const char *python_directive_handler(cmd_parms *cmd, py_config* conf,
     char d_is_location = 0;
     ap_regex_t *regex = NULL;
 
-    /* d_is_location is used by the map_to_storage handler - there is no need for it to
-       run, if we are inside a Location */
-
     determine_context(cmd->pool, cmd, &directory, &d_is_fnmatch, &d_is_location, &regex);
+
+    /* d_is_location is used by the map_to_storage handler - there is no need for it to
+       run, if we are inside a Location. We also do not prepend a Location to sys.path. */
     conf->d_is_location = d_is_location;
 
     /* a handler may be restricted to certain file type by
@@ -1394,22 +1395,19 @@ static const char *select_interp_name(request_rec *req, conn_rec *con,
 
             /* base interpreter on directory where the file is found */
             if (req && ap_is_directory(req->pool, req->filename)) {
-                /* Where req->filename is a directory, it is not guaranteed
-                 * that it will have a trailing slash in req->filename as
-                 * trailing slash redirection only happens in mod_dir at
-                 * the very end of the fixup phase. Thus need to ensure
-                 * that this is taken into consideration.
+                /*
+                 * We are appending a trailing slash here to be
+                 * consistent with what httpd's core.c does in
+                 * dirsection(), or else we will end up with a
+                 * different interpreter named without the slash.
                  */
-                if (req->filename[strlen(req->filename)-1]=='/') {
+                if (req->filename[strlen(req->filename)-1]=='/')
                     return ap_make_dirstr_parent(req->pool, req->filename);
-                }
-                else {
+                else
                     return ap_make_dirstr_parent(req->pool,
-                                                apr_pstrcat(req->pool, req->filename,
-                                                            "/", NULL ));
-                }
-            }
-            else {
+                                                 apr_pstrcat(req->pool, req->filename,
+                                                             "/", NULL ));
+            } else {
                 if (req && req->filename)
                     return ap_make_dirstr_parent(req->pool, req->filename);
                 else
@@ -1532,10 +1530,11 @@ static int python_handler(request_rec *req, char *phase)
             hlist_extend(req->pool, hlohle, dynhle);
     }
 
-    /* resolve wildcard or regex directory patterns */
+    /* resolve wildcard or regex directory patterns. we do not resolve
+     * Location patterns because there is no use case for it so far */
     hle = hlohle;
     while (hle) {
-        if (hle->d_is_fnmatch || hle->regex) {
+        if (!hle->d_is_location && (hle->d_is_fnmatch || hle->regex)) {
             hle->directory = resolve_directory(req, hle->directory,
                                                hle->d_is_fnmatch, hle->regex);
             hle->d_is_fnmatch = 0;
diff --git a/test/htdocs/tests.py b/test/htdocs/tests.py
@@ -1270,14 +1270,8 @@ def PythonOption_items(req):
     return apache.OK
 
 def interpreter(req):
-    if req.phase == "PythonFixupHandler":
-        if req.filename[-1] != '/' and os.path.isdir(req.filename):
-            req.write(req.interpreter)
-            return apache.DONE
-        return apache.OK
-    else:
-        req.write(req.interpreter)
-        return apache.DONE
+    req.write(req.interpreter)
+    return apache.DONE
 
 def index(req):
     return "test ok, interpreter=%s" % req.interpreter
diff --git a/test/test.py b/test/test.py
@@ -1648,7 +1648,6 @@ def test_interpreter_per_directory_conf(self):
                                   PythonInterpPerDirectory('On'),
                                   SetHandler("mod_python"),
                                   PythonFixupHandler("tests::interpreter"),
-                                  PythonHandler("tests::interpreter"),
                                   PythonDebug("On")),
                         )
 

Original file line number	Diff line number	Diff line change
`@@ -1648,7 +1648,6 @@ def test_interpreter_per_directory_conf(self):`
`1648`	`1648`	`PythonInterpPerDirectory('On'),`
`1649`	`1649`	`SetHandler("mod_python"),`
`1650`	`1650`	`PythonFixupHandler("tests::interpreter"),`
`1651`		`- PythonHandler("tests::interpreter"),`
`1652`	`1651`	`PythonDebug("On")),`
`1653`	`1652`	`)`
`1654`	`1653`