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.

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

client: fix access violation #9793

Merged
merged 2 commits into from Jun 28, 2016
Merged

Conversation

xiexingguo
Copy link
Member

@xiexingguo xiexingguo commented Jun 18, 2016

The MetaRequest is a refcounted object. On construction, it will default ref to 1.
The general process to make an MDS request will be:

  MetaRequest *req = new MetaRequest(op);
  // fill in something
  int r = make_request(req, uid, gid, target);
  return r;

As mentioned, make_request() will put the passed request before returning(and thereby
delete the request automatically).

To be more specific here, the make_request() will call "mds_requests[tid] = request->get();"
to keep the request well local tracked on entry(which raises the ref of request to 2).
And on error exit(especially for the following no reply is received case), by calling both

    unregister_request(request);
    put_request(request); // ours 

, it will decrease the reference of the corresponding request to zero and thereby delete request.
Therefore the call to request thereafter is at risk of "access violation".

Signed-off-by: xie xingguo xie.xingguo@zte.com.cn

@gregsfortytwo
Copy link
Member

Can you explain the reference counting a little more in the commit message? I think you're right but we do have references floating around here that are supposed to be removed. (The fact that cleanup is moving from the more usual handle_client_reply makes it a little more clear.)

The MetaRequest is a refcounted object. On construction, it will default ref to 1.
The general process to make an MDS request will be:

  MetaRequest *req = new MetaRequest(op);
  // fill in something
  int r = make_request(req, uid, gid, target);
  return r;

As mentioned, make_request() will put the passed request before returning(and thereby
delete the request automatically).

To be more specific here, the make_request() will call "mds_requests[tid] = request->get();"
to keep the request well local tracked on entry(which raises the ref of request to 2).
And on error exit(especially for the following no reply is received case), by calling both

    unregister_request(request);
    put_request(request); // ours

, it will decrease the reference of the corresponding request to zero and thereby delete request.
Therefore the call to request thereafter is at risk of "access violation".

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
Which is misleading.

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
@xiexingguo
Copy link
Member Author

@gregsfortytwo
Copy link
Member

@gregsfortytwo gregsfortytwo merged commit ee95553 into ceph:master Jun 28, 2016
@xiexingguo xiexingguo deleted the xxg-wip-client-mr branch June 28, 2016 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants