Skip to content

Conversation

@shinrich
Copy link
Member

I had added these client_vc checks to try and address stale references to client_vc pointers that we were seeing. However referencing the session object after calling do_io_close is dangerous because the session object may have been freed on return. @bneradt caught this as a use-after-free when working with an ASAN build and the traffic-dump plugin in our prod sym environment.

My current theory on the stale client_vc is that the netvc is closing due to EOS or an error while the read/write_vio's have the continuation set to 0. In that case the SM/tunnel/session will not be notified that the netvc has been deleted. I added a Warning message so we can look for that case in our logs. Thought about adding an assert, but I figured the Warning would be less invasive. It may be ok to have an unattached netvc in some cases.

I also removed one set of do_io_read null's that I don't think are necessary.

@shinrich shinrich added HTTP ASan Address Sanitizer labels Mar 26, 2020
@shinrich shinrich added this to the 10.0.0 milestone Mar 26, 2020
@shinrich shinrich self-assigned this Mar 26, 2020
@shinrich
Copy link
Member Author

[approve ci debian]

@ywkaras
Copy link
Contributor

ywkaras commented Mar 31, 2020

Have you or @bneradt retested to make sure this is really a fix?

@shinrich
Copy link
Member Author

Yes, @bneradt reran ASAN with this change.

@shinrich shinrich mentioned this pull request Apr 27, 2020
@shinrich
Copy link
Member Author

shinrich commented May 1, 2020

There are better ways of doing this by reconsidering the "fix" that caused this use after free. I have a new branch to PR once we get some more experience with it.

@shinrich shinrich closed this May 1, 2020
@zwoop
Copy link
Contributor

zwoop commented May 5, 2020

Please remember to remove Milestone and Projects from PRs that are closed without merging.

@zwoop zwoop removed this from the 10.0.0 milestone May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ASan Address Sanitizer HTTP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants