-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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 data race on stats due to an access w/o acquiring the lock #573
Conversation
Note: This PR tackles a different issue than #575. |
On 64-bit x86 systems the dirty read there should be fine, but that is wrong for 32bit. Throwing that global lock around every connect is a non-starter. At very least the maxconns_fast check should happen unlocked, then either atomics or a separate lock for curr_conns should be used. |
I have changed the PR to only lock if If you give me a few pointers how you would like the additional lock for |
memcached.c
Outdated
res = write(sfd, str, strlen(str)); | ||
close(sfd); | ||
bool reject; | ||
if(settings.maxconns_fast) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is now a space between the if
and the condition.
reserved_fds is static before any listener sockets are opened, which should make that easier. I'm not sure off the top of my head if there's a better way to fix it. The way you've patched it is probably fine since the STATS_LOCK() was there before, and removing the branch from within the lock should help keep it short. |
out in 1.5.21, thanks! |
For fuck's sake, how did I miss this.. This logic completely breaks the original feature; now every connection is counted as a rejection. The maxconns test doesn't cover rejections being counted properly and I was too distracted to check the tests here. |
In
drive_machine
, thestats_state
is accessed without holding theSTATS_LOCK
, which causes data races against other threads that mutate the stats even while they are holding the lock.This PR proposes to keep the time that this lock is held to a minimum by acquiring the lock before the
if
and releasing it after potentially updating stats. It would also be possible to acquire the lock twice: once to compute the condition, and once to increment the count of rejected connections.This behavior was detected using Symbolic Execution techniques developed in collaboration by the SYMBIOSYS research project at COMSYS, RWTH Aachen University and Cesar Rodriguez (Diffblue, Ltd.). This research is supported by the European Research Council (ERC) under the EU's Horizon 2020 Research and Innovation Programme grant agreement n. 647295 (SYMBIOSYS).