# Possible mutex starvation issue affects all nginx Linux versions **URL:** \ **Category:** NGINX\ **Created:** [December 2, 2013, 1:16pm UTC](https://www.ruby-forum.com/t/possible-mutex-starvation-issue-affects-all-nginx-linux-versions/237817 "2013-12-02T13:16:48Z")\ **Posts on this page:** 4\ **Page:** 1
**Author:** ![Detlef\_R](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/d/58f4c7/32.png) [@Detlef\_R](https://www.ruby-forum.com/u/Detlef_R)\ **Post date:** [December 2, 2013, 1:16pm UTC](https://www.ruby-forum.com/t/possible-mutex-starvation-issue-affects-all-nginx-linux-versions/237817/1 "2013-12-02T13:16:48Z")
Here is a patch for a possible mutex starvation issue which affects all nginx Linux versions. Already solved for Windows since nginx 1.5.7.1 Caterpillar. Can be reproduced when nginx reloads the config & worker holding mutex dies or hangs. Fixed by Vittorio Francesco Digilio, commercially sponsered solution by ITPP. target source mainline 1.5.8 - 30-11-2013. src/event/ngx\_event\_accept.c, line 402 was correctly found (starvation fix) but missed in: src/event/ngx\_event.c, line 259-260, 1 line added; 255 if (ngx\_posted\_accept\_events) { 256 ngx\_event\_process\_posted(cycle, &ngx\_posted\_accept\_events); 257 } 258 259 if (ngx\_accept\_mutex\_held) { –+ ngx\_accept\_mutex\_held=0; 260 ngx\_shmtx\_unlock(&ngx\_accept\_mutex); 261 } 262 263 if (delta) { 264 ngx\_event\_expire\_timers(); 265 } Also applies to; src/os/win32/ngx\_process\_cycle.c line 507-508, 3 lines added; 504 if (ngx\_processes[n].handle != h) { 505 continue; 506 } 507 –+ if(\*ngx\_accept\_mutex.lock==ngx\_processes[n].pid) { –+ \*ngx\_accept\_mutex.lock=0; –+ } 508 if (GetExitCodeProcess(h, &code) == 0) { 509 ngx\_log\_error(NGX\_LOG\_ALERT, cycle-\>log, ngx\_errno, 510 “GetExitCodeProcess(%P) failed”, 511 ngx\_processes[n].pid); 512 } Posted at Nginx Forum: > **[\[Patch\] possible mutex starvation issue affects all nginx Linux versions.](https://forum.nginx.org/read.php?2,245121,245121#msg-245121)** > > Here is a patch for a possible mutex starvation issue which affects all nginx Linux versions. Already solved for Windows since nginx 1.5.7.1 Caterpillar. Can be reproduced when nginx reloads the config & worker holding mutex dies or hangs. Fixed... ---
**Author:** ![itpp2012](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/i/f05b48/32.png) [@itpp2012](https://www.ruby-forum.com/u/itpp2012)\ **Post date:** [December 2, 2013, 2:39pm UTC](https://www.ruby-forum.com/t/possible-mutex-starvation-issue-affects-all-nginx-linux-versions/237817/2 "2013-12-02T14:39:48Z")
Hello! On Mon, Dec 02, 2013 at 07:15:52AM -0500, itpp2012 wrote: > but missed in: > 263 if (delta) { > 264 ngx\_event\_expire\_timers(); > 265 } This patch is wrong. The ngx\_accept\_mutex\_held don’t need to be unset here. The ngx\_accept\_mutex\_held is used as an idicator that on previous iteration the lock was held by the process, and unsetting it will result in incorrect behaviour. > 509 ngx\_log\_error(NGX\_LOG\_ALERT, cycle-\>log, ngx\_errno, > 510 “GetExitCodeProcess(%P) failed”, > 511 ngx\_processes[n].pid); > 512 } This patch is also wrong. In the current state of the win32 version it is not assumed that accept mutex is used at all. But if it is, correct aproach to unlock shared memory mutexes on abnormal process termination is to port (or move to platform-independed place) the ngx\_unlock\_mutexes() function from src/os/unix/ngx\_process.c. It correctly uses ngx\_shmtx\_force\_unlock() to properly unlock shared memory mutexes using atomic operations. Note well that unlocking shared memory mutexes on abnormal process termination is an emergency mechanism. If it actually happens, it indicate that something is really wrong in other places of the system. Please also consider reading [Contributing Changes](http://nginx.org/en/docs/contributing_changes.html). – Maxim D. [http://nginx.org/en/donation.html](http://nginx.org/en/donation.html) ---
**Author:** ![itpp2012](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/i/f05b48/32.png) [@itpp2012](https://www.ruby-forum.com/u/itpp2012)\ **Post date:** [December 11, 2013, 1:58pm UTC](https://www.ruby-forum.com/t/possible-mutex-starvation-issue-affects-all-nginx-linux-versions/237817/3 "2013-12-11T13:58:05Z")
From the patch author: Hi Maxim, I apologize for my late reply: I just had now time to sort this out. The short answer to your remarks is: the first patch is partially correct, just incomplete, and could be easily completed with the addition of a call to ngx\_disable\_accept\_events(…) addressing the issue of 2 workers accepting connections at the same time. The second one is windows only and as correct and atomical as the unix only ngx\_unlock\_mutexes(), addressing the very same issue that one is for (which btw showed up during my tests) but being just one line long. The long answer follows and, well, is quite long. So before everything else, and for the sake of clarity: accept mutex patching has been performed on codebase derived by nginx 1.5.6 after properly re-enabling it (the accept\_mutex) removing the following disable lines in ngx\_event.c #if (NGX\_WIN32) ``` /* * disable accept mutex on win32 as it may cause deadlock if * grabbed by a process which can't accept connections */ ngx_use_accept_mutex = 0; ``` #endif Thus submitted patch lines are not useless and are part of a larger patch included in Catepillar 1.5.7.1. The latter (Caterpillar) fully works distributing correctly the workload to any number of configured process workers (not just one of the configured number of them). Now getting to specific proposed patches: A) about the added line ngx\_accept\_mutex\_held=0 in ngx\_event.c > 259 if (ngx\_accept\_mutex\_held) { > –+ ngx\_accept\_mutex\_held=0; > 260 ngx\_shmtx\_unlock(&ngx\_accept\_mutex); > 261 } that is not wrong, it’s just incomplete. In fact, first of all, it pairs with the similar one in src/event/ngx\_event\_accept.c, line 402 which was patched in Caterpillar and that you too found later in your in 1.5.7. The problem with this one (line 402) was that when ngx\_accept\_mutex\_held’s process _local_ variable and the accept\_mutex _global_ to all nginx processes (being it allocated in shared memory) got out of synch. This resulted (as it has been showed in tests) in more than a worker process locally ‘thinking’ to have the ownership of the accept\_mutex at the same time. The latter interfers in the call of their respective ngx\_disable\_accept\_events(…) in turn leading, because of nasty time synching, to no worker being able to enabling them anymore and amounting to all workers (so the whole server) unable to handle any further connection. 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 can be done moving that ‘if’ completely patched code if (ngx\_accept\_mutex\_held) { ngx\_disable\_accept\_events(cycle); ngx\_accept\_mutex\_held=0; ngx\_shmtx\_unlock(&ngx\_accept\_mutex); } to ngx\_event\_accept.c (being ngx\_disable\_accept\_events(…) internal linkage) in a dedicated function, for example void ngx\_release\_accept\_mutex(ngx\_cycle\_t _cycle){ /_ the if above \*/ } which then gets called at 259 in ngx\_event.c instead of the ‘if’ itself. BTW to make the patch complete the ‘if’, in ngx\_trylock\_accept\_mutex, where ngx\_disable\_accept\_events() is called should be removed since substituted by that ngx\_release\_accept\_mutex() call. All of this is to avoid a partial incorrect behaviour that the ‘if’, as it is now, > 259 if (ngx\_accept\_mutex\_held) { > 260 ngx\_shmtx\_unlock(&ngx\_accept\_mutex); > 261 } causes at the end of an iteration when the accept mutex was held: the mutex is released in the ‘if’ above but the ngx\_disable\_accept\_events(…) won’t be called until the next iteration with ngx\_trylock\_accept\_mutex(…). Thus in the meanwhile another worker could succeed to acquire the accept mutex, via ngx\_trylock\_accept\_mutex(…), and start to accept connections as well ( ngx\_enable\_accept\_events() is called in ngx\_trylock\_accept\_mutex when mutex is acquired successfully). This means that 2 workers at the same time can accept connections and this goes against the reason of an accept mutex itself reasonably not being not that the intended behaviour. B) About the second proposed patch: the 3 lines if(\*ngx\_accept\_mutex.lock==ngx\_processes[n].pid) { \*ngx\_accept\_mutex.lock=0; } make a patch to prevent server’s accept\_mutex deadlock when its owning worker process dies unexpectedly (program crash while processing a request/answer or task manager’s abrupt termination). This is a scenario that occurred several times while testing and, when showing up, lead to server’s workers unable to accept any further connection. Given that, unlike in the unix version with its ngx\_unlock\_mutexes(), this scenario is not addressed in the windows version and, as said, the above lines are meant to fix it. They are correct and thread-safe (the operation is atomic) _in the mentioned specific scenario in which they are meant to be executed_ because: 1. they are invoked by master only and just when it detects that a worker process has terminated: worker process handle gets signaled on its termination and a WaitForMultipleObjects (in os/win32/ngx\_process\_cycle.c) waiting for them wakes up 2. the if condition makes sure 2 things are true at the same time: a) the spinlock contains the pid of the dead worker and b) that implies \*ngx\_accept\_mutex.lock != 0 (since no pid could be zero) 3. considering that the accept\_mutex is only acquired (during code flow) via ngx\_uint\_t ngx\_shmtx\_trylock(ngx\_shmtx\_t \*mtx) { return (\*mtx-\>lock == 0 && ngx\_atomic\_cmp\_set(mtx-\>lock, 0, ngx\_pid)); } where #define ngx\_atomic\_cmp\_set(lock, old, set) (InterlockedCompareExchange((void \*\*) lock, (void \*) set, (void \*) old) == (void \*) old) #endif then, in the above scenario, \*mtx-\>lock == pid (of the terminated worker process) and pid !=0 imply that the ngx\_atomic\_cmp\_set(…) (so its InterlockedCompareExchange(…) ) can’t ever be called for any other process different than the (already) dead one (i.e. code can’t get to the atomic InterlockedXXX operation at all, so no atomic operation is ever executed…). 1. furthermore accept\_mutex is released (when appropriate) only via calling ngx\_shmtx\_unlock() by the owning worker process and in such scenario that process is obviously dead before having had such a chance (or the ‘if( )’ fix’s condition wouldn’t trigger) 2. no other worker can release a mutex it didn’t previously acquire (and couldn’t even if, mistakenly, tried to; just enough to look at ngx\_shmtx\_unlock() implementation to see why). This last point shows also that, always in that scenario, ngx\_shmtx\_unlock() can’t ever be called by master to release the accept mutex (not being its owner). 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. 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. I realize that for more easily readable code and for possible future extensions (more mutexes in shared memory) it would be better to port that function but as of now it doesn’t really make a difference from the correctness of program execution point of view. The patch was still unpolished for full submission, sorry for that. HTH and regards, Vittorio F Digilio Posted at Nginx Forum: > **[Re: \[Patch\] possible mutex starvation issue affects all nginx Linux versions.](https://forum.nginx.org/read.php?2,245121,245417#msg-245417)** > > Here is a patch for a possible mutex starvation issue which affects all nginx Linux versions. Already solved for Windows since nginx 1.5.7.1 Caterpillar. Can be reproduced when nginx reloads the config & worker holding mutex dies or hangs. Fixed... ---
**Author:** ![itpp2012](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/i/f05b48/32.png) [@itpp2012](https://www.ruby-forum.com/u/itpp2012)\ **Post date:** [December 11, 2013, 6:11pm UTC](https://www.ruby-forum.com/t/possible-mutex-starvation-issue-affects-all-nginx-linux-versions/237817/4 "2013-12-11T18:11:38Z")
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 D. [http://nginx.org/](http://nginx.org/)