Add an optional size limit to read_line
#25
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously a call to `read_line` would read and buffer arbitrary amounts of data. If an untrusted entity supplies the data (e.g. consider a web-server reading a request or header line) then the `read_line` can be provoked into allocating so much memory that it kills the whole process / server. This patch changes the API by adding an optional `?len` parameter (in the same style as `read_some` and `read_exactly`) to bound the maximum length of the line. If the bound is exceeded then the function returns an error (`Line_too_long`) Users of this library dealing with untrusted data should decide on a suitable `len` value to avoid possible memory exhaustion. This patch includes a couple of extra unit tests. Signed-off-by: David Scott <dave@recoil.org>
In the case of an HTTP server using `read_line`, it should return 413 Entity too large if the headers exceed an implementation-specific limit. Signed-off-by: David Scott <dave@recoil.org>
djs55
added a commit
to djs55/ocaml-cohttp
that referenced
this pull request
May 1, 2018
Although the HTTP spec does not impose a maximum limit on header lengths, most implementations impose a limit to avoid a client from exhausting all available server memory. According to https://stackoverflow.com/a/8623061 the typical limit is between 4k and 48k, but this usually applies to the sum of the request line and all the headers. It's more convenient for us to apply the limit per header. This requires [mirage/mirage-channel#25] Signed-off-by: David Scott <dave@recoil.org>
avsm
reviewed
May 1, 2018
djs55
added a commit
to djs55/vpnkit
that referenced
this pull request
May 1, 2018
This imports mirage/mirage-channel#25 Signed-off-by: David Scott <dave.scott@docker.com>
djs55
added a commit
to djs55/vpnkit
that referenced
this pull request
May 1, 2018
This imports mirage/mirage-channel#25 Signed-off-by: David Scott <dave.scott@docker.com>
Previously `read_line` would drop the data from the channel when returning the `Line_too_long` error. This patch puts the data back in the input queue to make it easier to handle this error. For example an application would be able to call `read_some` or `read_exactly` to peek at the data and make a better error message. Signed-off-by: David Scott <dave@recoil.org>
|
My intuition about this is: you'd like to have an upper bound of the allocation for Naming: I'd prefer Semantics: What happens if
|
|
just a quick comment about names: |
|
Is this change going to be merged at a certain point? |
|
@djs55, any thoughts on @hannesm comments above? #25 (comment) Would like to get this into next cohttp |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously a call to
read_linewould read and buffer arbitrary amounts of data. If an untrusted entity supplies the data (e.g. consider a web-server reading a request or header line) then theread_linecan be provoked into allocating so much memory that it kills the whole process / server.This patch changes the API by adding an optional
?lenparameter (in the same style asread_someandread_exactly) to bound the maximum length of the line. If the bound is exceeded then the function returns an error (Line_too_long)Users of this library dealing with untrusted data should decide on a suitable
lenvalue to avoid possible memory exhaustion.This patch includes a couple of extra unit tests.
Signed-off-by: David Scott dave@recoil.org