[vdr] childTid thread safe?

[s.huelswitt@gmx.de (Stefan Huelswitt)]

On 31 Oct 2004 Klaus Schmidinger <Klaus.Schmidinger@cadsoft.de> wrote:

> Stefan Huelswitt wrote:
>> 
>> On 24 Oct 2004 Klaus Schmidinger <Klaus.Schmidinger@cadsoft.de> wrote:
>> 
>> > - Added some checks when cancelling a thread and removed the usleep() in
>> >   cThread::Start() (suggested by Ludwig Nussel). Also removed 'running' from
>> >   cThread and using only childTid to indicate whether a thread is actually
>> >   running.
>> 
>> Although the changes improves a lot, I think the code isn't 100%
>> tight. childTid is accessed from 2 threads and should be
>> protected by a mutex to be save.
>> 
>> Should I prepare a patch?
> 
> I don't think that this is necessary.
> childTid is an integral variable and thus read or written "as a whole".
> I don't see any danger here - but if you can come up with a scenario
> where there could be a problem, I might stand corrected.

The problem is not the actual access to childTid, which of course
is atomic.

When cThread::Action() exits, the childTid is set to NULL in
cThread::StartThread() (in context of child thread).
While in cThread::Cancel() (which normaly is executed in some
other context e.g. parent thread) we have:

if(childTid) {
  pthread_cancel(childTid)
  ...

In a race situation StartThread() could reset childTid inbetween
the check and the cancel.
There are other similiar places earlier in cThread::Cancel(), in
cThread::Action() and cThread::Start().

To be save, we would have to mutex lock/unlock around the
childTid accesses.

Two more things:

There is a race in reseting the childTid in StartThread(): if the
thread uses pthread_exit() the childTid is never reset.

The childTid is set twice: once in Start() and once in
StartThread().

Regards.

-- 
Stefan Huelswitt
s.huelswitt@gmx.de  | http://www.muempf.de/