Failure to flush Payload when it is dropped before handler consumes all bytes

[squidpickles]

In cases where a handler exits early without reading a supplied Payload, there is some new code in actix-web (see PR #2624) that consumes the rest of the bytes in the request.

It seems not to work with large request bodies (approaching 512k).

Expected Behavior

If a Payload is dropped before all bytes are consumed, the rest of the client request should still be consumed.

Current Behavior

If a Payload is dropped before all bytes are consumed, the rest of the request is consumed if the request is small, but larger requests stop reading and drop the connection with a message "handler did not read whole payload and dispatcher could not drain read buf; return 500 and close connection".

Possible Solution

Haven't quite found the root cause yet, but it appears that the read_buf in the Dispatcher is empty. When PayloadDecoder::decode() is called with read_buf as src, it reports zero bytes available, but self.kind contains a nonzero Kind::Length of remaining bytes. If they were both correctly zero, it would return PayloadItem::Eof, and the flush would terminate correctly. If they were both correctly nonzero, it would successfully return PayloadItem::Chunk(buf), which would continue the flush loop until exhausted.

I'm not sure which one is wrong here, but it only seems to happen if there are a certain number of reads required to flush the request.

Steps to Reproduce (for bugs)

Minimal server:

use actix_web::{post, web::Payload, App, HttpServer};

#[post("/hello")]
async fn greet(_: Payload) -> &'static str {
    "Ok\n"
}

#[actix_web::main]
async fn main() -> std::io::Result<()> {
    env_logger::init();
    HttpServer::new(|| App::new().service(greet))
        .bind(("127.0.0.1", 5000))?
        .run()
        .await
}

Client-side - I was able to duplicate this by sending a 511k file (succeeded), and a 512k file (failed), but managed to narrow down to exact number of bytes by using curl's -C to skip initial bytes in the file. Byte ranges may vary across OSs, not sure.

# create a 512k file of random data
dd if=/dev/random of=512k bs=1024 count=512
# this succeeds - continue sending data from byte 1753
curl -C 1753 -H 'Content-Type: application/octet-stream' -d @512k -v http://127.0.0.1:5000/hello
# this fails - continue sending data from byte 1752
curl -C 1752 -H 'Content-Type: application/octet-stream' -d @512k -v http://127.0.0.1:5000/hello

Context

I'd like to be able to return from a handler without having to worry about whether I've finished reading the Payload completely. For example, streaming a file to disk, then having a write fail, it's much easier to handle with a simple ? and a handler returning a Result, than having to wrap the whole thing in a handler that cleans up the Payload before returning an error.

Your Environment

• MacOS 12.4 x86_64
• actix-web 4.0.1
• actix-http 3.0.0
• rustc 1.61.0 (fe5b13d68 2022-05-18)

[squidpickles]

Looking closer, I'm seeing

actix-web/actix-http/src/h1/dispatcher.rs

Lines 703 to 709 in 6a5b370

	if let Some(sender) = &this.payload {
	// ...maybe handler does not want to read any more payload...
	if let PayloadStatus::Dropped = sender.need_read(cx) {
	debug!("handler dropped payload early; attempt to clean connection");
	// ...in which case poll request payload a few times
	loop {
	match this.codec.decode(this.read_buf)? {

which talks about polling the request payload. But nothing in this loop actually fills the read buffer. So, if there are unread bytes in the request according to the codec, but the read buffer is empty, it triggers the weird None return from the decode().

I'm not sure of the best way to fill the read buffer in this drain loop, but that is looking like the root cause.

[squidpickles]

Ok, I have something that seems to solve the problem, but it's not quite usable as a PR yet --

In the bit of code above, I add a check for empty this.read_buf. At that point, calling read_available() would fill the buffer and allow the cleaning to continue, but I can't find a way to call that function due to it taking a Pin<&mut Self>.

loop {
  if this.read_buf.is_empty() {
    log::trace!("read buffer is empty; reading available bytes");
    self.read_available(cx)?; // fails borrow checker
  }
  // ...
}

I ended up copying the entire contents of read_available() into this block, for testing. That seemed to work great.

Anyone know how I can call this function from inside poll_request(), which already has a Pin<&mut Self>?