Skip to content
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

Jump to bottom

TS-3743: Avoid crash due to accessing dead session under load #1006

Closed
wants to merge 1 commit into from
Closed

TS-3743: Avoid crash due to accessing dead session under load #1006

wants to merge 1 commit into from

Conversation

niq
Copy link
Member

@niq niq commented Sep 11, 2016

Patch by Sam Baskinger

@atsci
Copy link

atsci commented Sep 11, 2016

Linux build failed! See https://ci.trafficserver.apache.org/job/Github-Linux/674/ for details.

@atsci
Copy link

atsci commented Sep 11, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/778/ for details.

@zwoop
Copy link
Contributor

zwoop commented Sep 12, 2016

Clang format i think

@zwoop zwoop added the Core label Sep 12, 2016
@zwoop zwoop added this to the 7.0.0 milestone Sep 12, 2016
@SolidWallOfCode
Copy link
Member

What is the use case for this? Can this happen with just HTTP/1 clients or only with HTTP/2?

@SolidWallOfCode
Copy link
Member

Looking that the checks for m_active there is an apparently assumption that the value can change over the course of the HttpSM::setup_internal_transfer. Give the instance pointed at by ua_session should be locked along with the HttpSM instance, how can that happen? Even without locks any such change must happen in another thread in which case the member really needs to be volatile.

@zwoop zwoop modified the milestones: 7.1.0, 7.0.0 Sep 15, 2016
PSUdaemon
PSUdaemon reviewed Sep 27, 2016
View reviewed changes
proxy/http/HttpSM.cc
tunnel.tunnel_run(p);

// Only actually try to do this on live sessions.
if (ua_session->m_active) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we become active between line 6172 and line 6263?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, what @SolidWallOfCode said!

@PSUdaemon PSUdaemon removed their assignment Sep 27, 2016
@bryancall
Copy link
Contributor

Please clang format the change.

@zwoop zwoop modified the milestones: 7.1.0, 7.2.0 Jan 8, 2017
@zwoop zwoop modified the milestones: 7.2.0, 8.0.0 Apr 25, 2017
@bryancall
Copy link
Contributor

[approve ci]

1 similar comment
@bryancall
Copy link
Contributor

[approve ci]

@bryancall
Copy link
Contributor

If this is still and issue please reopen.

@bryancall bryancall closed this Jul 20, 2017
@zwoop zwoop removed this from the 8.0.0 milestone May 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PSUdaemon PSUdaemon PSUdaemon left review comments

@SolidWallOfCode SolidWallOfCode Awaiting requested review from SolidWallOfCode

Assignees

@niq niq

Labels
Core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@niq @atsci @zwoop @SolidWallOfCode @bryancall @PSUdaemon