Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

fix(security): fix bug in negotiation_service::on_negotiation_request when rpc_session is closed #652

Merged
merged 18 commits into from
Nov 4, 2020

Conversation

levy5307
Copy link
Contributor

@levy5307 levy5307 commented Oct 28, 2020

  1. Previously, if one rpc session is disconnected, we should remove the corresponding negotiation in negotiation_service.

In multi-thread environment, if thread A receive a negotiation message, and pass this message to negotiation_service::on_negotiation_request, which will find the correspoing negotiation to deal with this negotiation message.

And before this message is passed to negotiation_service::on_negotiation_reques, thread B receive a message which is illegal to process(for example: get operation), which will produce the rpc_session disconnected. In the meanwhile, the negotiation is removed.

So in negotiation_service::on_negotiation_request of thead A, the corresponding negotiation will be nullptr, this will produce a coredump.

Here is the process progress:

thread A:     -->receive nego msg--------------------------------------------------------------->get negotiation----->......---->

thread B: --------> receive get mesage-->close rpc_session-->remove negotiation----->......----->

Same situation with client_negotiation processing response.

  1. The ownership of the negotiation instance is held by rpc_session, because I refactor it before. security is on the upper level of network now. So negotiation is better to have the ownership of rpc_session.

  2. change the name from negotiaiton_service to negotiation_manager

@neverchanje neverchanje added the type/bug-fix This PR fixes a bug. label Oct 28, 2020
@hycdong hycdong merged commit 1863cf9 into XiaoMi:master Nov 4, 2020
@levy5307 levy5307 deleted the fix-negotiation-service branch November 4, 2020 03:47
levy5307 added a commit to levy5307/rdsn that referenced this pull request Dec 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants