hyperium
try_ methods to handle capacity overflow
This PR keeps old methods for backward compatibility, but deprecates them
PR in hyper - https://github.com/hyperium/hyper/pull/3270
Closes #603
Thanks for the PR! I wouldn't deprecate the methods, just like the methods on Vec aren't deprecated.
@seanmonstar The problem is that it's pretty easy to trigger panic through external HTTP call. The max value is very small, so in my opinion, code should be considered vulnerable, and it's just unsafe to use non-failable methods. When max value is 64 or 32 bits, it becomes too hard to expose it through the network. At least is should be marked somehow that non-try methods are just unsafe to use.
I'd vote for deprecation as well, because HeaderMap can be considered as quite specific wrapper with clear purpose, so if it can be fully safe API, why not?
It has its own rules regarding max capacity, if user wants to propagate the panic instead of handling - unwrap/expect is still an option and it makes the code clear that the implementation may panic in some another conditions than Vec/HashMap/etc
I appreciate the reasoning, but I still feel it should not be deprecated.
- It is a container and should have similar methods to
VecandHashMap. There are those that argue that every single allocation method on those containers should have been fallible, sure. However, there's decent value in being consistent with what libstd does. - It's not at all common for anything in HTTP to reach the max internal limit here. Requests and responses don't have 1000s of headers.
- Other limits can exist, such as in hyper's HTTP/1 handling, it limits parsing to 100 headers. The flaw would be that
h2doesn't include a limit. - They aren't unsafe, since unsafe has a very specific meaning in Rust, which is that it could cause memory unsafety. These methods cannot.
What I mentioned in the original issue would be fair: add a section # Panics to the method documentation, so anyone looking can decide if that's something they care about.
The main difference with std containers is that their limitations are only memory allocator and the amount of memory managed by OS. This leads to an inability to detect if allocation fails at the moment. With modern 64-bit systems and typically enough memory, we don't need to think much about allocation failures. So standard containers won't panic, they may lead to OOM termination.
The behavior of HeadersMap is different; it has small limit and panics, which may lead to an inconsistent state or termination without clear reason.
In our case, this issue was discovered by external testing.
Considering that there are already a lot of differences in the behavior of standard containers, I would definitely prefer never to have a chance to make the mistake of using non-try methods, because intuition would make me feel like HeaderMap would not introduce additional limitations.
It's not at all common for anything in HTTP to reach the max internal limit here. Requests and responses don't have 1000s of headers.
Yes, if it's not an attack. I don't think optimizing for happy-path scenarios is a good idea for such a fundamental library
Other limits can exist, such as in hyper's HTTP/1 handling, it limits parsing to 100 headers. The flaw would be that h2 doesn't include a limit.
it's good that hyper has this limit for HTTP/1, but I think http may also be used outside of hyper.
They aren't unsafe, since unsafe has a very specific meaning in Rust, which is that it could cause memory unsafety. These methods cannot.
For sure, I didn't mean rust unsafe functions. Panics section with information about limits may work, but I don't understand what are the benefits of keeping these function, if they do the code more fragile.
I do appreciate the desire to make things "safer" for more people. While I still don't agree with the arguments, how about this: you could separate the contribution into two steps. There's no disagreement about adding the try_reserve methods (well, maybe we should agree on exactly which ones to add). That can be merged quickly.
The second part, deprecating the other methods, can be separated into an issue, to have the discussion.
That way, the first part is available to people more quickly. What do you think?
Ok. I'll do it.
Please check the issue from chrono, where a similar panicking design was used - https://github.com/chronotope/chrono/issues/815. The author did a good job describing why using panicking operations is not a good option. Let's continue the discussion in a separate PR.
maybe we should agree on exactly which ones to add
Actually, there are not many options. I started with a few functions with assertions on MAX_SIZE. All try_ functions created were inherited from the demand to handle these few function's results.
I'm sorry I let this slip. I've resolved the merge conflicts, and also a type change that was needed since 1.0 already shipped, and put it in #682.