Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return an error rather than panicking when HeaderName is too long #433

Merged
merged 1 commit into from
Aug 3, 2020

Conversation

acfoltzer
Copy link
Member

Fixes #432.

This eliminates an undocumented panic from the HeaderName creation functions, returning instead an InvalidHeaderName when the name length exceeds MAX_HEADER_NAME_LEN.

I considered making InvalidHeaderName a richer error type, but since it was already used for another type of length error (rejecting zero-length names) I figured it was okay to reuse.

I also redefined MAX_HEADER_NAME_LEN slightly, so that it is equal to the largest allowed header length, rather than one past that value. This was motivated by discovering a bug in my comparison logic when I went to write the new test exercising the wrong-length error conditions.

Fixes hyperium#432.

This eliminates an undocumented panic from the `HeaderName` creation functions, returning instead an
`InvalidHeaderName` when the name length exceeds `MAX_HEADER_NAME_LEN`.

I considered making `InvalidHeaderName` a richer error type, but since it was already used for
another type of length error (rejecting zero-length names) I figured it was okay to reuse.

I also redefined `MAX_HEADER_NAME_LEN` slightly, so that it is equal to the largest allowed header
length, rather than one past that value. This was motivated by discovering a bug in my comparison
logic when I went to write the new test exercising the wrong-length error conditions.
Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me, thanks!

@seanmonstar seanmonstar merged commit b00473c into hyperium:master Aug 3, 2020
@acfoltzer acfoltzer deleted the header-name-len-error branch August 4, 2020 17:54
@dekellum dekellum mentioned this pull request Dec 14, 2020
BenxiangGe pushed a commit to BenxiangGe/http that referenced this pull request Jul 26, 2021
…perium#433)

Fixes hyperium#432.

This eliminates an undocumented panic from the `HeaderName` creation functions, returning instead an
`InvalidHeaderName` when the name length exceeds `MAX_HEADER_NAME_LEN`.

I considered making `InvalidHeaderName` a richer error type, but since it was already used for
another type of length error (rejecting zero-length names) I figured it was okay to reuse.

I also redefined `MAX_HEADER_NAME_LEN` slightly, so that it is equal to the largest allowed header
length, rather than one past that value. This was motivated by discovering a bug in my comparison
logic when I went to write the new test exercising the wrong-length error conditions.
jeddenlea added a commit to jeddenlea/http that referenced this pull request Aug 20, 2021
... plus some clean-up.

It was only after I came up with the `const fn
StandardHeader::from_bytes() -> Option<StandardHeader>` scheme that I
noticed the debug+wasm32-wasi version of parse_hdr which had something
very similar.

While cleaning up that function, I realized it still would still panic
if an attempted name was too long, which had been fixed for all other
targets and profiles in hyperium#433.  Then, I thought it would be worth seeing
if the use of `eq!` in the primary version of `parse_hdr` still made any
difference.

And, it would not appear so. At least not on x86_64, nor wasm32-wasi run
via wasmtime  I've run the benchmarks a number of times now, and it
seems the only significant performance change anywhere is actually that
of `from_static` itself, which now seems to run in about 2/3 the time on
average.

Unfortunately, `const fn` still cannot `panic!`, but I've followed the
lead from `HeaderValue::from_static`.  While that version required 1.46,
this new function requires 1.49.  That is 8.5 months old, so hopefully
this isn't too controversial!
jeddenlea added a commit to jeddenlea/http that referenced this pull request Aug 20, 2021
... plus some clean-up.

It was only after I came up with the `const fn
StandardHeader::from_bytes() -> Option<StandardHeader>` scheme that I
noticed the debug+wasm32-wasi version of parse_hdr which had something
very similar.

While cleaning up that function, I realized it still would still panic
if an attempted name was too long, which had been fixed for all other
targets and profiles in hyperium#433.  Then, I thought it would be worth seeing
if the use of `eq!` in the primary version of `parse_hdr` still made any
difference.

And, it would not appear so. At least not on x86_64, nor wasm32-wasi run
via wasmtime  I've run the benchmarks a number of times now, and it
seems the only significant performance change anywhere is actually that
of `from_static` itself, which now seems to run in about 2/3 the time on
average.

Unfortunately, `const fn` still cannot `panic!`, but I've followed the
lead from `HeaderValue::from_static`.  While that version required 1.46,
this new function requires 1.49.  That is almost 8 months old, so
hopefully this isn't too controversial!
jeddenlea added a commit to jeddenlea/http that referenced this pull request Aug 20, 2021
... plus some clean-up.

It was only after I came up with the `const fn
StandardHeader::from_bytes() -> Option<StandardHeader>` scheme that I
noticed the debug+wasm32-wasi version of parse_hdr which had something
very similar.

While cleaning up that function, I realized it still would still panic
if an attempted name was too long, which had been fixed for all other
targets and profiles in hyperium#433.  Then, I thought it would be worth seeing
if the use of `eq!` in the primary version of `parse_hdr` still made any
difference.

