We found a bug in the hyper HTTP library

We found a bug in the hyper HTTP library(blog.cloudflare.com)

46 points by Pop_- 4 days ago | 9 comments

edelbitter 1 hour ago |

Cloudflare does not notice (until a customer complains) that they are sending broken responses at scale? I would have thought they would notice this from sampling and linting a few replies.. just in case they did something like Cloudbleed again.

100ms 34 minutes ago |

> The failure was caused by a timing-dependent race condition in hyper’s HTTP/1 connection handling. When the reader was slower and the socket buffer filled, poll_flush returned Poll::Pending, but the dispatch loop discarded that result. Hyper then treated the response as complete and shut down the socket while data remained buffered internally, causing the client to receive an EOF before the full body arrived.

https://github.com/hyperium/hyper/issues/4022

Saved you 3000 words

giammbo 1 hour ago |

The detection story here is what's interesting from an SRE perspective. A 200 OK with truncated data and no error logs is about the hardest class of bug to catch with standard monitoring — your error rate is flat, your latency looks normal, and the only signal is a customer saying "my image is broken."

The race condition aspect makes it worse: it only triggers when the reader is slower than the writer, which in production means it's intermittent and load-dependent. The kind of thing synthetic monitoring almost never catches because your test client is usually fast.

logicchains 1 hour ago |

How does terrible code like this survive so long in such a key piece of infrastructure:

        let _ = self.poll_read(cx)?;
        let _ = self.poll_write(cx)?;
        let _ = self.poll_flush(cx)?;

Surely at the very least a linter should have flagged that the return values aren't handled.

fwlr 1 hour ago | |

    let _ = …?

This is the Rust idiom for “I am intentionally ignoring this return value”. The linter would have caught

    self.poll_read()?;

and in fact one of the options the linter itself suggests in this case is exactly this “let underscore equals” idiom. (Arguably, this code exists because of the linter, not due to its absence!)

In any case, the return value is being “handled” - the question mark examines the result and breaks the loop if the result is not `Ok(…)`, ie if the call is not successful.

Intentionally ignoring the successful return value isn’t necessarily terrible, either - you could be calling the function for its side effect, and you don’t care what the specific result of that effect is, just as long as there is some effect. E.g. maybe you have a state machine, and this is the code that repeatedly drives it.

(Not coincidentally, polling is what you do to Futures, and Futures are state machines that you need to repeatedly drive…)

In conclusion, I do not think this is prima facie terrible code, nor is it an obvious bug. Async rust is subtle and complicated, and not always fully understood by those who nevertheless have to use it.

lifthrasiir 1 hour ago | |

It is an explicit way to discard return values; `self.poll_read(cx)?` etc. alone would warn. Or in this case, `Poll<Result<(), Error>>` is unwrapped once and `Result<(), Error>` is being discarded. The decision to discard `Result<(), Error>` should have been intentional, albeit turned out to be not always the case.

watt 1 hour ago | | |

If they're not going to handle the return values, they should change the function signature to reflect this aspirational contract, that that function "never fails".

I see in the article they did change the poll_flush to run just-in-time at poll_shutdown. So they definitely can make a "best effort" poll_flush version that just does not return any errors for use in that loop.

But all in all? Amateur hour.

QuantumNomad_ 1 hour ago | |

Assigning to _ in Rust specifically means that you intentionally want to discard the value, and the clippy linter and the Rust compiler both know that.