[Patch] possible mutex starvation issue affects all nginx Linux versions.

[Maxim Dounin]

Hello!

On Wed, Dec 11, 2013 at 07:57:32AM -0500, itpp2012 wrote:

[...]

> A) about the added line ngx_accept_mutex_held=0 in ngx_event.c

[...]

> The line above, between 259 and 260, tried to fix this too, but, there, a
> call to ngx_disable_accept_events(...) is missing.
> In fact to be completely correct and not resulting in partially incorrect
> behaviour as you correctly pointed out, such a call must be added too.

This is wrong as well.  There is no reason to disable accept 
events till we are sure that we wasn't able to re-aquire accept 
mutex before going into the kernel to wait for events.  It's just 
waste of resources.

You are misunderstanding the code, probably becase the 
ngx_accept_mutex_held variable has somewhat misleading name.  It 
is not expected to mean "we have the accept mutex locked", it means 
"we had the accept mutex locked on previous iteration, we have to 
disable accept events if we won't be able to re-aquire it".

There is no need to fix it, it's not broken.

[...]

> B) About the second proposed patch:

[...]

> Hopefully at this point it should be clear enough that releasing the
> accept_mutex directly with
> 
> *ngx_accept_mutex.lock=0;
> 
> is as much safe as calling ngx_shmtx_force_unlock(), the choice of which one
> I just deem cosmetic (they're functionally equivalent): my choice then went
> for the more performant, straight variable assignment.

Even considering the code currently there for win32 version, there 
is no guarantee that reading *ngx_accept_mutex.lock is atomic.  
While usually it is, in theory it can be non-atomic, and the code 
will start doing wrong things due to the if() check unexpectedly 
succeeding.

> NOt wanting to make this post longer than it is I conclude just mentioning
> that, as of now, the accept mutex is the only mutex living in shared memory
> so unlocking other possibly shared mutexes is as well unrequired.

That's not true.  Something like

$ grep -r shmtx_lock src/

will give you an idea where shared memory mutexes are used in 
nginx.

While it's tricky to get all of this working on modern Windows 
versions with ASLR, the accept mutex is certainly not the only 
shared memory mutex used in nginx.

As I already wrote, I don't object adding an unlock here, but I 
would like to see the code which is correct and in-line with the 
unix version of the code.

-- 
Maxim Dounin
http://nginx.org/