Re: [Libjit] [PATCH] windows: fix race condition in _jit_thread_init()

[Aleksey Demakov]

Hi Ben,

Thanks for the patch. I believe there is no need for
InterlockedCompareExchange in the case 1. It should be possible to
spin with something like this:

while (once_control != 2)
{
  _ReadWriteBarrier();
}

I don't have a win32 box handy to test this, could you please give it a try?

Thanks,
Aleksey


On Tue, Jan 21, 2014 at 2:26 AM, Ben Noordhuis <address@hidden> wrote:
> On POSIX platforms, libjit uses a pthread_once_t guard for its one-time
> initialization logic.
>
> pthread_once() guarantees that in the contended case, one thread gets
> to run the initialization callback while the other threads are blocked
> until the callback returns.
>
> The Windows initialization code currently has no such guarantees.
> It guards against invoking init_win32_thread() twice but it does
> nothing to stop other threads from returning prematurely.
>
> That's bad because it means the global mutex and the thread-local
> storage may not have been initialized when _jit_thread_init() returns.
>
> This commit addresses that with an ersatz spinlock.  Spinning is not
> very nice but the alternatives are arguably worse: INIT_ONCE is not
> available on Windows XP while CreateEvent() can fail in low memory
> situations.
>
> Caveat emptor: I have not actually tested the patch.  I'm reasonably
> confident that it works but it's probably best to view it as a bug
> report with suggested fix attached. ;-)
>
> Signed-off-by: Ben Noordhuis <address@hidden>
> ---
>  ChangeLog        |  5 +++++
>  jit/jit-thread.c | 12 ++++++++++--
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index a0b6bbb..ff6ec4a 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-01-20  Ben Noordhuis  <address@hidden>
> +
> +       * jit/jit-thread.c: Fix Windows-only race condition in
> +       _jit_thread_init().
> +
>  2013-10-17  Aleksey Demakov  <address@hidden>
>
>         * README, doc/libjit.texi: Update info on obtaining libjit sources.
> diff --git a/jit/jit-thread.c b/jit/jit-thread.c
> index da3aae6..31ea8ba 100644
> --- a/jit/jit-thread.c
> +++ b/jit/jit-thread.c
> @@ -103,9 +103,17 @@ void _jit_thread_init(void)
>         pthread_once(&once_control, init_pthread);
>  #elif defined(JIT_THREADS_WIN32)
>         static LONG volatile once_control = 0;
> -       if(!InterlockedExchange((PLONG)&once_control, 1))
> +       switch(InterlockedCompareExchange(&once_control, 1, 0))
>         {
> -               init_win32_thread();
> +               case 0:
> +                       init_win32_thread();
> +                       InterlockedExchange(&once_control, 2);
> +                       break;
> +               case 1:
> +                       while(InterlockedCompareExchange(&once_control, 2, 2) 
> != 2)
> +                       {
> +                       }
> +                       break;
>         }
>  #endif
>  }
> --
> 1.8.4.2
>
>