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

Thread race updating proton raw connection max capacity #1198

Closed
kgiusti opened this issue Sep 13, 2023 · 1 comment
Closed

Thread race updating proton raw connection max capacity #1198

kgiusti opened this issue Sep 13, 2023 · 1 comment
Assignees
Milestone

Comments

@kgiusti
Copy link
Contributor

kgiusti commented Sep 13, 2023

The router tracks the maximum free buffer capacity offered by a proton raw connection. See here and here.

Note that max_capacity is a global variable, and that these functions may be called simultaneously on different threads.

Given that concurrency it is possible that max_capacity may not reflect the actual maximum capacity if two threads invoke this code at the same time with different values of capacity that are both greater than the current value of max_capacity.

Example: assume max_capacity is 1. Thread A calls the code with capacity=3 and Thread B calls the code with capacity=5 at the same time. Depending on order both threads may see (capacity > max_capacity) as True. That means both threads attempt to set max_capacity. Again depending on order the last write to max_capacity wins, and in this case it may be 3, not 5.

While that probably isn't a big deal - eventually there will be enough calls to probably store the correct max_capacity. However the real issue is further down the function, see here and here.

The race may result in a violation that max_capacity is > capacity and the result of max_capacity - capacity is negative. In the example for thread B capacity will be 5 but max_capacity will be 3, the result will overflow the size_t. That's the bug that results from the race.

TL;DR: the test and update of max_capacity has to be atomic.

@kgiusti kgiusti self-assigned this Sep 13, 2023
@kgiusti
Copy link
Contributor Author

kgiusti commented Sep 13, 2023

Note: this race is suppressed in tsan.supp. Remove the suppression as part of the fix.

kgiusti added a commit to kgiusti/skupper-router that referenced this issue Sep 18, 2023
kgiusti added a commit to kgiusti/skupper-router that referenced this issue Sep 18, 2023
kgiusti added a commit to kgiusti/skupper-router that referenced this issue Sep 20, 2023
kgiusti added a commit to kgiusti/skupper-router that referenced this issue Sep 21, 2023
@ganeshmurthy ganeshmurthy added this to the 2.5.0 milestone Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants