[En-Nut-Discussion] Race condition and stack overflow in tcpsm.c

[Philipp Burch]

Hi Harald!

On 19.11.2014 14:54, Harald Kipp wrote:
> Hi Philipp,
> 
> On 14.11.2014 10:33, Philipp Burch wrote:
>> +#include <sys/mutex.h>
> 
> I agree that the problem exists, but I do not agree with the way you
> tried to fix this.
> 
> MUTEX is application candy, which means, that it is possibly used by
> some existing code, but not in the kernel. Your change would add quite
> some extra bytes for the rest by creating a reference to nut/os/mutex.c.

Agreed, I didn't think about that.

> 
> http://lists.egnite.de/pipermail/en-nut-discussion/2009-July/025199.html
> 
> While Nut/OS is cooperative, mutual exclusion can be done with much less
> effort:
> 
> int NutTcpInitStateMachine(void)
> {
>  static int_fast8_t initialized;
>  if (!initialized) {
>   initialized = 1;
>   ...
>   Code for initialization
>   ...
>  }
>  return whatever;
> }

I'm aware of the cooperative nature of Nut/OS and the benefits of that,
but I think that this solution won't be ideal here. The problem only
arises if two threads try to initialize the TCP state machine "at the
same time". When the first thread enters the function, it will set the
flag to 1 indicating that the initialization is complete, which is
actually a lie. As it cannot be interrupted by another thread, it will
then start the NutTcpSm thread *and switch to it* (as it has higher
priority). At this time, it is no longer guaranteed that the same thread
will be scheduled immediately afterwards, so it is possible that another
thread comes to life and sees that the state machine isn't ready yet. It
then calls NutTcpInitStateMachine() again but immediately returns, as
the initialized flag is set already. What isn't set yet, however, is the
tcpThread variable, as NutThreadCreate() of the first call not yet
returned. So if another thread should try to access this variable for
any reason, it would dereference a NULL pointer.

It is possible that this will not cause a problem, if no code except
NutTcpInitStateMachine() in tcpsm.c (or somewhere else) ever directly
accesses the tcpThread variable. I haven't checked that yet. A solution
(or better: a workaround) for this would probably be to set the
tcpThread right at the start of THREAD(NutTcpSm, arg) to runningThread,
so it will be set before NutThreadCreate actually returns.

Thanks and best regards,
Philipp