Skip to content

Fix heap-use-after-free on Http2Stream::destroy() #6346

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
Merged
masaori335 merged 1 commit into from
Jan 27, 2020
Merged

Fix heap-use-after-free on Http2Stream::destroy() #6346

masaori335 merged 1 commit into from
Jan 27, 2020

Conversation

@masaori335
Copy link
Contributor

@masaori335 masaori335 commented Jan 24, 2020

Fix #6345.

@masaori335 masaori335 added this to the 10.0.0 milestone Jan 24, 2020
@masaori335 masaori335 self-assigned this Jan 24, 2020
masaori335
masaori335 commented Jan 24, 2020
View reviewed changes
@masaori335 masaori335 force-pushed the 6345 branch 2 times, most recently from 4e6df85 to feecbae Compare January 27, 2020 04:39
@masaori335 masaori335 changed the title Avoid scheduling EOS event to itself Fix heap-use-after-free on Http2Stream::destroy() Jan 27, 2020
@masaori335 masaori335 marked this pull request as ready for review January 27, 2020 04:39
maskit
maskit reviewed Jan 27, 2020
View reviewed changes
proxy/http2/Http2Stream.cc
h2_proxy_ssn->connection_state.release_stream(this);

cid = _proxy_ssn->connection_id();
// Do not access `_proxy_ssn` in below. It might be freed by `release_stream`.
Copy link
Member

@maskit maskit Jan 27, 2020

Choose a reason for hiding this comment

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

Looks familiar. Don't we have a similar comment somewhere? Or did we remove it and now putting it back? We may want something we can avoid the heap-use-after-free.

Copy link
Contributor Author

@masaori335 masaori335 Jan 27, 2020

Choose a reason for hiding this comment

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

This one? I agree with we need something.

trafficserver/proxy/http2/Http2ConnectionState.cc

Lines 1378 to 1382 in 48bcbe6

ua_session->destroy();
// Can't do this because we just destroyed right here ^,
// or we can use a local variable to do it.
// ua_session = nullptr;

maskit
maskit approved these changes Jan 27, 2020
View reviewed changes
Copy link
Member

@maskit maskit left a comment

Choose a reason for hiding this comment

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

I'm not sure if this fix the issue, but the change makes sense.

zizhong
zizhong approved these changes Jan 27, 2020
View reviewed changes
Copy link
Member

@zizhong zizhong left a comment

Choose a reason for hiding this comment

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

Looks good

@masaori335 masaori335 merged commit ca67471 into apache:master Jan 27, 2020
@zwoop zwoop modified the milestones: 10.0.0, 9.0.0 Feb 5, 2020
@zwoop
Copy link
Contributor

zwoop commented Feb 5, 2020

Cherry-picked to v9.0.x branch.

@masaori335 masaori335 mentioned this pull request Mar 31, 2020
Merged
@zwoop zwoop modified the milestones: 9.0.0, 8.1.0 Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maskit maskit maskit approved these changes

@shinrich shinrich shinrich approved these changes

@zizhong zizhong zizhong approved these changes

Assignees

@masaori335 masaori335

Labels

HTTP/2

Projects

None yet

Milestone

8.1.0

Development

Successfully merging this pull request may close these issues.

ASan: heap-use-after-free on Http2Stream::destroy()

5 participants

@masaori335 @zwoop @maskit @shinrich @zizhong