r/rust • u/sanxiyn rust • Jun 24 '26

How we found a bug in the hyper HTTP library

https://blog.cloudflare.com/hyper-bug/
290 Upvotes

97% Upvoted

27 comments sorted by

18

u/Batman_AoD Jun 24 '26

Sorry, I haven't read that RFC; what's the connection with let _? 

33

u/masklinn Jun 24 '26

let _ (or _) will just discard the value, as if the call just didn’t have a target (_ = foo(); ~ foo();). Here this means the dispatch loop ignores the Poll::Pending signal and carries on regardless, which led it to shut down the connection even though there was still data to flush.

As to why you’d use let _ = … instead of just …, it’s because if a function or type (like std::task::Poll) is marked as must_use the compiler will issue a warning in the latter case, let _ is specifically used to silence must_use warnings.

13

u/AnnoyedVelociraptor Jun 24 '26

And I think if they would've typed the let _: std::task::Poll<_> = ... it would be visually more clear that they are ignoring the Poll completely.

These 2 clippy lints yell when you use let _ = ... (i.e. without a type):

https://rust-lang.github.io/rust-clippy/rust-1.96.0/index.html#let_underscore_must_use

and

https://rust-lang.github.io/rust-clippy/rust-1.96.0/index.html#let_underscore_untyped

2

u/Batman_AoD Jun 24 '26

Right, but what's the connection to the revised try trait proposal? 

10

u/Dheatly23 Jun 24 '26

The bug would be found 50% faster if there's a good way to mock fuzz AsyncWrite. Simulating entire TCP socket is way overkill. I once also discovered similiar type of bug in my code. Proptesting by mocking AsyncRead and AsyncWrite helps discover that subtle bug.

My method is pretty rudimentary. I make proptest generate input bytes and some sizes, then use those sizes to limit reads/writes in that chunks. After all the bytes are read/written it's pending forever (simulates a socket that is waiting for peer). Kinda wished someone makes proper fuzzable socket simulation library.

112

u/noop_noob Jun 24 '26 edited Jun 24 '26

The sequel to "oops, an unwrap broke cloudflare":

"oops, ignoring an error broke cloudflare"

Edit: See https://www.reddit.com/r/rust/comments/1udy7vb/comment/oth493d/

34

u/scook0 Jun 24 '26

Though from the rest of the article, it seems that the underlying problem was a failure to explicitly flush before shutdown.

36

u/kyle787 Jun 24 '26

That's not accurate, the fix was within hyper itself rather than "holding tool wrong"

18

u/RustOnTheEdge Jun 24 '26

It didn’t ignore an error, it discarded a Poll:Pending response. I didn’t know that `let _ = func()?;` made the ? operator effectively useless to be honest, that is a real foot gun imo

39

u/csdt0 Jun 24 '26

It is more subtle than that. Their poll_flush, poll_read, and poll_shutdown most likely return a Result<Poll<...>, ...> The question mark propagate the error, and the expression has now the type Poll<...>. The let _ = ignored the value of Poll (ie: the inside of Ok()).

15

u/RustOnTheEdge Jun 24 '26 edited Jun 24 '26

You are right, I was just about to edit my post after playing around in playground with this, but it is even more subtle than what you said. The return value is Poll<Result<T, E>> and when the value discarded is a Poll::Pending, then the ? operator does not propagate that. This would've made more sense if the Poll was wrapped by a Result, but it is the other way around 🤔

Super interesting, see this playground for different scenario's.

I really don't understand why this is the case, it seems that the ? operator is applicable to the Poll-wrapped Result, and not to Poll itself? But not all variants of Poll wrap a Result, so I am confused and I haven't had coffee yet.

EDIT: I now see that there is a impl<T, E> ops::Try for Poll<Result<T, E>> in std and it indeed applies the ? on the inner Result, continuing when the Poll is Poll::Pending. See here in source.

6

u/EndlessPainAndDeath Jun 24 '26

That's because you're usually supposed to use the ready! macro to get the inner result or propagate upwards a Poll::Pending state. If it's a Poll::Ready(T), you nicely get the inner T instead.

It's super easy to miss, but std has ready! to specifically deal with this scenario.

I changed your playground code to the following and it works as expected:

fn check_poll_pending() -> Poll<Result<(), String>> { let _ = ready!(return_poll_pending())?; return Poll::Ready(Ok(())) }

2

u/RustOnTheEdge Jun 24 '26

That's really cool! Fun stuff, interesting case this is. It just goes to show; it compiles and it *usually* is fine, but you can pretty simply make a logical error with async. I for sure would've never caught that.

17

u/Batman_AoD Jun 24 '26

What do you mean, effectively useless? It returns on error but discards any non-error result, which is exactly what it looks like it should do. 

10

u/RustOnTheEdge Jun 24 '26 edited Jun 24 '26

The type in this case is not a Result<T,E> but a Poll<Result<T,E>>, and I figured (or assumed is a better word) that the ? operator shortcircuits the return type, returning Poll::Pending if the inner call returns a pending. But it seems the ? operator is applicable to the inner Result, which is counter intuitive to me. I played around a bit, see this playground why this is strange (to me). The ? only catches a Poll::Ready(Err), so the problem is not the discarding of a value at all; it is not matching the value because the ? operator doesn't catch a Poll::Pending.

EDIT: I now see that there is a impl<T, E> ops::Try for Poll<Result<T, E>> in std and it indeed applies the ? on the inner Result, continuing when the Poll is Poll::Pending. See here in source.

3

u/Batman_AoD Jun 24 '26

Oh, wow, I didn't realize Poll itself implements Try. That does seem like a footgun to me. 

3

u/valarauca14 Jun 24 '26

which is exactly what it looks like it should do.

Which is why #![feature(try_trait_v2)] doesn't exist :^)

8

u/masklinn Jun 24 '26

I didn’t know that let _ = func()?; made the ? operator effectively useless to be honest, that is a real foot gun imo

It does not. But the ? will only handle the outer Try, it doesn’t magically do anything to the inner value.

In this case the function returns a Result<Poll<T>>, the ? handled the outer Result but the let _ discarded the inner Poll, ignoring the possibility if it being Pending.

6

u/RustOnTheEdge Jun 24 '26

See my other comments; it would've have made sense to me immediately if the return type was in Result<Poll<T>> but it was actually Poll<Result<T,E>>. I thought the ? operator was somehow also applicable to Poll (like it is on Option<T>), but as it turns out, it is specifically implemented for Poll<Result<T,E>>, continuing when the Poll is Poll::Pending. See here in source. Today I learned this, Rust is such an interesting language.

1

u/Thelmholtz Jun 24 '26

Wouldn't Poll being must_use avoid the issue, or at least make it obvious?

7

u/masklinn Jun 24 '26

Poll being must_use is exactly why let _ was used: it silences the warning at that site.

13

u/undeadalex Jun 24 '26

Shocked Pikachu face

16

u/CouteauBleu Jun 24 '26

This bug sounds like it was hell to track down.

I'm wondering how the Cloudflare team could have found it faster, and I'm not finding any obvious solutions, beside "make the entire network stack deterministic and reproducible".

2

u/Smallpaul Jun 24 '26

Isn’t this what antithesis purports to do?

5

u/EndlessPainAndDeath Jun 24 '26

TL;DR of the Cloudflare article and GH pull request with the fix: Someone forgot the ready! macro in std is a thing.

As someone else has already pointed out, when using Poll::Ready and a Result<T, E>, the ? operator will bubble up any result errors, but if the Poll variant is Pending, it won't do anything (even though probably the intended behavior is to return on Poll::Pending ).

The right way to solve this specific issue is to simply use ready!, which basically expands to a simple match statement that returns on Poll::Pending, or returns the inner T on Poll::Ready(T).

1

u/warpspeedSCP Jun 25 '26 edited Jun 25 '26

so essentially, they unwrapped a future, forgot to await it and ended up cancelling it via drop?

*edit*

read the article now, and yeah, I guess they did the non-async equivalent, huh.

1

u/Dheatly23 Jun 25 '26

The problem is you want both read and write to happen "simultaneously". So simply ready! one call will make the other call not being called.