facebook / facebook/proxygen

HTTPSessionAcceptor::getController() should return a raw pointer, instead of std::shared_ptr

Open
#554 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
8.4k
Forks
1.5k
Avg merge
10m
Merged PRs (30d)
2

Description

The reason why it should return a raw pointer is because: One HTTPSessionController is mapped to one HTTPSession. An HTTPSessionAcceptor manages multiple controllers. It is better to follow the same lifetime management as HQSessionController, which return a raw pointer and let itself do self-destroyed in HQSessionController::detachSession().

Otherwise, in https://github.com/facebook/proxygen/blob/3f8d21fc2fb4def298785b28d79168e301438c20/proxygen/lib/http/session/HTTPSessionAcceptor.cpp#L71, controller is created as shared_ptr, but used as raw pointer in https://github.com/facebook/proxygen/blob/3f8d21fc2fb4def298785b28d79168e301438c20/proxygen/lib/http/session/HTTPSessionAcceptor.cpp#L99. Then it is auto-released after HTTPSessionAcceptor::onNewConnection is done.

It will increase a lot of burden on HTTPSessionAcceptor side to maintain a copy of std::shared_ptr to handle each HTTPSessionController's lifetime. Otherwise, without a copy inside HTTPSessionAcceptor, after HTTPSessionAcceptor::onNewConnection is done, HTTPSessionController will be released, then HTTPDownstreamSession will encounter a potential heap-use-after-free issue.

blame commit: https://github.com/facebook/proxygen/commit/ad9fd639d74f19fa771ebe04e64f1eb723073e37

My suggestion is to revert the above commit.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.