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

Fix connection ownership issue #2217 #2221

Closed
wants to merge 2 commits into from
Closed

Conversation

basuke
Copy link
Contributor

@basuke basuke commented Jan 5, 2018

Before calling Curl_client_chop_write(), change the owner of connection
to the current Curl_easy handle. This will fix the issue #2217.

Before calling Curl_client_chop_write(), change the owner of connection
to the current Curl_easy handle. This will fix the issue curl#2217.
lib/easy.c Outdated
if(!result) {
struct connectdata *conn = data->easy_conn;
if(conn && conn->data != data && data->mstate > CURLM_STATE_CONNECT &&
data->mstate < CURLM_STATE_COMPLETED)
Copy link
Member

Choose a reason for hiding this comment

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

@bagder Why is the state check necessary here? I'm not suggesting it's wrong I just don't understand why it would be necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Also wouldn't the ownership need to be reverted after the loop completes? since curl_easy_pause can be called from anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jay I put those checks because I am not 100% sure about mstate changes. For me, it is reasonable to check.

For reverting the owner, as far as I've checked, the owner is just changed by multi handler before handling easy handle on demand basic. No assumption can be seen here. But I agree if you say that's better and safer.

Copy link
Member

Choose a reason for hiding this comment

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

@jay I'm actually not convinced the state check is necessary, but I also don't think it hurts since this function shouldn't really be called if that state check doesn't match.

Copy link
Member

Choose a reason for hiding this comment

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

And yes, I think a reversion at the end of the loop is a good idea so that other parts of the code won't get surprises.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll save and restore old owner at the end. Also I confirmed Curl_client_chop_write does not reply on mstate, so I will remove mstate here.

Also remove unnecessary state check.
@jay jay closed this in 2a6dbb8 Jan 9, 2018
@jay
Copy link
Member

jay commented Jan 9, 2018

Thanks

@lock lock bot locked as resolved and limited conversation to collaborators May 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants