Skip to content

condition that cause always passthrough bypass#1472

Closed
ykopel wants to merge 1 commit intoapache:masterfrom
ykopel:tr-pass
Closed

condition that cause always passthrough bypass#1472
ykopel wants to merge 1 commit intoapache:masterfrom
ykopel:tr-pass

Conversation

@ykopel
Copy link
Copy Markdown
Contributor

@ykopel ykopel commented Feb 19, 2017

Fix for 497e475.
This commit contains a condition that will always be true.
This is because ua_raw_buffer_reader->read_avail() always will be grater than zero.
You haven't see it until now because of another bug (#1471) that cause is_transparent_passthrough_allowed() to be always false and this for it never reached to your lines.
This is why I suggest to just delete those lines meantime.

Fix for apache/trafficserver@497e475.
This commit contains a condition that will always be true.
This is because ua_raw_buffer_reader->read_avail() always will be grater than zero.
You haven't see it until now because of another bug (5817738) that cause is_transparent_passthrough_allowed() to be always false and this for it never reached to your lines.
This is why I suggest to just delete those lines meantime.
@zwoop zwoop added this to the 7.2.0 milestone Feb 19, 2017
@SolidWallOfCode
Copy link
Copy Markdown
Member

Have you tested this in practice? ua_buffer_reader->read_avail() should be zero in most cases, because all of the header has been read and the user agent shouldn't send more data after that if there no is keep alive set (which is also checked).

@ykopel
Copy link
Copy Markdown
Contributor Author

ykopel commented Feb 22, 2017

I tested it in my env. And I see it is grater than zero.
There are several lines before it that assume that "read_avail" is grater than zero:

  // Reset the inactivity timeout if this is the first
  //   time we've been called.  The timeout had been set to
  //   the accept timeout by the ProxyClientTransaction
  //
  if ((ua_buffer_reader->read_avail() > 0) && (client_request_hdr_bytes == 0)) {
    milestones[TS_MILESTONE_UA_FIRST_READ] = Thread::get_hrtime();
    ua_session->set_inactivity_timeout(HRTIME_SECONDS(t_state.txn_conf->transaction_no_activity_timeout_in));
  }

@zwoop
Copy link
Copy Markdown
Contributor

zwoop commented Feb 22, 2017

[approve ci]

@atsci
Copy link
Copy Markdown

atsci commented Feb 22, 2017

Intel CC build successful! See https://ci.trafficserver.apache.org/job/icc-github/31/ for details.

@atsci
Copy link
Copy Markdown

atsci commented Feb 22, 2017

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1493/ for details.

@atsci
Copy link
Copy Markdown

atsci commented Feb 22, 2017

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1599/ for details.

@atsci
Copy link
Copy Markdown

atsci commented Feb 22, 2017

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/163/ for details.

@oknet
Copy link
Copy Markdown
Member

oknet commented Mar 1, 2017

It is fixed by “TS-5103: replace ua_raw_buffer_reader with ua_buffer_reader” and #1271
please check the code in master branch.

@oknet
Copy link
Copy Markdown
Member

oknet commented Mar 1, 2017

"TS-3100: Extend tr-pass to allow malformed HTTP GET requests to be blind tunneled." is added by @shinrich .

@zwoop
Copy link
Copy Markdown
Contributor

zwoop commented Apr 12, 2017

@oknet Does that mean we should close this PR (without merging) ?

@oknet
Copy link
Copy Markdown
Member

oknet commented Apr 13, 2017

@zwoop yes, close without merging.

@zwoop zwoop modified the milestones: 7.2.0, 8.0.0 Apr 25, 2017
@zwoop zwoop closed this Jun 8, 2017
@zwoop zwoop modified the milestone: 8.0.0 Jul 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants