An improper locking due to the unreleased lock before program exit

[ryancaicse]

Hi developers, in the below codes, the lock &T->mtx could be not released before program's exit exit(1); when noeintr_write(T->wakeupsock[0], &zero, 1) != 1 . I think there is no harm to write pthread_mutex_unlock(&T->mtx) before the exit(1); for better resource management and code symmetry. Thanks!

spiped/lib/dnsthread/dnsthread.c

Lines 107 to 120 in 5ce9cff

	if ((rc = pthread_mutex_lock(&T->mtx)) != 0) {
	warn0("pthread_mutex_lock: %s", strerror(rc));
	exit(1);
	}
	/* Write the answer back. */
	T->sas = sas;
	T->res_errno = res_errno;
	/* Send a completion message. */
	if (noeintr_write(T->wakeupsock[0], &zero, 1) != 1) {
	warnp("Error writing to wakeup socket");
	exit(1);
	}

Best,

[gperciva]

Hmm. It's quite true that the mutex isn't released, and there's no harm in doing

--- a/lib/dnsthread/dnsthread.c
+++ b/lib/dnsthread/dnsthread.c
@@ -116,6 +116,8 @@ workthread(void * cookie)
                /* Send a completion message. */
                if (noeintr_write(T->wakeupsock[0], &zero, 1) != 1) {
                        warnp("Error writing to wakeup socket");
+                       if ((rc = pthread_mutex_unlock(&T->mtx)) != 0)
+                               warn0("pthread_mutex_unlock: %s", strerror(rc));
                        exit(1);
                }

However, if we want to fully clean up, we'd also need to call pthread_cond_destroy(), pthread_mutex_destroy(), and free(T). I suspect that using pthread_cleanup_push() and pthread_cleanup_pop() would be a safer approach, like we did in spiped/pushbits.c.

... that said, I guess that whether or not we clean up the other resources, we'd still need a specific pthread_mutex_unlock() in that spot, since it can't be handled by a generic "clean up" function. Hmm.

Let me think about it for a bit. I have a higher-priority task to finish first, then I'll revisit this in a few days.

Thanks for the report, @ryancaicse!

[ryancaicse]

@gperciva OK, Thank you very much.