2018-10-17 05:12 CEST

View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0005411libmicrohttpdcompliancepublic2018-10-05 19:24
Reportersamh 
Assigned ToChristian Grothoff 
PrioritynormalSeverityminorReproducibilityalways
StatusresolvedResolutionfixed 
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
  • patch file icon 0001-Don-t-set-transfer-encoding-if-content-length-suppli.patch (2,155 bytes) 2018-07-25 13:07 -
    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
    
    

-Relationships Relation Graph ] Dependency Graph ]
+Relationships

-Notes

~0013174

Christian Grothoff (manager)

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.

~0013188

samh (reporter)

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.

~0013259

Christian Grothoff (manager)

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...).

~0013260

Christian Grothoff (manager)

Git commit is bc8e12c8..0db81a92
+Notes

-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
+Issue History