Skip to content

TS-4396: fix number_of_redirections off-by-one #786

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.

Sign up for GitHub

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

Closed
mingzym wants to merge 1 commit into from
Closed

TS-4396: fix number_of_redirections off-by-one #786

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions proxy/http/HttpSM.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1954,9 +1954,7 @@ HttpSM::state_read_server_response_header(int event, void *data)
t_state.api_next_action = HttpTransact::SM_ACTION_API_READ_RESPONSE_HDR;

// if exceeded limit deallocate postdata buffers and disable redirection
if (enable_redirection && (redirection_tries < t_state.txn_conf->number_of_redirections)) {
++redirection_tries;
} else {
if (enable_redirection && (redirection_tries >= t_state.txn_conf->number_of_redirections)) {
Copy link
Copy Markdown
Contributor

@jpeach jpeach Jul 5, 2016

Choose a reason for hiding this comment

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

Can we check is_redirect_required() here?

Copy link
Copy Markdown
Member Author

@mingzym mingzym Jul 6, 2016

Choose a reason for hiding this comment

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

I don't think so this is a break of the multiple redirection, and make the post redirection cleanup, we are in progress of a redirection circle, and the SM is reseted, we should not check the old information here. and if we need to stop, we don't need to check the response anyway.

Copy link
Copy Markdown
Contributor

@jpeach jpeach Jul 6, 2016

Choose a reason for hiding this comment

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

Yes, I asked about this because I was not sure whether we should be checking the response here. The problem that I see with this change is that the condition disagrees with is_redirect_required() when redirection_tries is equal to number_of_redirections.

tunnel.deallocate_redirect_postdata_buffers();
enable_redirection = false;
}
Expand Down Expand Up @@ -7710,6 +7708,7 @@ HttpSM::redirect_request(const char *redirect_url, const int redirect_len)
}

t_state.redirect_info.redirect_in_process = true;
redirection_tries++;

// set the passed in location url and parse it
URL &redirectUrl = t_state.redirect_info.redirect_url;
Expand Down