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

connection_check: restore original conn->data after the check #3559

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@jay
Copy link
Member

commented Feb 12, 2019

  • Save the original conn->data before it's changed to the specified
    data transfer for the connection check and then restore it afterwards.

This is a follow-up to 38d8e1b 2019-02-11.

History:

It was discovered a month ago that before checking whether to extract a
dead connection that that connection should be associated with a "live"
transfer for the check (ie original conn->data ignored and set to the
passed in data). A fix was landed in 54b201b which did that and also
cleared conn->data after the check. The original conn->data was not
restored, so presumably it was thought that a valid conn->data was no
longer needed.

Several days later it was discovered that a valid conn->data was needed
after the check and follow-up fix was landed in bbae24c which partially
reverted the original fix and attempted to limit the scope of when
conn->data was changed to only when pruning dead connections. In that
case conn->data was not cleared and the original conn->data not
restored.

A month later it was discovered that the original fix was somewhat
correct; a "live" transfer is needed for the check in all cases
because original conn->data could be null which could cause a bad deref
at arbitrary points in the check. A fix was landed in 38d8e1b which
expanded the scope to all cases. conn->data was not cleared and the
original conn->data not restored.

A day later it was discovered that not restoring the original conn->data
may lead to busy loops in applications that use the event interface, and
given this observation it's a pretty safe assumption that there is some
code path that still needs the original conn->data. This commit is the
follow-up fix for that, it restores the original conn->data after the
connection check.

Assisted-by: tholin@users.noreply.github.com
Reported-by: tholin@users.noreply.github.com

Fixes #3542
Closes #xxxx


@jeroen @donny-dont @jnbr Please test this PR to make sure I didn't break the fixes for your issues (#3463, #3541).

/cc @tholin

connection_check: restore original conn->data after the check
- Save the original conn->data before it's changed to the specified
  data transfer for the connection check and then restore it afterwards.

This is a follow-up to 38d8e1b 2019-02-11.

History:

It was discovered a month ago that before checking whether to extract a
dead connection that that connection should be associated with a "live"
transfer for the check (ie original conn->data ignored and set to the
passed in data). A fix was landed in 54b201b which did that and also
cleared conn->data after the check. The original conn->data was not
restored, so presumably it was thought that a valid conn->data was no
longer needed.

Several days later it was discovered that a valid conn->data was needed
after the check and follow-up fix was landed in bbae24c which partially
reverted the original fix and attempted to limit the scope of when
conn->data was changed to only when pruning dead connections. In that
case conn->data was not cleared and the original conn->data not
restored.

A month later it was discovered that the original fix was somewhat
correct; a "live" transfer is needed for the check in all cases
because original conn->data could be null which could cause a bad deref
at arbitrary points in the check. A fix was landed in 38d8e1b which
expanded the scope to all cases. conn->data was not cleared and the
original conn->data not restored.

A day later it was discovered that not restoring the original conn->data
may lead to busy loops in applications that use the event interface, and
given this observation it's a pretty safe assumption that there is some
code path that still needs the original conn->data. This commit is the
follow-up fix for that, it restores the original conn->data after the
connection check.

Assisted-by: tholin@users.noreply.github.com
Reported-by: tholin@users.noreply.github.com

Fixes #3542
Closes #xxxx
@jeroen

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

Don't have time to test right now, but if you are able to run the crawler example for a few minutes without crashing, then my case is probably covered.

@jnbr

This comment has been minimized.

Copy link

commented Feb 12, 2019

works for me

@bagder

bagder approved these changes Feb 12, 2019

@jnbr jnbr referenced this pull request Feb 12, 2019

Closed

curl: update to 7.64.0. #8584

@bagder

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

(just the standard flakt appveyor fail, safe to ignore)

@bagder bagder closed this in 4015fae Feb 14, 2019

@jay jay deleted the jay:fix_conn_check branch Feb 18, 2019

@donny-dont

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

Also worked for WinCairo port of WebKit.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.