Provide a way to allow get_resource and get_parent_resource to return errors

that might occur during the parsing of the URI and/or the lookup of the
resource in the repository.

Specifically: return a dav_error* and move the returned dav_resource* to an
"out" parameter of the hook function.

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

diff --git a/modules/dav/fs/repos.c b/modules/dav/fs/repos.c
index 68070351b938c90406192a95859e826b84a398f3..a6a102ee2435e4a5bd02ac0f965acc72c398e0b0 100644 (file)
--- a/modules/dav/fs/repos.c
+++ b/modules/dav/fs/repos.c
@@ -591,11 +591,12 @@ static dav_error *dav_fs_deleteset(apr_pool_t *p, const dav_resource *resource)
 ** REPOSITORY HOOK FUNCTIONS
 */
 
-static dav_resource * dav_fs_get_resource(
+static dav_error * dav_fs_get_resource(
     request_rec *r,
     const char *root_dir,
     const char *target,
-    int is_label)
+    int is_label,
+    dav_resource **result_resource)
 {
     dav_resource_private *ctx;
     dav_resource *resource;
@@ -679,7 +680,10 @@ static dav_resource * dav_fs_get_resource(
                ** be in path_info. The resource is simply an error: it
                ** can't be a null or a locknull resource.
                */
-               return NULL;    /* becomes HTTP_NOT_FOUND */
+                return dav_new_error(r->pool, HTTP_BAD_REQUEST, 0,
+                                     "The URL contains extraneous path "
+                                     "components. The resource could not "
+                                     "be identified.");
            }
 
            /* retain proper integrity across the structures */
@@ -689,10 +693,12 @@ static dav_resource * dav_fs_get_resource(
        }
     }
 
-    return resource;
+    *result_resource = resource;
+    return NULL;
 }
 
-static dav_resource * dav_fs_get_parent_resource(const dav_resource *resource)
+static dav_error * dav_fs_get_parent_resource(const dav_resource *resource,
+                                              dav_resource **result_parent)
 {
     dav_resource_private *ctx = resource->info;
     dav_resource_private *parent_ctx;
@@ -706,8 +712,10 @@ static dav_resource * dav_fs_get_parent_resource(const dav_resource *resource)
 #else
         strcmp(ctx->pathname, "/") == 0
 #endif
-       )
+       ) {
+        *result_parent = NULL;
         return NULL;
+    }
 
     /* ### optimize this into a single allocation! */
 
@@ -740,7 +748,8 @@ static dav_resource * dav_fs_get_parent_resource(const dav_resource *resource)
         parent_resource->exists = 1;
     }
 
-    return parent_resource;
+    *result_parent = parent_resource;
+    return NULL;
 }
 
 static int dav_fs_is_same_resource(

diff --git a/modules/dav/main/mod_dav.c b/modules/dav/main/mod_dav.c
index 49d4102f0bbff4947801f50bc0f163523584883d..9e71897bf2122cf45e83d8ded066f9a17b09eb34 100644 (file)
--- a/modules/dav/main/mod_dav.c
+++ b/modules/dav/main/mod_dav.c
@@ -618,39 +618,54 @@ static int dav_get_overwrite(request_rec *r)
  * by either a DAV:version or DAV:label-name element (passed as
  * the target argument), or any Target-Selector header in the request.
  */
-static int dav_get_resource(request_rec *r, int target_allowed,
-                            ap_xml_elem *target, dav_resource **res_p)
+static dav_error * dav_get_resource(request_rec *r, int target_allowed,
+                                    ap_xml_elem *target, dav_resource **res_p)
 {
     void *data;
     dav_dir_conf *conf;
     const char *target_selector = NULL;
     int is_label = 0;
     int result;
+    dav_error *err;
 
-    /* go look for the resource if it isn't already present */
+    /* only look for the resource if it isn't already present */
     (void) apr_get_userdata(&data, DAV_KEY_RESOURCE, r->pool);
     if (data != NULL) {
         *res_p = data;
-        return OK;
+        return NULL;
     }
 
     /* if the request target can be overridden, get any target selector */
     if (target_allowed) {
+        /* ### this should return a dav_error* */
         if ((result = dav_get_target_selector(r, target,
                                               &target_selector,
                                               &is_label)) != OK)
-           return result;
+            return dav_new_error(r->pool, result, 0,
+                                 "Could not process the method target.");
     }
 
     conf = ap_get_module_config(r->per_dir_config, &dav_module);
     /* assert: conf->provider != NULL */
 
     /* resolve the resource */
-    *res_p = (*conf->provider->repos->get_resource)(r, conf->dir,
-                                                    target_selector, is_label);
+    err = (*conf->provider->repos->get_resource)(r, conf->dir,
+                                                 target_selector, is_label,
+                                                 res_p);
+    if (err != NULL) {
+        err = dav_push_error(r->pool, HTTP_INTERNAL_SERVER_ERROR, 0,
+                             "Could not fetch resource information.", err);
+        return err;
+    }
+
+    /* Note: this shouldn't happen, but just be sure... */
     if (*res_p == NULL) {
-        /* Apache will supply a default error for this. */
-        return HTTP_NOT_FOUND;
+        /* ### maybe use HTTP_INTERNAL_SERVER_ERROR */
+        return dav_new_error(r->pool, HTTP_NOT_FOUND, 0,
+                             apr_psprintf(r->pool,
+                                          "The provider did not define a "
+                                          "resource for %s.",
+                                          ap_escape_html(r->pool, r->uri)));
     }
 
     (void) apr_set_userdata(*res_p, DAV_KEY_RESOURCE, apr_null_cleanup,
@@ -661,7 +676,7 @@ static int dav_get_resource(request_rec *r, int target_allowed,
      * add it now */
     dav_add_vary_header(r, r, *res_p);
 
-    return OK;
+    return NULL;
 }
 
 static dav_error * dav_open_lockdb(request_rec *r, int ro, dav_lockdb **lockdb)
@@ -715,14 +730,15 @@ static int dav_method_get(request_rec *r)
 {
     dav_resource *resource;
     int result;
+    dav_error *err;
 
     /* This method should only be called when the resource is not
      * visible to Apache. We will fetch the resource from the repository,
      * then create a subrequest for Apache to handle.
      */
-    result = dav_get_resource(r, 1 /*target_allowed*/, NULL, &resource);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(r, 1 /*target_allowed*/, NULL, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
     if (!resource->exists) {
         /* Apache will supply a default error for this. */
         return HTTP_NOT_FOUND;
@@ -912,13 +928,11 @@ static int dav_method_post(request_rec *r)
 {
     dav_resource *resource;
     dav_error *err;
-    int result;
 
     /* Ask repository module to resolve the resource */
-    result = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
-    if (result != OK) {
-        return result;
-    }
+    err = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
 
     /* Note: depth == 0. Implies no need for a multistatus response. */
     if ((err = dav_validate_request(r, resource, 0, NULL, NULL,
@@ -953,10 +967,9 @@ static int dav_method_put(request_rec *r)
     }
 
     /* Ask repository module to resolve the resource */
-    result = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
-    if (result != OK) {
-        return result;
-    }
+    err = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
 
     /* If not a file or collection resource, PUT not allowed */
     if (resource->type != DAV_RESOURCE_TYPE_REGULAR) {
@@ -1168,9 +1181,9 @@ static int dav_method_delete(request_rec *r)
     }
 
     /* Ask repository module to resolve the resource */
-    result = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
     if (!resource->exists) {
         /* Apache will supply a default error for this. */
        return HTTP_NOT_FOUND;
@@ -1522,11 +1535,12 @@ static int dav_method_options(request_rec *r)
     apr_array_header_t *uri_ary;
     ap_xml_doc *doc;
     const ap_xml_elem *elem;
+    dav_error *err;
 
     /* resolve the resource */
-    result = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
 
     /* parse any request body */
     if ((result = ap_xml_parse_input(r, &doc)) != OK) {
@@ -1872,9 +1886,9 @@ static int dav_method_propfind(request_rec *r)
     dav_response *multi_status;
 
     /* Ask repository module to resolve the resource */
-    result = dav_get_resource(r, 1 /*target_allowed*/, NULL, &resource);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(r, 1 /*target_allowed*/, NULL, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
 
     if (dav_get_resource_state(r, resource) == DAV_RESOURCE_NULL) {
        /* Apache will supply a default error for this. */
@@ -2136,9 +2150,9 @@ static int dav_method_proppatch(request_rec *r)
     dav_prop_ctx *ctx;
 
     /* Ask repository module to resolve the resource */
-    result = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
     if (!resource->exists) {
        /* Apache will supply a default error for this. */
        return HTTP_NOT_FOUND;
@@ -2336,9 +2350,9 @@ static int dav_method_mkcol(request_rec *r)
                                                 &dav_module);
 
     /* Ask repository module to resolve the resource */
-    result = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
 
     if (resource->exists) {
        /* oops. something was already there! */
@@ -2455,9 +2469,9 @@ static int dav_method_copymove(request_rec *r, int is_move)
     int resource_state;
 
     /* Ask repository module to resolve the resource */
-    result = dav_get_resource(r, !is_move /*target_allowed*/, NULL, &resource);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(r, !is_move /*target_allowed*/, NULL, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
     if (!resource->exists) {
        /* Apache will supply a default error for this. */
        return HTTP_NOT_FOUND;
@@ -2509,9 +2523,9 @@ static int dav_method_copymove(request_rec *r, int is_move)
     }
 
     /* Resolve destination resource */
-    result = dav_get_resource(lookup.rnew, 0 /*target_allowed*/, NULL, &resnew);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(lookup.rnew, 0 /*target_allowed*/, NULL, &resnew);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
 
     /* are the two resources handled by the same repository? */
     if (resource->hooks != resnew->hooks) {
@@ -2838,9 +2852,9 @@ static int dav_method_lock(request_rec *r)
      * so allow it, and let provider reject the lock attempt
      * on a version if it wants to.
      */
-    result = dav_get_resource(r, 1 /*target_allowed*/, NULL, &resource);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(r, 1 /*target_allowed*/, NULL, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
 
     /*
     ** Open writable. Unless an error occurs, we'll be
@@ -3025,9 +3039,9 @@ static int dav_method_unlock(request_rec *r)
      * so allow it, and let provider reject the unlock attempt
      * on a version if it wants to.
      */
-    result = dav_get_resource(r, 1 /*target_allowed*/, NULL, &resource);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(r, 1 /*target_allowed*/, NULL, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
 
     resource_state = dav_get_resource_state(r, resource);
 
@@ -3083,9 +3097,9 @@ static int dav_method_vsn_control(request_rec *r)
         return DECLINED;
 
     /* ask repository module to resolve the resource */
-    result = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
 
     /* remember the pre-creation resource state */
     resource_state = dav_get_resource_state(r, resource);
@@ -3284,9 +3298,9 @@ static int dav_method_checkout(request_rec *r)
     }
 
     /* Ask repository module to resolve the resource */
-    result = dav_get_resource(r, 1 /*target_allowed*/, target, &resource);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(r, 1 /*target_allowed*/, target, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
     if (!resource->exists) {
         /* Apache will supply a default error for this. */
         return HTTP_NOT_FOUND;
@@ -3349,9 +3363,9 @@ static int dav_method_uncheckout(request_rec *r)
     }
 
     /* Ask repository module to resolve the resource */
-    result = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
     if (!resource->exists) {
         /* Apache will supply a default error for this. */
         return HTTP_NOT_FOUND;
@@ -3412,9 +3426,9 @@ static int dav_method_checkin(request_rec *r)
     }
 
     /* Ask repository module to resolve the resource */
-    result = dav_get_resource(r, 0 /* target_allowed */, NULL, &resource);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(r, 0 /* target_allowed */, NULL, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
     if (!resource->exists) {
         /* Apache will supply a default error for this. */
         return HTTP_NOT_FOUND;
@@ -3518,9 +3532,9 @@ static int dav_method_set_target(request_rec *r)
         return DECLINED;
 
     /* Ask repository module to resolve the resource */
-    result = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
     if (!resource->exists) {
         /* Apache will supply a default error for this. */
         return HTTP_NOT_FOUND;
@@ -3700,9 +3714,9 @@ static int dav_method_label(request_rec *r)
         return DECLINED;
 
     /* Ask repository module to resolve the resource */
-    result = dav_get_resource(r, 1 /*target_allowed*/, NULL, &resource);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(r, 1 /*target_allowed*/, NULL, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
     if (!resource->exists) {
         /* Apache will supply a default error for this. */
         return HTTP_NOT_FOUND;
@@ -3835,9 +3849,9 @@ static int dav_method_report(request_rec *r)
      * for this report.
      */
     target_allowed = (*vsn_hooks->report_target_selector_allowed)(doc);
-    result = dav_get_resource(r, target_allowed, NULL, &resource);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(r, target_allowed, NULL, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
     if (!resource->exists) {
         /* Apache will supply a default error for this. */
         return HTTP_NOT_FOUND;
@@ -3882,9 +3896,9 @@ static int dav_method_make_workspace(request_rec *r)
         return DECLINED;
 
     /* ask repository module to resolve the resource */
-    result = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(r, 0 /*target_allowed*/, NULL, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
 
     /* parse the request body (must be a mkworkspace element) */
     if ((result = ap_xml_parse_input(r, &doc)) != OK) {
@@ -3961,16 +3975,15 @@ static int dav_method_bind(request_rec *r)
     dav_response *multi_response = NULL;
     dav_lookup_result lookup;
     int overwrite;
-    int result;
 
     /* If no bindings provider, decline the request */
     if (binding_hooks == NULL)
         return DECLINED;
 
     /* Ask repository module to resolve the resource */
-    result = dav_get_resource(r, 0 /*!target_allowed*/, NULL, &resource);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(r, 0 /*!target_allowed*/, NULL, &resource);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
     if (!resource->exists) {
        /* Apache will supply a default error for this. */
        return HTTP_NOT_FOUND;
@@ -4015,9 +4028,9 @@ static int dav_method_bind(request_rec *r)
     }
 
     /* resolve binding resource */
-    result = dav_get_resource(lookup.rnew, 0 /*!target_allowed*/, NULL, &binding);
-    if (result != OK)
-        return result;
+    err = dav_get_resource(lookup.rnew, 0 /*!target_allowed*/, NULL, &binding);
+    if (err != NULL)
+        return dav_handle_err(r, err, NULL);
 
     /* are the two resources handled by the same repository? */
     if (resource->hooks != binding->hooks) {

diff --git a/modules/dav/main/mod_dav.h b/modules/dav/main/mod_dav.h
index 6d553692d8aea44813c98a50bc2647405f5d6063..22902c9cf76f16c884df55aabe49219689fcba88 100644 (file)
--- a/modules/dav/main/mod_dav.h
+++ b/modules/dav/main/mod_dav.h
@@ -1566,32 +1566,41 @@ struct dav_hooks_repository
      */
     int handle_get;
 
-    /* Get a resource descriptor for the URI in a request.
-     * A descriptor is returned even if the resource does not exist.
-     * The return value should only be NULL for some kind of fatal error.
+    /* Get a resource descriptor for the URI in a request. A descriptor
+     * should always be returned even if the resource does not exist. This
+     * repository has been identified as handling the resource given by
+     * the URI, so an answer must be given. If there is a problem with the
+     * URI or accessing the resource or whatever, then an error should be
+     * returned.
      *
-     * The root_dir is the root of the directory for which this repository
-     * is configured.
-     * The target is either a label, or a version URI, or NULL. If there
-     * is a target, then is_label specifies whether the target is a label
-     * or a URI.
+     * root_dir: the root of the directory for which this repository is
+     *           configured.
+     * target: is either a label, or a version URI, or NULL. If there is
+     *         a target, then is_label specifies whether the target is a
+     *         label or a URI.
      *
-     * The provider may associate the request storage pool with the resource,
-     * to use in other operations on that resource.
+     * The provider may associate the request storage pool with the resource
+     * (in the resource->pool field), to use in other operations on that
+     * resource. 
      */
-    dav_resource * (*get_resource)(
+    dav_error * (*get_resource)(
         request_rec *r,
         const char *root_dir,
        const char *target,
-        int is_label
+        int is_label,
+        dav_resource **resource
     );
 
     /* Get a resource descriptor for the parent of the given resource.
      * The resources need not exist.  NULL is returned if the resource 
      * is the root collection.
+     *
+     * An error should be returned only if there is a fatal error in
+     * fetching information about the parent resource.
      */
-    dav_resource * (*get_parent_resource)(
-        const dav_resource *resource
+    dav_error * (*get_parent_resource)(
+        const dav_resource *resource,
+        dav_resource **parent_resource
     );
 
     /* Determine whether two resource descriptors refer to the same resource.

diff --git a/modules/dav/main/util.c b/modules/dav/main/util.c
index 9aa6a4f23779a66545f4b1ea0c9ffc83fedc981f..51ca37a3435bea4763ae05f8af69c5d564cfac3f 100644 (file)
--- a/modules/dav/main/util.c
+++ b/modules/dav/main/util.c
@@ -1345,13 +1345,15 @@ dav_error * dav_validate_request(request_rec *r, dav_resource *resource,
 
     /* (2) Validate the parent resource if requested */
     if (err == NULL && (flags & DAV_VALIDATE_PARENT)) {
-        dav_resource *parent_resource = (*repos_hooks->get_parent_resource)(resource);
+        dav_resource *parent_resource;
 
-       if (parent_resource == NULL) {
+        err = (*repos_hooks->get_parent_resource)(resource, &parent_resource);
+
+       if (err == NULL && parent_resource == NULL) {
            err = dav_new_error(r->pool, HTTP_FORBIDDEN, 0,
                                "Cannot access parent of repository root.");
        }
-       else {
+       else if (err == NULL) {
            err = dav_validate_resource_state(r->pool, parent_resource, lockdb,
                                              if_header,
                                               flags | DAV_VALIDATE_IS_PARENT,
@@ -1603,7 +1605,12 @@ dav_error *dav_ensure_resource_writable(request_rec *r,
 
     /* check parent resource if requested or if resource must be created */
     if (!resource->exists || parent_only) {
-       dav_resource *parent = (*resource->hooks->get_parent_resource)(resource);
+       dav_resource *parent;
+
+        if ((err = (*resource->hooks->get_parent_resource)(resource,
+                                                           &parent)) != NULL)
+            return err;
+
         if (parent == NULL || !parent->exists) {
            body = apr_psprintf(r->pool,
                                 "Missing one or more intermediate collections. "

diff --git a/modules/dav/main/util_lock.c b/modules/dav/main/util_lock.c
index 3a138dd16faad9e15e05991f82fe28de4ed02620..cd54fac2fdc55c7fd09c974b087c2d8013e31a85 100644 (file)
--- a/modules/dav/main/util_lock.c
+++ b/modules/dav/main/util_lock.c
@@ -460,6 +460,7 @@ static dav_error * dav_get_direct_resource(apr_pool_t *p,
     while (resource != NULL) {
        dav_error *err;
        dav_lock *lock;
+        dav_resource *parent;
 
        /*
        ** Find the lock specified by <locktoken> on <resource>. If it is
@@ -488,7 +489,12 @@ static dav_error * dav_get_direct_resource(apr_pool_t *p,
        }
 
        /* the lock was indirect. move up a level in the URL namespace */
-       resource = (*resource->hooks->get_parent_resource)(resource);
+       if ((err = (*resource->hooks->get_parent_resource)(resource,
+                                                           &parent)) != NULL) {
+            /* ### add a higher-level desc? */
+            return err;
+        }
+        resource = parent;
     }
 
     return dav_new_error(p, HTTP_INTERNAL_SERVER_ERROR, 0,
@@ -625,14 +631,20 @@ static dav_error * dav_inherit_locks(request_rec *r, dav_lockdb *lockdb,
     dav_response *multi_status;
 
     if (use_parent) {
-       which_resource = (*repos_hooks->get_parent_resource)(resource);
-       if (which_resource == NULL) {
+        dav_resource *parent;
+       if ((err = (*repos_hooks->get_parent_resource)(resource,
+                                                       &parent)) != NULL) {
+            /* ### add a higher-level desc? */
+            return err;
+        }
+       if (parent == NULL) {
            /* ### map result to something nice; log an error */
            return dav_new_error(r->pool, HTTP_INTERNAL_SERVER_ERROR, 0,
                                 "Could not fetch parent resource. Unable to "
                                 "inherit locks from the parent and apply "
                                 "them to this resource.");
        }
+        which_resource = parent;
     }
     else {
        which_resource = resource;