Incorrect usage of pthread_kill() leads to crashes

[iskunk]

In further testing of Motion (Git master), I've observed crashes that occur shortly after the watchdog mechanism kills a thread. Valgrind reports an invalid read in pthread_kill() as called from main() in motion.c...

                    if (cnt_list[i]->watchdog == WATCHDOG_KILL) {
                        /* if 0 then it finally did clean up (and will restart without any further action here)
                         * kill(, 0) == ESRCH means the thread is no longer running
                         * if it is no longer running with running set, then cleanup here so it can restart
                         */
                        if(cnt_list[i]->running && pthread_kill(cnt_list[i]->thread_id, 0) == ESRCH) {
/* HERE -------------------------------------------^^^^^^^^^^^^ */
                            MOTION_LOG(ERR, TYPE_ALL, NO_ERRNO, "%s: cleaning Thread %d", cnt_list[i]->threadnr);
                            pthread_mutex_lock(&global_lock);
                            threads_running--;

In this code, pthread_kill() is used to determine if the thread (which earlier received a pthread_cancel()) is still running or not.

The problem is, this usage is incorrect. If the thread has already terminated, then the thread_id variable effectively points to freed memory---or worse, may point to a different thread that was recently created (due to re-use of memory). Simply put, you can't use pthread_kill() to determine if a thread is still alive. Some references:

https://stackoverflow.com/questions/1693180/how-do-i-determine-if-a-pthread-is-alive

https://stackoverflow.com/questions/2156353/how-do-you-query-a-pthread-to-see-if-it-is-still-running

By the same token, you can't use pthread_kill() to send signals to a thread that may or may not be alive, so that nixes the other use of the function a few lines further down.

(It's arguable that the "Schrödinger's thread" usage should be supported, and I think that would be reasonable behavior. But Ulrich Drepper is dead-set against it, and unfortunately he calls the shots in the Linux/glibc world.)

A different strategy is going to be needed here. In researching the issue, I saw some mention of pthread cleanup handlers, which may be a possibility (e.g. have the handler clear the cnt_list[i]->running flag). As for sending SIGVTALRM, maybe use a timer and a signal handler within the thread?

[jogu]

I suspect the use of pthread_kill was an attempt to deal with v4l devices that had kernel level driver problems that caused threads to lock up. I don't think it's particularly necessary with netcams. (Though this is just a guess.)

Can any of the issues be solved if we stop detaching threads? I've never been comfortable with code that uses pthread_detach / PTHREAD_CREATE_DETACHED.

[iskunk]

What I got from investigating the issue is that joining threads when they are finished is safer inasmuch as it avoids the whole "Schrödinger's thread" race condition. Do you know why detached threads are used in the first place? Was it that v4l issue?

(Kernel-level thread lockups are a pretty serious problem to try to work around in userspace. Are those still a thing? I'd hope that's more of a historical thing, especially given how long Motion has been around.)

The code does suggest that (netcam) threads could get stuck in a network read, hence the sending of SIGVTALRM (so that syscalls exit with EINTR). So there is some relevance to the RTSP use case. (But then FFmpeg has the interrupt callback, so isn't that good enough?)

It'll sure help to know the backstory to this code, as it looks like a lot of effort was put into working around certain failure modes---without describing exactly what those failure modes were.

[tosiara]

Is this investigation still in progress?

[iskunk]

We could use some information on what scenarios necessitated the use of pthread_kill() in the first place. But barring that, my tree has been working fine going no further than pthread_cancel().

(Assuming that the original issue was bad V4L drivers, I'd certainly hope that those are in better shape today than when this code was originally written. #363 would certainly suggest as much.)

Currently, I have the pthread_kill() logic #if 0'ed out. If you like, I can clean up that and submit a proper PR that makes this change official.

[dorvan]

Thread forking management problem?

[Mr-Dave]

Closing this issue with a 'wontfix' as it does not appear to be 'fixable'. To address a few of the questions listed in this issue:

1. From what I've observed, the use of detached threads is so that we do not get into a block on the main thread. There are situations which where the pthread_cancel does not succeed and cancel the thread so doing a pthread_join on the main thread would then just block that thread as well.

2. The call to pthead_kill is required to invoke a signal and allow the pthread_cancel to de-queue and process the cancel request.

To test these scenarios, two consecutive locks were requested on the same mutex in a spawned thread. Obviously the second the lock request will hang the spawned thread which the watchdog needs to break out of.