Skip to content

Adds a new configuration proxy.config.http.allow_multi_range #3106

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

Merged
merged 1 commit into from
Feb 14, 2018

Conversation

zwoop
Copy link
Contributor

@zwoop zwoop commented Feb 10, 2018

This is needed to prevent potential abuse with well formed multi-
range requests.

@zwoop zwoop added this to the 8.0.0 milestone Feb 10, 2018
@zwoop zwoop self-assigned this Feb 10, 2018
@zwoop zwoop requested a review from oknet February 11, 2018 21:57
@@ -4428,24 +4428,47 @@ HttpSM::do_range_setup_if_necessary()
do_range_parse(field);

if (t_state.range_setup == HttpTransact::RANGE_REQUESTED) {
bool do_transform = false;

if (!t_state.range_in_cache) {
Debug("http_range", "range can't be satisfied from cache, force origin request");
Copy link
Member

Choose a reason for hiding this comment

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

t_state.num_range_fields is -1 here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you are saying here. How can it be -1, if we have

t_state.range_setup == HttpTransact::RANGE_REQUESTED

This gets set, as far as I can tell, in HttpSM::parse_range_and_compare, no ?

Copy link
Member

Choose a reason for hiding this comment

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

Within HttpSM::parse_range_and_compare, if

  • parse failed, goto Lfaild;
  • the number of valid range is not greater than 0, goto Lfaild;

At the Lfaild, range_in_cache = false and num_range_fields = -1.

t_state.range_setup = HttpTransact::RANGE_REQUESTED and t_state.num_range_fields = nr only if nr > 0.

The valid value of num_range_fields should be -1 or any number great than 0.

} else {
t_state.range_setup = HttpTransact::RANGE_NOT_SATISFIABLE;
}
} else {
Copy link
Member

Choose a reason for hiding this comment

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

t_state.num_range_fields is 1 here.

Copy link
Contributor Author

@zwoop zwoop Feb 12, 2018

Choose a reason for hiding this comment

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

Hmm, you saying I should reset num_range_fields to 0 ? That certainly seems reasonable, I've updated the PR with that change.

This is needed to prevent potential abuse with well formed multi-
range requests.
@zwoop zwoop merged commit 3b48c5e into apache:master Feb 14, 2018
@zwoop zwoop deleted the MultiRange branch February 14, 2018 04:18
@zwoop
Copy link
Contributor Author

zwoop commented Feb 14, 2018

This has many, but trivial, merge conflicts, primarily due to clang-formatting.

@zwoop zwoop modified the milestones: 8.0.0, 7.1.3 Feb 14, 2018
@zwoop
Copy link
Contributor Author

zwoop commented Feb 14, 2018

Cheery-picked to 7.1.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants