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

TS-4970: Crash in INKVConnInternal when handle_event is called after destroy() #1109

Closed
wants to merge 1 commit into
base: 6.2.x
from

Conversation

Projects
None yet
6 participants
@jacksontj
Contributor

jacksontj commented Oct 14, 2016

No description provided.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 14, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1009/ for details.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 14, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/901/ for details.

@jpeach

This comment has been minimized.

Show comment
Hide comment
@jpeach

jpeach Oct 14, 2016

Contributor

Are we using github for backports? I thought the process was to mark the JIRA?

Contributor

jpeach commented Oct 14, 2016

Are we using github for backports? I thought the process was to mark the JIRA?

@jacksontj

This comment has been minimized.

Show comment
Hide comment
@jacksontj

jacksontj Oct 14, 2016

Contributor

@jpeach TBH I'm not sure, I figured the PR would be good at least to get the CI to run on it-- I'm not sure what our official process is now that we do github for some things

Contributor

jacksontj commented Oct 14, 2016

@jpeach TBH I'm not sure, I figured the PR would be good at least to get the CI to run on it-- I'm not sure what our official process is now that we do github for some things

@SolidWallOfCode

This comment has been minimized.

Show comment
Hide comment
@SolidWallOfCode

SolidWallOfCode Oct 14, 2016

Member

Does the destroy method also clear m_deletable?

Member

SolidWallOfCode commented Oct 14, 2016

Does the destroy method also clear m_deletable?

@shinrich

This comment has been minimized.

Show comment
Hide comment
@shinrich

shinrich Oct 14, 2016

Member

Would it be easier to just back port the fix from TS-4590? I'm a bit concerned about the proposed fix in this PR. I don't think is correctly using the m_deleted/m_deletable parameters?

Member

shinrich commented Oct 14, 2016

Would it be easier to just back port the fix from TS-4590? I'm a bit concerned about the proposed fix in this PR. I don't think is correctly using the m_deleted/m_deletable parameters?

@jacksontj

This comment has been minimized.

Show comment
Hide comment
@jacksontj

jacksontj Oct 14, 2016

Contributor

@shinrich we could-- this patch seems to be working fine for our build though-- seemed like a less intrusive patch to an LTS release.

Contributor

jacksontj commented Oct 14, 2016

@shinrich we could-- this patch seems to be working fine for our build though-- seemed like a less intrusive patch to an LTS release.

@jacksontj

This comment has been minimized.

Show comment
Hide comment
@jacksontj

jacksontj Oct 17, 2016

Contributor

@shinrich I did look a bit more into the other backport-- and it seems that the outcome would be quite similar-- as the handle_event method is still using this m_deleted flag to determine if the struct was deleted, so it ends up being a superset of this change.

Contributor

jacksontj commented Oct 17, 2016

@shinrich I did look a bit more into the other backport-- and it seems that the outcome would be quite similar-- as the handle_event method is still using this m_deleted flag to determine if the struct was deleted, so it ends up being a superset of this change.

@jacksontj

This comment has been minimized.

Show comment
Hide comment
@jacksontj

jacksontj Oct 17, 2016

Contributor

After doing some more looking, the m_deleted flag is just marking the VConn as "we should delete this" and that combined with m_deletable lets it reschedule the delete in the future when it can. The fundamental problem I'm seeing is it is double freed under some conditions-- since all events trigger a delete. This is fixed in 7.x with TS-4590 -- so I've updated this PR to workaround the issue in a simpler mechanism that more closely mirrors what is happening on 7.x

Contributor

jacksontj commented Oct 17, 2016

After doing some more looking, the m_deleted flag is just marking the VConn as "we should delete this" and that combined with m_deletable lets it reschedule the delete in the future when it can. The fundamental problem I'm seeing is it is double freed under some conditions-- since all events trigger a delete. This is fixed in 7.x with TS-4590 -- so I've updated this PR to workaround the issue in a simpler mechanism that more closely mirrors what is happening on 7.x

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 17, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1035/ for details.

@atsci

This comment has been minimized.

Show comment
Hide comment
@atsci

atsci commented Oct 17, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/927/ for details.

@zwoop

This comment has been minimized.

Show comment
Hide comment
@zwoop

zwoop Oct 18, 2016

Contributor

Is this a duplicate PR?

Contributor

zwoop commented Oct 18, 2016

Is this a duplicate PR?

@shinrich

This comment has been minimized.

Show comment
Hide comment
@shinrich

shinrich Oct 18, 2016

Member

Not a complete solution as Thomas noted, but may be sufficient to keep 6.2.x moving. Alternatively, we may just want to back port TS-4590.

Member

shinrich commented Oct 18, 2016

Not a complete solution as Thomas noted, but may be sufficient to keep 6.2.x moving. Alternatively, we may just want to back port TS-4590.

@zwoop zwoop added the Backport label Oct 20, 2016

@zwoop zwoop added this to the 6.2.1 milestone Oct 20, 2016

@jacksontj

This comment has been minimized.

Show comment
Hide comment
@jacksontj

jacksontj Oct 26, 2016

Contributor

Closing the PR-- as we'll take care of this in Jira.

Contributor

jacksontj commented Oct 26, 2016

Closing the PR-- as we'll take care of this in Jira.

@jacksontj jacksontj closed this Oct 26, 2016

@zwoop zwoop modified the milestone: 6.2.1 May 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment