Project

General

Profile

Actions

Feature #2924

closed

Suboptimal keep-alive handling when request body is not read

Added by helmut almost 6 years ago. Updated almost 6 years ago.

Status:
Fixed
Priority:
Normal
Category:
core
Target version:
ASK QUESTIONS IN Forums:

Description

Consider the following situation:
  • A HTTP client performs a HTTP request.
  • The request is eligible for keep-alive (e.g. because HTTP/1.1 was used)
  • The request includes a request body (e.g. POST)
  • lighttpd determines a reply without reading the request body (e.g. mod_auth returns 401)

In this case, the connection_handle_response_end_state function determines that keep-alive is not possible and initiates a connection shutdown. A remote client may be unprepared for such a shutdown, because lighttpd did not include a "Connection: close" header in its reply and attempt a second request on the same connection. This is subject to a race condition (i.e. if the shutdown is deferred compared to the response). When reproducing the issue, I hit that race at most in 0.1% of the attempts (i.e. quite unlikely). A client that is prone to hitting it is python-requests. python-requests' behaviour is suboptimal as well as is documented at https://github.com/requests/requests/issues/4664.

I propose that when lighttpd sends its response headers, it performs the same check as in connection_handle_response_end_state and adds the relevant Connection: close header. "gps" pointed out that this may be undesirable when using server.stream-request-body = 1 with mod_proxy. Requiring server.stream-request-body = 0 is a reasonable compromise to me. Is the patch a reasonable trade-off?


Files

connection_close.patch (1.11 KB) connection_close.patch helmut, 2019-01-09 07:44
Actions #1

Updated by helmut almost 6 years ago

I should also mention relevant versions:
  • Not reproducible on 1.4.35
  • Reproducible on 1.4.45
  • Reproducible on 1.4.52
Actions #2

Updated by gstrauss almost 6 years ago

  • Tracker changed from Bug to Feature
  • Target version changed from 1.4.x to 1.4.53

A remote client may be unprepared for such a shutdown, because lighttpd did not include a "Connection: close" header in its reply and attempt a second request on the same connection.

That is a bug, but not in lighttpd.

This ticket is a feature request.

The patch as provided is too blunt. As noted, it may result in premature closure of keep-alive connections when server.stream-request-body = 1 is in use and the backend has not yet read the full request body.

The following might be a better solution, since it will trigger "Connection: close" if the response body has not been fully read and one of: there is no backend which might be streaming the request body or there is no streaming request body. The short-circuit to check for any content length in the patch below is because many requests are GET requests without a request body.

     /* disable keep-alive if requested */
     if (con->request_count > con->conf.max_keep_alive_requests || 0 == con->conf.max_keep_alive_idle) {
         con->keep_alive = 0;
+    } else if (0 != con->request.content_length
+               && con->request.content_length != con->request_content_queue->bytes_in
+               && (con->mode == DIRECT || 0 == con->conf.stream_request_body)) {
+        con->keep_alive = 0;
     } else {
         con->keep_alive_idle = con->conf.max_keep_alive_idle;
     }
Actions #3

Updated by gstrauss almost 6 years ago

  • Status changed from New to Fixed
  • % Done changed from 0 to 100
Actions #4

Updated by helmut almost 6 years ago

Thank you for your review and fixing the issue with improved checks. I expect that your version fully resolves the pratical problems I'm seeing.

Actions

Also available in: Atom