Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

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 •
edited
Loading

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 DDtKey mentioned this issue Jul 18, 2023
Open
@DDtKey
Copy link

DDtKey commented Jul 18, 2023 •
edited
Loading

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
Open
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
A-headers Area: HTTP headers E-easy Effort: easy. Start here :D S-docs Severity: docs. Explain things better.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

try_ methods to handle capacity overflow glebpom/http
3 participants
@seanmonstar @jongiddy @DDtKey