6 ms·
We found a bug in the hyper HTTP library
- edelbitter 3mo agoCloudflare 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.
- ramon156 3mo agoCan you get reasonable results without exposing sensitive info? I'm asking because I genuinely have no idea what it's like at their scale
- deleted 3mo ago[deleted]
- logicchains 3mo ago[flagged]
- deleted 3mo ago[deleted]
- lifthrasiir 3mo agoIt 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 3mo agoIf 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.
- a_cul 3mo agoYou're missing how rust works. The function is explicitly allowed to fail, which is why it returns a Result<(), Error>. They're using the function calls within for their side effects. The ? at the end of each line signals that the function will short-circuit return with an error if the function call fails, and only if it is successful it returns the actual value: they just don't care about this value, hence the let _ =. Basically, they are doing the equivalent of: let _, err = function_call(); if err { return err } ...
- watt 3mo agoWhat I am saying, is make another version of the function, which is explicitly not allowed to fail, if you want to use it in the loop.
- deleted 3mo ago[deleted]
- QuantumNomad_ 3mo agoAssigning to _ in Rust specifically means that you intentionally want to discard the value, and the clippy linter and the Rust compiler both know that.
- fwlr 3mo agolet _ = …? 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.
- logicchains 3mo ago>This is the Rust idiom for “I am intentionally ignoring this return value”. That doesn't make the code any less awful, it just makes idiomatic Rust sound awful. Discarding a return value without even a comment to explain why shouldn't be allowed in any critical project, and the linter should be perfectly capable of ensuring that a comment accompanies the discard and complaining loudly when it doesn't.
- dwroberts 3mo agoThis is missing that it’s a human issue though. If someone is determined to discard an error and not do anything about it, they’ll just put in a dummy comment to appease the linter any way. Force people to handle errors and you end up with the exception fiasco in eg Java where everything ends up being a runtime exception to avoid it
- throw_await 3mo agoAs the top comment states, there is a lint rule, but you have to turn it on.
- giammbo 3mo ago[flagged]
- nopurpose 3mo ago[dead]
- deleted 3mo ago[deleted]
- 100ms 3mo 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 https://github.com/hyperium/hyper/issues/4022 Saved you 3000 words
- michalc 3mo agoReminds me of another “slow client”-related bug in gunicorn: https://github.com/benoitc/gunicorn/issues/3334 https://github.com/benoitc/gunicorn/issues/3334
- microgpt 3mo agoThat's not even a bug. That's how TCP works. If you keep sending data to a socket the other side has closed, you get RST.
- edelbitter 3mo agoIn case of plain HTTP over TCP, there is even a hint in the spec about why and how a server might want to avoid fully closing prematurely. https://datatracker.ietf.org/doc/html/rfc9112#section-9.6 https://datatracker.ietf.org/doc/html/rfc9112#section-9.6 (this was already in https://datatracker.ietf.org/doc/html/rfc7230#section-6.6 https://datatracker.ietf.org/doc/html/rfc7230#section-6.6)
- microgpt 3mo agoThis is relevant if the client sends multiple requests but the server decides to close the connection after one of them. The server should discard the additional requests until the client signals no more requests are coming.
- NooneAtAll3 3mo ago
- algoth1 3mo agoI wonder if this bug was found via project glasswing
- re-thc 3mo ago> I wonder if this bug was found via project glasswing Did you read how they said it took weeks? Would run out of tokens at that rate...
- worldsavior 3mo ago> We spent six weeks chasing a nearly invisible bug — a race condition that occurred only under specific conditions — in the hyper library that impacted how the Images binding returned processed image data back to the client. In the end, it took four lines of code to fix it. That's a long time, must be frustrating.
- gmueckl 3mo agoIt is a long time and it gets frustrating when there is significant time where there is flailing with no visible progress. I have had long bug hunts (~a month each) and witnessed ones that took much, much longer. But the longest one I witnessed was drawn out because reproduction was initially unreliable and could take weeks to months. Thankfully, reproduction was by letting a box sit in a corner while tje people involved moved on to other tasks. This kept everybody sane.
- Atotalnoob 3mo agoSometimes the best option is to enhance observability and see when it happens again
- microgpt 3mo agoWould using Rust have prevented this?
- Cthulhu_ 3mo agoThe Hyper library in question is a Rust library. Did you read the article, or are you a "use rust" parrot / bot based on titles?
- re-thc 3mo agoIsn't this already Rust?
- pjmlp 3mo agoThat was obviously a joke question, pointing that Rust isn't the solution for everything.
- lelanthran 3mo agoWoosh :-)
- geodel 3mo agoAgree. This is warning to people who thought Rust is optional at cloud scale.
- Ygg2 3mo agoNo. Anyone expecting that hasn't read No Silver Bullet essay.
- Thaxll 3mo agoSo much for Rust forcing you to handle errors.
- wongarsu 3mo agoYou could argue the bug happened exactly because hyper's poll_flush treats flushing some but not all data as a successful return, not an error case.
- atoav 3mo agoYou could say the exact same thing about safety belts and airbags in cars after someone has died in a crash. Why even bother with measures that prevent many problems if they won't prevent all of them, right?
- chlorion 3mo agoThis is the argument I like too. It's the same argument anti-vaxers love to make. "Well you can still get covid after getting the shot", which is something I read and heard quite a lot. That doesn't make the thing useless. Humans are really dumb.
- atoav 3mo agoThe older I get the more I realise that I have taken way to much of my mathematical intuition for granted. This I'd an example of people not grasping simple probabilities. Forced to play Russian Roulette they prefer the revolver with 5 bullets in its chambers or the one with just a single bullet, because in both cases people die. Other concepts many people do not grasp are feedback-loops and exponentials. And by not grasp I mean: You explain it to them, the nod along and when faced with the thing in slightly different clothes they will actively deny it'd existence.
- Matl 3mo agoGo does force you too, but it also supports _ as a bypass - because sometimes you do know better. Just not in this case. Rust never promised it'll let programmers turn off their brain, that's what LLMs are for.
- Twey 3mo agoThis would have been flagged by Clippy lints `let_underscore_untyped` or `let_underscore_must_use`, which sadly are not enabled by default.
- pwdisswordfishq 3mo agoEhh, easy fix #[allow(clippy::let_underscore_untyped,clippy::let_underscore_must_use)] let _ = self.poll_flush(cx)?;
- nesarkvechnep 3mo agoYeah, but you must know about them and the possible bug first in order to allow them...
- Joker_vD 3mo agoAt which point you wouldn't have written this bug in the first place; or the warnings would trigger immediately, you'd change _ to an actual variable and then remove the warning pragmas because now you don't assign to _.
- Twey 3mo ago`Poll` is marked `#[must_use]` so if you were assigning to something other than `_` you'd get a warning that you're ignoring the `Pending` path. The Clippy lint is only for `_` which Rust considers a use by default.
- Twey 3mo agoHence ‘sadly’. IMNSHO both of these (or at least _untyped) should be enabled by default. Untyped `let _` is too big a footgun during refactorings.
- turboponyy 3mo agoNot really. If I'm using a linter, I go and configure the strictest possible ruleset, and only disable rules when justified on a need-by-need basis. It's just a matter of discipline.
- pseudony 3mo agoSo “fearless concurrency” still only happens when one just decides to not be afraid… :)
- c0balt 3mo agoThis does not appear to be a concurrency bug though?
- pseudony 3mo ago“ a race condition that occurred only under specific conditions — in the hyper library”
- conradludgate 3mo agoFearless concurrency has always been regarding data races. There's no fear of undefined behaviour due to a race condition. Rust has never promised to solve race conditions as a whole.
- microgpt 3mo agoOf course it's a concurrency bug. It races sending data to the kernel against the kernel sending data to the network. If the wrong one wins the bug occurs.
- inexcf 3mo agoIsn't that like saying there can never be a language with safe concurrency since the code could interact with C code that segfaults? I dunno this kinda reminds me of the 10/10 Rust CVE that turned out to be cmd.exe on Windows not sanitizing inputs and languages like Java just labeled it "won't fix".
- microgpt 3mo agoYou mean the one where Windows doesn't have argv the way Unix does, and instead just has a single string that is interpreted slightly differently by each executable? That is a language making false assertions about how the underlying platform works, causing an impedance mismatch that is impossible to fix.
- nopurpose 3mo agoNice writeup, but I don't understand how `curl` didn't trigger bug for them (or any other hyper HTTP server out there), given the explanation in the article. `curl --http1.1` sends `Connection: Close` so sender (hyper) must attempt to shutdown connection after sending whole body. Surely any network is slower than memory copy into socket kernel buffers, so it must reliably trigger condition "buffer flush can't be done in one go" and thus trigger early TCP shutdown.
- deleted 3mo ago[deleted]
- xacky 3mo agoYet Cloudflare relies on bugs in browsers to "verify" you.