diff --git a/modules/dav/main/ms_wdv.c b/modules/dav/main/ms_wdv.c index 1b80b9a8e2e..316e1b7724d 100644 --- a/modules/dav/main/ms_wdv.c +++ b/modules/dav/main/ms_wdv.c @@ -157,7 +157,8 @@ static dav_error *check_locked_by_other(request_rec *r) /* Let lock method fail the request */ out: - (*lockdb->hooks->close_lockdb)(lockdb); + if (lockdb) + (*lockdb->hooks->close_lockdb)(lockdb); return err; } diff --git a/test/pytest_suite/t/conf/extra.conf.in b/test/pytest_suite/t/conf/extra.conf.in index 1dfcb914b34..7146fa688ee 100644 --- a/test/pytest_suite/t/conf/extra.conf.in +++ b/test/pytest_suite/t/conf/extra.conf.in @@ -656,6 +656,7 @@ Alias /manual @inherit_documentroot@/manual DAV On + DAVMSext All diff --git a/test/pytest_suite/tests/t/modules/test_dav.py b/test/pytest_suite/tests/t/modules/test_dav.py index aed5fd26f79..5bb2797debe 100644 --- a/test/pytest_suite/tests/t/modules/test_dav.py +++ b/test/pytest_suite/tests/t/modules/test_dav.py @@ -207,3 +207,41 @@ def test_dav(http): os.rmdir(p) except OSError: pass + + +@need_module("dav") +def test_dav_mswdv_null_deref(http): + """BZ 70051: a PUT/DELETE/MOVE/LOCK whose dav_get_resource() fails (e.g. an + extraneous path component under an existing file) must not crash the + worker. Under DAVMSext, check_locked_by_other() in ms_wdv.c used to + dereference its lockdb unconditionally in the out: label, even on the + paths that goto out before open_lockdb() runs and leave it NULL. + """ + htdocs = http.vars("documentroot") + os.makedirs(os.path.join(htdocs, DIR), exist_ok=True) + name = "mswdv-70051.html" + fpath = os.path.join(htdocs, DIR, name) + if os.path.exists(fpath): + os.unlink(fpath) + + base = http.base_url + url = f"{base}/{DIR}/{name}" + + with httpx.Client(timeout=30.0) as c: + resp = c.put(url, content=BODY) + assert resp.is_success, f"seed PUT failed (got {resp.status_code})" + + # dav_fs_get_resource() rejects the extra path component below the + # plain file with an error -- dav_get_resource() fails before any + # lockdb is opened. Pre-fix, check_locked_by_other()'s "out:" label + # dereferenced that still-NULL lockdb and crashed the worker. + resp = c.put(f"{url}/extra", content="boom") + assert resp.status_code in (403, 404, 409), \ + f"PUT past a file should be a clean DAV error, got {resp.status_code}" + + # the worker must still be alive: a normal request must succeed. + resp = c.get(url) + assert resp.status_code == 200, \ + f"server did not survive the request (got {resp.status_code})" + + os.unlink(fpath)