Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect handling of Expect: 100-continue during large file uploads #1808

Open
solarispika opened this issue Mar 25, 2024 · 4 comments
Open

Comments

@solarispika
Copy link

Description

When using curl to upload large files (>1M) to a server written with cpp-httplib, curl adds the Expect: 100-continue header. After the server rejects the request by returning a non-100 status code and some data, curl blocks for 1 second after receiving the data and then fails with a broken pipe error.

Steps to Reproduce

  1. Set up a server using cpp-httplib.

  2. Use curl to upload a file larger than 1M to the server.

  3. Observe the --trace output from curl, which shows that it still sends data after handling the 100-continue response.

    == Info: Done waiting for 100-continue
    => Send data, 65536 bytes (0x10000)
    

Expected Behavior

According to RFC 7231 section 5.1.1:

A server that responds with a final status code before reading the
entire message body SHOULD indicate in that response whether it
intends to close the connection or continue reading and discarding
the request message (see Section 6.6 of [RFC7230]).

The expected behavior is for the server to either:

  1. Read and discard the remaining request data, or
  2. Close the connection immediately with a Connection: close header.

Current Behavior

Currently, cpp-httplib does not handle this scenario correctly. It neither reads and discards the remaining request data nor closes the connection immediately. Instead, it leaves the connection as-is and continues to respond with the Keep-Alive header for CPPHTTPLIB_KEEPALIVE_MAX_COUNT times if the client does not close the connection in the request header.

Attempted Solution

The following modification to the code at

default: return write_response(strm, close_connection, req, res);
resolves the issue for curl:

default:
  connection_closed = true;
  return write_response(strm, true, req, res);

With this change, curl exits without the broken pipe error.

Additional Context

@solarispika
Copy link
Author

Update: The latest RFC 9110 section 10.1.1 says A server that responds with a final status code before reading the entire request content SHOULD indicate whether it intends to close the connection (e.g., see Section 9.6 of [HTTP/1.1]) or continue reading the request content.

@yhirose
Copy link
Owner

yhirose commented Mar 26, 2024

@solarispika thanks for the report. Could you send a pull request that you suggests and a unit test in test/test.cc? Thanks!

@solarispika
Copy link
Author

Sure! I'll take some time to see how I can add some proper tests.

@solarispika
Copy link
Author

@yhirose I have difficulty where httplib::Client doesn't implement the Expect: 100-continue behavior, so I can't use it to test the server behavior effectively. It simply sends all the data and encounters a write error.

To comprehensively test the server implementation, the client should also support the required behavior as per the specification. However, I don't have enough time to implement the necessary changes in httplib::Client to handle the Expect: 100-continue header and the associated server responses.

Instead, I propose using an external tool or library that already supports the Expect: 100-continue behavior to test the server implementation. This approach will allow me to focus on the server-side changes while leveraging existing solutions that correctly handle the required client-side behavior.

Please let me know if you have any concerns or suggestions regarding this proposed approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants