Windows: use an unnested mutex like unix for LDAP

On Windows, a subrequest (against a different per-dir config?)
could get and cleanup an LDC that the main subrequest will also
cleanup.

Also don't use ldc->r for trace if null for any reason

Submitted by: covener

Github: closes #830


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1939088 13f79535-47bb-0310-9956-ffa450edef68

modules/aaa/mod_authnz_ldap.c

@@ -369,7 +369,10 @@ static void *create_authnz_ldap_dir_config(apr_pool_t *p, char *d)
 static apr_status_t authnz_ldap_cleanup_connection_close(void *param)
 {
     util_ldap_connection_t *ldc = param;
-    ap_log_rerror(APLOG_MARK, APLOG_TRACE4, 0, ldc->r, "Release ldc %pp", ldc);
+    /* NULL ldc->r implies some kind of mismanagement/reuse but the close is tolerant */
+    if (ldc->r) {
+        ap_log_rerror(APLOG_MARK, APLOG_TRACE4, 0, ldc->r, "Release ldc %pp", ldc);
+    }
     util_ldap_connection_close(ldc);
     return APR_SUCCESS;
 }

modules/ldap/util_ldap.c

@@ -908,7 +908,12 @@ static util_ldap_connection_t *
         l->st = st;
 
 #if APR_HAS_THREADS
-        apr_thread_mutex_create(&l->lock, APR_THREAD_MUTEX_DEFAULT, l->pool);
+        /* uldap_connection_find() relies on apr_thread_mutex_trylock() failing
+         * for a connection this thread already holds. APR_THREAD_MUTEX_DEFAULT
+         * is a recursive critical section on Windows, which would hand the same
+         * connection back to a nested caller, so ask for a non-recursive lock.
+         */
+        apr_thread_mutex_create(&l->lock, APR_THREAD_MUTEX_UNNESTED, l->pool);
         apr_thread_mutex_lock(l->lock);
 #endif
         l->bound = 0;

test/pytest_suite/t/conf/extra.conf.in

@@ -884,6 +884,8 @@ LimitRequestFields    32
   Alias /modules/ldap/group @DocumentRoot@
   Alias /modules/ldap/refer @DocumentRoot@
   Alias /modules/ldap/search @DocumentRoot@
+  Alias /modules/ldap/subreq @DocumentRoot@
+  Alias /modules/ldap/target @DocumentRoot@
 
   # Simple user lookup
   <Location /modules/ldap/simple>
@@ -924,6 +926,40 @@ LimitRequestFields    32
      Require ldap-search cn=Subgroup
   </Location>
 
+  # Subrequest issued from the fixups phase (mod_rewrite -U) into a second
+  # per-dir config with identical LDAP settings. A subrequest only re-runs
+  # authn/authz when its per-dir config differs from the parent's, and at
+  # mod_rewrite's fixup the parent still holds the LDAP connection it used
+  # for authz. On platforms where APR_THREAD_MUTEX_DEFAULT is recursive
+  # (Windows) the subrequest used to be handed the parent's connection, and
+  # the parent's cleanup then ran against an already released connection.
+  <IfModule mod_rewrite.c>
+  <Location /modules/ldap/subreq>
+     AuthLDAPURL "ldap://localhost:8389/dc=example,dc=com?uid"
+     AuthLDAPBindDN "cn=httpd,dc=example,dc=com"
+     AuthLDAPBindPassword mod_authnz_ldap
+     AuthType Basic
+     AuthName ldap-subreq@httpd.apache.org
+     AuthBasicProvider ldap
+     Require ldap-group cn=Group One,dc=example,dc=com
+     RewriteEngine On
+     RewriteCond /modules/ldap/target/foobar.html -U
+     RewriteRule ^ - [E=LDAP_SUBREQ:ok]
+     <IfModule mod_headers.c>
+        Header set X-LDAP-Subreq "%{LDAP_SUBREQ}e"
+     </IfModule>
+  </Location>
+  <Location /modules/ldap/target>
+     AuthLDAPURL "ldap://localhost:8389/dc=example,dc=com?uid"
+     AuthLDAPBindDN "cn=httpd,dc=example,dc=com"
+     AuthLDAPBindPassword mod_authnz_ldap
+     AuthType Basic
+     AuthName ldap-target@httpd.apache.org
+     AuthBasicProvider ldap
+     Require ldap-group cn=Group One,dc=example,dc=com
+  </Location>
+  </IfModule>
+
 </IfDefine>
 
 ##

test/pytest_suite/tests/t/modules/test_ldap.py

@@ -27,6 +27,8 @@ CASES = [
     ("/modules/ldap/refer/", "alpha", "Alpha", 401),
     ("/modules/ldap/refer/", "beta", "Beta", 200),
     ("/modules/ldap/search/", "unchecked", "unchecked", 200),
+    ("/modules/ldap/subreq/foobar.html", "delta", "Delta", 200),
+    ("/modules/ldap/target/foobar.html", "delta", "Delta", 200),
 ]
 
 
@@ -46,3 +48,18 @@ def test_ldap(http, url, username, password, expected):
         r = http.GET(url)
     creds = f"{username}/{password}" if username else "no credentials"
     assert t_cmp(r.status_code, expected), f"test for {url} with {creds}"
+
+
+@need_module("authnz_ldap", "rewrite", "headers")
+def test_ldap_fixups_subrequest(http):
+    """A mod_rewrite -U subrequest from the parent's fixups phase lands in a
+    second per-dir config with the same LDAP settings, so its own LDAP authn
+    and authz run while the parent still holds its authz connection. The
+    parent must complete, and the header proves the subrequest succeeded."""
+    if not _ldap_configured(http):
+        pytest.skip("LDAP testing not configured (run with --defines LDAP)")
+    r = http.GET("/modules/ldap/subreq/foobar.html",
+                 auth=httpx.BasicAuth("delta", "Delta"))
+    assert t_cmp(r.status_code, 200), "parent request with nested LDAP subrequest"
+    assert t_cmp(r.headers.get("x-ldap-subreq"), "ok"), \
+        "subrequest into second LDAP per-dir config did not succeed"