dnsthread: register the wakeup listener before starting work - #439
Open
woahwhattheheck wants to merge 1 commit into
Open
dnsthread: register the wakeup listener before starting work#439woahwhattheheck wants to merge 1 commit into
woahwhattheheck wants to merge 1 commit into
Conversation
dnsthread_resolveone() set THREAD_HASWORK and signalled the worker thread before registering the wakeup listener, and err1 only unlocked the mutex. A failed events_network_register() therefore returned -1 with a resolution already running and nothing registered to read its completion byte. callback_resolveone() is the only thing which frees T->addr and the only consumer of T->sas, so both leaked, and the caller's callback never fired. The undrained byte then made the next dnsthread_resolveone() on the same thread fire callback_resolveone() immediately, which freed T->addr while the worker was passing that same pointer to sock_resolve(); the worker drops the mutex for the duration of the resolution, so the lock does not prevent this. That callback also handed the previous resolution's result to the new request. Register the listener before handing the work over, so that a failure leaves the worker untouched and the address can simply be freed. err2 frees the address; err3 additionally restores THREAD_SLEEPING and cancels the registration, and is reached only if pthread_cond_signal() fails. err1 still handles the strdup() failure, where there is nothing to free.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Fixes #438.
dnsthread_resolveone()setTHREAD_HASWORKand signalled the worker threadbefore registering the wakeup listener, and
err1only unlocked the mutex.A failed
events_network_register()therefore returned -1 with a resolutionalready running and nothing registered to read its completion byte:
T->addrand
T->sasboth leaked, the caller's callback never fired, and theundrained byte made the next
dnsthread_resolveone()firecallback_resolveone()immediately — freeingT->addrwhile the workerthread was passing that same pointer to
sock_resolve().The change
Register the listener before handing the work over, and unwind properly:
err2frees the strduped address — reached when the registration fails,at which point the worker has not been touched.
err3additionally restoresTHREAD_SLEEPINGand cancels theregistration — reached only if
pthread_cond_signal()fails.err1is unchanged and still handles thestrdup()failure, where there isnothing to free.
events_network_cancel()'s return is ignored, matchingnetwork_accept.c,network_connect.c,network_read.candnetwork_write.c.Note
I did not try to make the failure path take the work back from the worker
once it is running, because it cannot be done safely — the worker drops the
mutex for the duration of
sock_resolve(). Doing the registration firstmeans that situation never arises.