search

http-rs / http-types

Common types for HTTP operations
https://docs.rs/http-types
Apache License 2.0
200 stars 84 forks source link

Use `ascii` crate for string values inside `HeaderValue`, `HeaderName` types #313

Open yoshuawuyts opened 3 years ago

yoshuawuyts commented 3 years ago

We could remove almost every instance of unsafe in our codebase by replacing the use of String in HeaderName and HeaderValue with ascii::AsciiString instead. Implementing this shouldn't be too hard, but code-wise it's a fair few lines that need changing.

This wouldn't change anything in our API signatures either, assuming we don't have any bugs in how we handle ASCII today. @brightly-salty would you be interested in perhaps trying to tackle this?

brightly-salty commented 3 years ago

Sure!

brightly-salty commented 3 years ago

Would you like me to use from_ascii(..).unwrap() or unsafe { from_ascii_unchecked(..)} It seems like both would work equally well but the second preserves unsafe.

yoshuawuyts commented 3 years ago

Oh, good question. Yeah, I actually care less about the perf here than I care about removing unsafe. I think the former is probably desirable.

jbr commented 3 years ago

If we panic on improper ascii, doesn't that mean a http client could shut down the server?

brightly-salty commented 3 years ago

We are already calling ensure! before the unsafe code. I'm not aware of the specifics of the implementation of ensure but I think it will panic in some form. The unwrap panicking should be impossible because we are already ensuring it is ASCII beforehand.

brightly-salty commented 3 years ago

Should the behavior of HeaderName::from_lowecase_str be to panic, be unsafe, or to return a result? Currently it assumes the given str is valid ASCII and is lowercase.

brightly-salty commented 3 years ago

So the question for HeaderName::from_lowecase_str is how to make it work with existing usages. We can use an unsafe {AsciiStr::from_ascii_unchecked(s)} but this function is not const which we need for HeaderName::from_lowercase_str to be const, which we need to be able to create consts in src/headers/constants.rs.

The only solution I can think of is to use lazy_static for the constants, thus allowing the use of non-const functions in their initialization, thus allowing us to use unsafe {AsciiStr::from_ascii_unchecked(s)} in HeaderName::from_lowercase_str.

@jbr @yoshuawuyts

brightly-salty commented 3 years ago

@yoshuawuyts I'm going to "unassign" myself from this one, because it requires multiple high-level design decisions.