And, it would not appear so. At least not on x86_64, nor wasm32-wasi run
via wasmtime  I've run the benchmarks a number of times now, and it
seems the only significant performance change anywhere is actually that
of `from_static` itself, which now seems to run in about 2/3 the time on
average.

Unfortunately, `const fn` still cannot `panic!`, but I've followed the
lead from `HeaderValue::from_static`.  While that version required 1.46,
this new function requires 1.49.  That is almost 8 months old, so
hopefully this isn't too controversial!
jeddenlea added a commit to jeddenlea/http that referenced this pull request Aug 20, 2021
... plus some clean-up.

It was only after I came up with the `const fn
StandardHeader::from_bytes() -> Option<StandardHeader>` scheme that I
noticed the debug+wasm32-wasi version of parse_hdr which had something
very similar.

While cleaning up that function, I realized it still would still panic
if an attempted name was too long, which had been fixed for all other
targets and profiles in hyperium#433.  Then, I thought it would be worth seeing
if the use of `eq!` in the primary version of `parse_hdr` still made any
difference.

And, it would not appear so. At least not on x86_64, nor wasm32-wasi run
via wasmtime  I've run the benchmarks a number of times now, and it
seems the only significant performance change anywhere is actually that
of `from_static` itself, which now seems to run in about 2/3 the time on
average.

Unfortunately, `const fn` still cannot `panic!`, but I've followed the
lead from `HeaderValue::from_static`.  While that version required 1.46,
this new function requires 1.49.  That is almost 8 months old, so
hopefully this isn't too controversial!
jeddenlea added a commit to jeddenlea/http that referenced this pull request Aug 20, 2021
... plus some clean-up.

It was only after I came up with the `const fn
StandardHeader::from_bytes() -> Option<StandardHeader>` scheme that I
noticed the debug+wasm32-wasi version of parse_hdr which had something
very similar.

While cleaning up that function, I realized it still would still panic
if an attempted name was too long, which had been fixed for all other
targets and profiles in hyperium#433.  Then, I thought it would be worth seeing
if the use of `eq!` in the primary version of `parse_hdr` still made any
difference.

And, it would not appear so. At least not on x86_64, nor wasm32-wasi run
via wasmtime  I've run the benchmarks a number of times now, and it
seems the only significant performance change anywhere is actually that
of `from_static` itself, which now seems to run in about 2/3 the time on
average.

Unfortunately, `const fn` still cannot `panic!`, but I've followed the
lead from `HeaderValue::from_static`.  While that version required 1.46,
this new function requires 1.49.  That is almost 8 months old, so
hopefully this isn't too controversial!
jeddenlea added a commit to jeddenlea/http that referenced this pull request Aug 21, 2021
... plus some clean-up.

It was only after I came up with the scheme using
`const fn from_bytes(&[u8]) -> Option<StandardHeader>`
that I noticed the debug+wasm32-wasi version of parse_hdr, which had
something very similar.

While cleaning up that function, I realized it still would still panic
if an attempted name was too long, which had been fixed for all other
targets and profiles in hyperium#433.  Then, I thought it would be worth seeing
if the use of `eq!` in the primary version of `parse_hdr` still made any
difference.

And, it would not appear so. At least not on x86_64, nor wasm32-wasi run
via wasmtime.  I've run the benchmarks a number of times now, and it
seems the only significant performance change anywhere is actually that
of `HeaderName::from_static` itself, which now seems to run in about 2/3
the time on average.

Unfortunately, `const fn` still cannot `panic!`, but I've followed the
lead from `HeaderValue::from_static`.  While that version required 1.46,
this new function requires 1.49.  That is almost 8 months old, so
hopefully this isn't too controversial!
jeddenlea added a commit to jeddenlea/http that referenced this pull request Aug 24, 2021
... plus some clean-up.

It was only after I came up with the scheme using
`const fn from_bytes(&[u8]) -> Option<StandardHeader>`
that I noticed the debug+wasm32-wasi version of `parse_hdr`, which had
something very similar.

While cleaning up that function, I realized it still would still panic
if an attempted name was too long, which had been fixed for all other
targets and profiles in hyperium#433.  Then, I thought it would be worth seeing
if the use of `eq!` in the primary version of `parse_hdr` still made any
difference.

And, it would not appear so. At least not on x86_64, nor wasm32-wasi run
via wasmtime.  I've run the benchmarks a number of times now, and it
seems the only significant performance change anywhere is actually that
of `HeaderName::from_static` itself, which now seems to run in about 2/3
the time on average.

Unfortunately, `const fn` still cannot `panic!`, but I've followed the
lead from `HeaderValue::from_static`.  While that version required 1.46,
this new function requires 1.49.  That is almost 8 months old, so
hopefully this isn't too controversial!
jeddenlea added a commit to jeddenlea/http that referenced this pull request Dec 17, 2021
... plus some clean-up.

It was only after I came up with the scheme using
`const fn from_bytes(&[u8]) -> Option<StandardHeader>`
that I noticed the debug+wasm32-wasi version of `parse_hdr`, which had
something very similar.

