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

Undocumented capacity limit for HeaderMap #603

Open
jongiddy opened this issue Apr 27, 2023 · 3 comments · May be fixed by #617
Open

Undocumented capacity limit for HeaderMap #603

jongiddy opened this issue Apr 27, 2023 · 3 comments · May be fixed by #617
Labels
A-headers Area: HTTP headers E-easy Effort: easy. Start here :D S-docs Severity: docs. Explain things better.

Comments

@jongiddy
Copy link

HeaderMap has a capacity limit of 24576 headers. Adding any more headers triggers a panic when reserving more capacity.

I can't see any way to prevent the panic when handling incoming headers, except to hard-wire a check for this value into calling code.

It would be useful to expose this value as a public constant.

At the very least it should be documented. Currently HeaderMap::reserve says "Panics if the new allocation size overflows usize." This should be "Panics if the new allocation size is greater than 24576."

@seanmonstar
Copy link
Member

seanmonstar commented Apr 28, 2023

Good catch, thanks for reporting this! I'd propose a few improvements:

  • Add a section to the struct docs about the max capacity. Since there are a few methods that would allocate more space, seems better to have a single place to refer to the limit.
  • Update reserve to say "panicks if the new size is larger than the internal max. See struct docs for more."
  • Optional: Add a try_reserve method. This changes the task from a docs fix to a feature request, so perhaps that should be a separate ticket if anyone wants this method.

@seanmonstar seanmonstar added E-easy Effort: easy. Start here :D A-headers Area: HTTP headers S-docs Severity: docs. Explain things better. labels Apr 28, 2023
@DDtKey
Copy link

DDtKey commented Jul 18, 2023

I believe the correct way to fix that is to introduce fallible try_append method.
It's too fragile to rely on reservation methods & documentation only and easy to miss the handling.

It's clearly vulnerability right now, because all usages like this one will cause a panic

cc @seanmonstar

@seanmonstar
Copy link
Member

Yea, I think the three bullet points I listed above would be a great addition. Want to submit a PR?

@DDtKey DDtKey linked a pull request Jul 20, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-headers Area: HTTP headers E-easy Effort: easy. Start here :D S-docs Severity: docs. Explain things better.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants