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: webhook race condition crash #1525

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

lgrossi
Copy link
Contributor

@lgrossi lgrossi commented Aug 28, 2023

Solves #1522

When sending webhooks there is a small chance that the webhooks.empty() if passes but when it gets the task the webhooks deque is already empty.

That causes task to be a nullptr and accessing it causes a segmentation fault.

There are two ways to solve if: place the lock in the beginning of the function or check if task exists before proceeding, we are adopting the first.

@lgrossi lgrossi mentioned this pull request Aug 28, 2023
5 tasks
When sending webhooks there is a small chance that the webhooks.empty() if passes but when it gets the task the webhooks deque is already empty.

That causes task to be a nullptr and accessing it causes a segmentation fault.

There are two ways to solve if: place the lock in the beginning of the function or check if task exists before proceeding, we are adopting the first.
@lgrossi lgrossi force-pushed the lucas/fix-webhook-crash-race-condition branch from 728e9ca to a85b7e9 Compare August 28, 2023 23:42
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@lgrossi
Copy link
Contributor Author

lgrossi commented Aug 29, 2023

Signed off by @Sorairei

@lgrossi lgrossi merged commit 4c2d70b into main Aug 29, 2023
@dudantas dudantas deleted the lucas/fix-webhook-crash-race-condition branch August 29, 2023 05:45
@dudantas dudantas linked an issue Aug 29, 2023 that may be closed by this pull request
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Latest updates random crash
2 participants