While cleaning up that function, I realized it still would still panic
if an attempted name was too long, which had been fixed for all other
targets and profiles in hyperium#433.  Then, I thought it would be worth seeing
if the use of `eq!` in the primary version of `parse_hdr` still made any
difference.

And, it would not appear so. At least not on x86_64, nor wasm32-wasi run
via wasmtime.  I've run the benchmarks a number of times now, and it
seems the only significant performance change anywhere is actually that
of `HeaderName::from_static` itself, which now seems to run in about 2/3
the time on average.

Unfortunately, `const fn` still cannot `panic!`, but I've followed the
lead from `HeaderValue::from_static`.  While that version required 1.46,
this new function requires 1.49.  That is almost 8 months old, so
hopefully this isn't too controversial!
jeddenlea added a commit to jeddenlea/http that referenced this pull request Feb 9, 2022
... plus some clean-up.

It was only after I came up with the scheme using
`const fn from_bytes(&[u8]) -> Option<StandardHeader>`
that I noticed the debug+wasm32-wasi version of `parse_hdr`, which had
something very similar.

While cleaning up that function, I realized it still would still panic
if an attempted name was too long, which had been fixed for all other
targets and profiles in hyperium#433.  Then, I thought it would be worth seeing
if the use of `eq!` in the primary version of `parse_hdr` still made any
difference.

And, it would not appear so. At least not on x86_64, nor wasm32-wasi run
via wasmtime.  I've run the benchmarks a number of times now, and it
seems the only significant performance change anywhere is actually that
of `HeaderName::from_static` itself, which now seems to run in about 2/3
the time on average.

Unfortunately, `const fn` still cannot `panic!`, but I've followed the
lead from `HeaderValue::from_static`.  While that version required 1.46,
this new function requires 1.49.  That is almost 8 months old, so
hopefully this isn't too controversial!
jeddenlea added a commit to jeddenlea/http that referenced this pull request Apr 6, 2022
... plus some clean-up.

It was only after I came up with the scheme using
`const fn from_bytes(&[u8]) -> Option<StandardHeader>`
that I noticed the debug+wasm32-wasi version of `parse_hdr`, which had
something very similar.

While cleaning up that function, I realized it still would still panic
if an attempted name was too long, which had been fixed for all other
targets and profiles in hyperium#433.  Then, I thought it would be worth seeing
if the use of `eq!` in the primary version of `parse_hdr` still made any
difference.

And, it would not appear so. At least not on x86_64, nor wasm32-wasi run
via wasmtime.  I've run the benchmarks a number of times now, and it
seems the only significant performance change anywhere is actually that
of `HeaderName::from_static` itself, which now seems to run in about 2/3
the time on average.

Unfortunately, `const fn` still cannot `panic!`, but I've followed the
lead from `HeaderValue::from_static`.  While that version required 1.46,
this new function requires 1.49.  That is almost 8 months old, so
hopefully this isn't too controversial!
jeddenlea added a commit to jeddenlea/http that referenced this pull request Apr 15, 2022
... plus some clean-up.

It was only after I came up with the scheme using
`const fn from_bytes(&[u8]) -> Option<StandardHeader>`
that I noticed the debug+wasm32-wasi version of `parse_hdr`, which had
something very similar.

While cleaning up that function, I realized it still would still panic
if an attempted name was too long, which had been fixed for all other
targets and profiles in hyperium#433.  Then, I thought it would be worth seeing
if the use of `eq!` in the primary version of `parse_hdr` still made any
difference.

And, it would not appear so. At least not on x86_64, nor wasm32-wasi run
via wasmtime.  I've run the benchmarks a number of times now, and it
seems the only significant performance change anywhere is actually that
of `HeaderName::from_static` itself, which now seems to run in about 2/3
the time on average.

Unfortunately, `const fn` still cannot `panic!`, but I've followed the
lead from `HeaderValue::from_static`.  While that version required 1.46,
this new function requires 1.49.  That is almost 8 months old, so
hopefully this isn't too controversial!
seanmonstar pushed a commit that referenced this pull request Apr 16, 2022
... plus some clean-up.

It was only after I came up with the scheme using
`const fn from_bytes(&[u8]) -> Option<StandardHeader>`
that I noticed the debug+wasm32-wasi version of `parse_hdr`, which had
something very similar.

While cleaning up that function, I realized it still would still panic
if an attempted name was too long, which had been fixed for all other
targets and profiles in #433.  Then, I thought it would be worth seeing
if the use of `eq!` in the primary version of `parse_hdr` still made any
difference.

And, it would not appear so. At least not on x86_64, nor wasm32-wasi run
via wasmtime.  I've run the benchmarks a number of times now, and it
seems the only significant performance change anywhere is actually that
of `HeaderName::from_static` itself, which now seems to run in about 2/3
the time on average.

Unfortunately, `const fn` still cannot `panic!`, but I've followed the
lead from `HeaderValue::from_static`.  While that version required 1.46,
this new function requires 1.49.  That is almost 8 months old, so
hopefully this isn't too controversial!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HeaderName constructors panic for names longer than 64KiB
2 participants