Mailing List Archive

Does the attached patch solve your issue?

Regards

Rüdiger

On 9/20/21 8:01 PM, Evgeny Kotkov wrote:
> Hi,
>
> I think that I have stumbled across a significant regression in httpd 2.4.49
> where mod_dav has been changed in a way that makes it ignore the errors
> returned by a versioning provider during REPORT requests.
>
> I haven't checked that in detail, but I think that r1892513 [1] is the change
> that is causing the new behavior.
>
> Consider the new core part of the dav_method_report() function:
>
> …
> result = dav_run_deliver_report(r, resource, doc,
> r->output_filters, &err);
> switch (result) {
> case OK:
> return DONE;
> case DECLINED:
> /* No one handled the report */
> return HTTP_NOT_IMPLEMENTED;
> default:
> if ((err) != NULL) {
> … handle the error
> }
>
> Assuming there are no deliver_report hooks, this code is going to call
> dav_core_deliver_report(), whose relevant part is as follows:
>
> …
> if (vsn_hooks) {
> *err = (*vsn_hooks->deliver_report)(r, resource, doc,
> r->output_filters);
> return OK;
> }
> …
>
> Here the "return OK" part skips the error handling on the calling site,
> even if there is an associated error returned in *err.
>
> In turn, this causes the following regression:
>
> - For a failed request where the server hasn't started sending the response,
> it is now going to erroneously send a 200 OK with an empty body instead of
> an error response with the appropriate HTTP status.
>
> - For a failed request where the server has started sending the response body,
> it is now going to handle it as if it was successfully completed instead of
> aborting the connection. The likely outcome of this is that the server is
> going to send a truncated payload with a 2xx status indicating success.
>
> This regression can cause unexpected behavior of the Subversion clients,
> for example in cases where the (ignored) error occurred due to a non-successful
> authorization check. Other DAV clients may be susceptible to some kinds of
> unexpected behavior as well.
>
> [1] https://svn.apache.org/r1892513
>
>
> Thanks,
> Evgeny Kotkov
>

Ruediger Pluem <rpluem@apache.org> writes:

> Does the attached patch solve your issue?

It does appear to solve the problem with missing errors, thanks!

I haven't checked that in detail, but I think there might be a discrepancy
in how `err` is handled in the patch and for example when calling the
method_precondition() hook.

With the patch, `err` is checked even if all hooks DECLINE the operation.
Not too sure if that's intended, because the variable could potentially
contain an arbitrary value or a leftover from some previous call.


Thanks,
Evgeny Kotkov

On 9/22/21 3:49 PM, Evgeny Kotkov wrote:
> Ruediger Pluem <rpluem@apache.org> writes:
>
>> Does the attached patch solve your issue?
>
> It does appear to solve the problem with missing errors, thanks!
>
> I haven't checked that in detail, but I think there might be a discrepancy
> in how `err` is handled in the patch and for example when calling the
> method_precondition() hook.
>
> With the patch, `err` is checked even if all hooks DECLINE the operation.
> Not too sure if that's intended, because the variable could potentially
> contain an arbitrary value or a leftover from some previous call.

The below new version should address the case that there was a left over in err from
calling dav_run_method_precondition by resetting err to NULL.
If we should ignore err if all hooks return DECLINED but set err, to be honest I don't know. I hope for someone with more DAV
insights to comment and tell me, what is correct here :-).
Depending on this it should be easy to adjust the patch accordingly if needed.


Index: modules/dav/main/mod_dav.c
===================================================================
--- modules/dav/main/mod_dav.c (revision 1893507)
+++ modules/dav/main/mod_dav.c (working copy)
@@ -4403,10 +4403,32 @@
/* set up defaults for the report response */
r->status = HTTP_OK;
ap_set_content_type(r, DAV_XML_CONTENT_TYPE);
+ err = NULL;

/* run report hook */
result = dav_run_deliver_report(r, resource, doc,
r->output_filters, &err);
+ if (err != NULL) {
+
+ if (! r->sent_bodyct) {
+ /* No data has been sent to client yet; throw normal error. */
+ return dav_handle_err(r, err, NULL);
+ }
+
+ /* If an error occurred during the report delivery, there's
+ basically nothing we can do but abort the connection and
+ log an error. This is one of the limitations of HTTP; it
+ needs to "know" the entire status of the response before
+ generating it, which is just impossible in these streamy
+ response situations. */
+ err = dav_push_error(r->pool, err->status, 0,
+ "Provider encountered an error while streaming"
+ " a REPORT response.", err);
+ dav_log_err(r, err, APLOG_ERR);
+ r->connection->aborted = 1;
+
+ return DONE;
+ }
switch (result) {
case OK:
return DONE;
@@ -4414,27 +4436,7 @@
/* No one handled the report */
return HTTP_NOT_IMPLEMENTED;
default:
- if ((err) != NULL) {
-
- if (! r->sent_bodyct) {
- /* No data has been sent to client yet; throw normal error. */
- return dav_handle_err(r, err, NULL);
- }
-
- /* If an error occurred during the report delivery, there's
- basically nothing we can do but abort the connection and
- log an error. This is one of the limitations of HTTP; it
- needs to "know" the entire status of the response before
- generating it, which is just impossible in these streamy
- response situations. */
- err = dav_push_error(r->pool, err->status, 0,
- "Provider encountered an error while streaming"
- " a REPORT response.", err);
- dav_log_err(r, err, APLOG_ERR);
- r->connection->aborted = 1;
-
- return DONE;
- }
+ return DONE;
}

return DONE;

On 9/22/21 4:08 PM, Ruediger Pluem wrote:
>
>
> On 9/22/21 3:49 PM, Evgeny Kotkov wrote:
>> Ruediger Pluem <rpluem@apache.org> writes:
>>
>>> Does the attached patch solve your issue?
>>
>> It does appear to solve the problem with missing errors, thanks!
>>
>> I haven't checked that in detail, but I think there might be a discrepancy
>> in how `err` is handled in the patch and for example when calling the
>> method_precondition() hook.
>>
>> With the patch, `err` is checked even if all hooks DECLINE the operation.
>> Not too sure if that's intended, because the variable could potentially
>> contain an arbitrary value or a leftover from some previous call.
>
> The below new version should address the case that there was a left over in err from
> calling dav_run_method_precondition by resetting err to NULL.
> If we should ignore err if all hooks return DECLINED but set err, to be honest I don't know. I hope for someone with more DAV
> insights to comment and tell me, what is correct here :-).
> Depending on this it should be easy to adjust the patch accordingly if needed.

r1893589

Regards

Rüdiger