Fix: Lock interest_mutex_ When interest_ Modified in ctlEx #1237
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
EventMethEpollEquivImpl::ctlEx now locks interest_mutex_ when performing its add/mod/del actions. Previously, the interest_mutex_ lock was released before these add/mod/del actions were performed. This was confirmed to occasionally cause a "use after free" in findEmEventInAnInterestSet (called out of getReadyEmEvents) in another thread. It might also result in corruption of the interest_ std::set.
Member
|
Good find @dgreatwood. There appears to be a small hiccup in CI on the macOS runner trying to get through the |
Collaborator
Author
|
The issue with http_server_test is an intermittent issue I've seen before
and is unrelated to this checkin.
https://github.com/pistacheio/pistache/actions/runs/10626628792/job/29458477214
I'll write to you separately on that http_server_test issue.
…On Thu, Aug 29, 2024 at 10:09 PM Kip ***@***.***> wrote:
Good find @dgreatwood <https://github.com/dgreatwood>. There appears to
be a small hiccup
<https://github.com/pistacheio/pistache/actions/runs/10626628792/job/29458477214?pr=1237>
in CI on the macOS runner trying to get through the http_server_test.
—
Reply to this email directly, view it on GitHub
<#1237 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFMA25TVVUSQGPWMXYWIUDZT7467AVCNFSM6AAAAABNLVSKDSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRQGA3DSMJXGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
NOTICE: This email and its attachments may contain privileged and
confidential information, only for the viewing and use of the intended
recipient. If you are not the intended recipient, you are hereby notified
that any disclosure, copying, distribution, acting upon, or use of the
information contained in this email and its attachments is strictly
prohibited and that this email and its attachments must be immediately
returned to the sender and deleted from your system. If you received this
email erroneously, please notify the sender immediately. Xage Security,
Inc. and its affiliates will never request personal information (e.g.,
passwords, Social Security numbers) via email. Report suspicious emails to
***@***.*** ***@***.***>
|
Member
|
Kip, I would've waited for @tyler92 to provide some feedback before merging, since he's been providing a lot of useful feedback lately regarding misplaced locks and data races
|
Member
|
Sorry @Tachi107. @dgreatwood and I discussed offline prior to merging. Next time I'll wait longer for additional eyeballs to test and review. |
Collaborator
Author
|
Hi Andrea -
You may (or may not) be thinking of a different issue. I am still waiting
on tyler92's feedback on PR #1229, exactly as you said.
This issue here (PR #1237) shows up solely on macOS - Linux with
force-libevent does *not* show it - and tyler92 has been testing on Linux
so I think this PR #1237 is not his bag. It is also a pretty clear and safe
change.
I am definitely interested in his input on PR #1229 tho.
Best.
…On Fri, Aug 30, 2024 at 10:37 AM Kip ***@***.***> wrote:
Sorry @Tachi107 <https://github.com/Tachi107>. @dgreatwood
<https://github.com/dgreatwood> and I discussed offline prior to merging.
Next time I'll wait longer for additional eyeballs to test and review.
—
Reply to this email directly, view it on GitHub
<#1237 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFMA25ITLSNPCRMCU7UKK3ZUCUUZAVCNFSM6AAAAABNLVSKDSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRSGAZTSOJSG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
NOTICE: This email and its attachments may contain privileged and
confidential information, only for the viewing and use of the intended
recipient. If you are not the intended recipient, you are hereby notified
that any disclosure, copying, distribution, acting upon, or use of the
information contained in this email and its attachments is strictly
prohibited and that this email and its attachments must be immediately
returned to the sender and deleted from your system. If you received this
email erroneously, please notify the sender immediately. Xage Security,
Inc. and its affiliates will never request personal information (e.g.,
passwords, Social Security numbers) via email. Report suspicious emails to
***@***.*** ***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
EventMethEpollEquivImpl::ctlEx now locks interest_mutex_ when performing its add/mod/del actions.
Previously, the interest_mutex_ lock was released before these add/mod/del actions were performed. This was confirmed to occasionally cause a "use after free" in findEmEventInAnInterestSet (called out of getReadyEmEvents) in another thread. It might also result in corruption of the interest_ std::set.
This change affects the code path only when libevent is used.