View Issue Details

IDProjectCategoryView StatusLast Update
0005411libmicrohttpdcompliancepublic2021-09-02 17:54
Reportersamh Assigned ToChristian Grothoff  
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version0.9.59 
Target Version0.9.60Fixed in Version0.9.60 
Summary0005411: MHD incorrectly sets Transfer-Encoding when Content-Length header is present
DescriptionHi,

We have a piece of software which uses libmicrohttpd to serve HTTP responses. The data to be returned is of a known length, and as such we set the Content-Length header in the response. In our particular use-case, the client needs to know the full length of the response upfront.

However, we've observed that when setting the Content-Length header via MHD_add_response_header, there's nothing in MHD to flag that it must not (as per https://tools.ietf.org/html/rfc7230#section-3.3.2 ) send a Content-Length header in a response that contains a Transfer-Encoding header.

I've attached a quick patch which adds an additional flag into build_header_response so that Transfer-Encoding is not set if the application has already provided a Content-Length header.

Let me know what you think.

-Sam
TagsNo tags attached.
Attached Files
0001-Don-t-set-transfer-encoding-if-content-length-suppli.patch (2,155 bytes)   
From 406da99114e5fb972793897ab1dc0e973e3b8057 Mon Sep 17 00:00:00 2001
From: Sam Hurst <samuelh@rd.bbc.co.uk>
Date: Wed, 25 Jul 2018 12:05:49 +0100
Subject: [PATCH] Don't set transfer encoding if content-length supplied

---
 src/microhttpd/connection.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/microhttpd/connection.c b/src/microhttpd/connection.c
index 1778c59..555d18a 100644
--- a/src/microhttpd/connection.c
+++ b/src/microhttpd/connection.c
@@ -1419,6 +1419,7 @@ build_header_response (struct MHD_Connection *connection)
   bool client_requested_close;
   bool response_has_close;
   bool response_has_keepalive;
+  bool response_has_content_length;
   const char *have_encoding;
   const char *have_content_length;
   int must_add_close;
@@ -1483,6 +1484,7 @@ build_header_response (struct MHD_Connection *connection)
   must_add_keep_alive = MHD_NO;
   must_add_content_length = MHD_NO;
   response_has_close = false;
+  response_has_content_length = false;
   switch (connection->state)
     {
     case MHD_CONNECTION_FOOTERS_RECEIVED:
@@ -1495,6 +1497,8 @@ build_header_response (struct MHD_Connection *connection)
       client_requested_close = MHD_lookup_header_s_token_ci (connection,
                                                             MHD_HTTP_HEADER_CONNECTION,
                                                             "close");
+      if (NULL != MHD_get_response_header(connection->response, "Content-Length"))
+          response_has_content_length = true;
 
       if (0 != (connection->response->flags & MHD_RF_HTTP_VERSION_1_0_ONLY))
         connection->keepalive = MHD_CONN_MUST_CLOSE;
@@ -1512,7 +1516,8 @@ build_header_response (struct MHD_Connection *connection)
            (NULL == connection->response->upgrade_handler) &&
 #endif /* UPGRADE_SUPPORT */
            (! response_has_close) &&
-           (! client_requested_close) )
+           (! client_requested_close) &&
+           (! response_has_content_length))
         {
           /* size is unknown, and close was not explicitly requested;
              need to either to HTTP 1.1 chunked encoding or
-- 
2.7.4

Activities

Christian Grothoff

2018-08-05 22:09

manager   ~0013174

Ok, interesting. But re-reading RFC 7230 it seems to me that we need to do a bit more about Transfer-Encoding vs. Content-Length: if a client sets 'Transfer-Encoding: gzip' we must also detect this, append ", chunked" *and* then disable setting of Content-Length and instead do the chunking. I think we don't do that either.

Let me know if this is urgent, then I could apply the patch quickly. Otherwise, I'd rather spend the time to address this issue more comprehensively.

samh

2018-08-08 11:13

reporter   ~0013188

It's not urgent for us, I can always add patches to our own packages to fix the specific issue that we're seeing for now. By all means add your own changes to cover the case of gzip as well.

Christian Grothoff

2018-10-05 19:24

manager   ~0013259

I've looked more carefully at the RFC and our code, and decided this:

1) There is an issue where MHD would set the "Transfer-Encoding" to "identity" and "Content-length" as well. I'm changing the code to prevent this.
2) I'm changing (and documenting) MHD_add_response_header() to prevent applications from setting "Content-length" at all. It was never needed, always a bad idea, and can obviously cause all kinds of protocol violations. So the right check is to prevent that, rather than stopping MHD from computing the proper "Transfer-Encoding".
3) I'm also preventing applications from setting a "Transfer-Encoding" that MHD does not support (so only 'identity' and 'chunked' are now allowed).

The change is largely backwards-compatible as we now simply return "MHD_NO" to the application whenever it requests something stupid. Well-written applications should tolerate this change in the return value (fingers crossed...).

Christian Grothoff

2018-10-05 19:24

manager   ~0013260

Git commit is bc8e12c8..0db81a92

Christian Grothoff

2021-09-02 17:54

manager   ~0018180

Fix committed to master branch.

Related Changesets

libmicrohttpd: master 0db81a92

2018-10-05 21:23

Christian Grothoff


Details Diff
fix 0005411 Affected Issues
0005411
mod - ChangeLog Diff File
mod - doc/libmicrohttpd.texi Diff File
mod - src/microhttpd/connection.c Diff File
mod - src/microhttpd/response.c Diff File

Issue History

Date Modified Username Field Change
2018-07-25 13:07 samh New Issue
2018-07-25 13:07 samh File Added: 0001-Don-t-set-transfer-encoding-if-content-length-suppli.patch
2018-08-05 22:09 Christian Grothoff Note Added: 0013174
2018-08-05 22:09 Christian Grothoff Assigned To => Christian Grothoff
2018-08-05 22:09 Christian Grothoff Status new => assigned
2018-08-08 11:13 samh Note Added: 0013188
2018-10-05 19:24 Christian Grothoff Note Added: 0013259
2018-10-05 19:24 Christian Grothoff Status assigned => resolved
2018-10-05 19:24 Christian Grothoff Resolution open => fixed
2018-10-05 19:24 Christian Grothoff Fixed in Version => 0.9.60
2018-10-05 19:24 Christian Grothoff Note Added: 0013260
2018-10-05 19:24 Christian Grothoff Target Version => 0.9.60
2018-11-06 19:46 Christian Grothoff Status resolved => closed
2021-09-02 17:54 Christian Grothoff Changeset attached => libmicrohttpd master 0db81a92
2021-09-02 17:54 Christian Grothoff Note Added: 0018180