Fixed race condition that occurs when initializing the executable_allocator_is_working variable in the pcre2_jit_compile function

[larinsv]

The error fixed by this PR

If pcre2_jit_compiler is called simultaneously for the first time from 2 (or more) threads, then one of the calls may fail with the PCRE2_ERROR_NOMEMORY error.

[zherczeg]

The point of this check is checking once that the allocator is working. If it does not, it disables jit compiling. It can be run in multiple threads. Of course the allocator may fail to allocate memory later, but at least it is working, so there is a chance it can do it.

[larinsv]

The logic you described is absolutely clear to me.

I'll try to explain the error.

Parallel execution of 2 threads can be like this:

1. variable executable_allocator_is_working == 0
2. 1st thread executes if (executable_allocator_is_working == 0)
3. 2nd thread executes if (executable_allocator_is_working == 0)
4. 1st thread executes void *ptr = SLJIT_MALLOC_EXEC(32, NULL);
5. 2nd thread starts execute void *ptr = SLJIT_MALLOC_EXEC(32, NULL);
6. 1st thread executes if (ptr != NULL), SLJIT_FREE_EXEC(((sljit_u8*)(ptr)) + SLJIT_EXEC_OFFSET(ptr), NULL);, executable_allocator_is_working = 1; and pause a little
   variable executable_allocator_is_working == 1
7. 2nd thread completes the execution of void *ptr = SLJIT_MALLOC_EXEC(32, NULL);
8. 2nd thread executes executable_allocator_is_working = -1;
   variable executable_allocator_is_working == -1
9. 1st thread executes if (executable_allocator_is_working < 0)
   But the value of executable_allocator_is_working is now -1,
   so 1st thread executes return PCRE2_ERROR_NOMEMORY; - ERROR!

[carenas]

wouldn't it then be better to make the assignment of executable_allocator_is_working as part of the else?

if (ptr != NULL)
    {
    SLJIT_FREE_EXEC(((sljit_u8*)(ptr)) + SLJIT_EXEC_OFFSET(ptr), NULL);
    executable_allocator_is_working = 1;
    }
else executable_allocator_is_working = -1;

as documented around lines 14362 to 14365, 2 different threads should follow the same branch regardless. maybe changing the wording of the last sentence might be needed to make that more clear.

there is always a possible race condition, but any application that gets PCRE2_ERROR_NOMEMORY from this function (under this check) is expected to disable JIT, since the only way that would be triggered, is because the system is no longer allowing RWX pages (ex: SELinux enabled) and the alternative is to segfault.

[larinsv]

Most likely this will also work correctly:

if (executable_allocator_is_working == 0)
  {
  /* Checks whether the executable allocator is working. This check
     might run multiple times in multi-threaded environments, but the
     result should not be affected by it. */
  void *ptr = SLJIT_MALLOC_EXEC(32, NULL);
  if (ptr != NULL)
    {
    SLJIT_FREE_EXEC(((sljit_u8*)(ptr)) + SLJIT_EXEC_OFFSET(ptr), NULL);
    executable_allocator_is_working = 1;
    }
  else executable_allocator_is_working = -1;
  }

if (executable_allocator_is_working < 0)
  return PCRE2_ERROR_NOMEMORY;

Which implementation will we use?

[larinsv]

The first version has the advantage that there is only one place where the value of the executable_allocator_is_working variable is set.
But it's not very important.

[carenas]

I prefer the later, but since you are already touching it might as well change it to so it is a more common tri state (-1 = unknown, 0 = false, 1 = true), which will also clean the last check so it will be instead the more reasonable :

if (!executable_alllocator_is_working)
  return PCRE2_ERROR_NOMEMORY;

[larinsv]

Do I understand correctly what you are suggesting to write like this?

static int executable_allocator_is_working = -1;

...

if (executable_allocator_is_working == -1)
  {
  /* Checks whether the executable allocator is working. This check
     might run multiple times in multi-threaded environments, but the
     result should not be affected by it. */
  void *ptr = SLJIT_MALLOC_EXEC(32, NULL);
  if (ptr != NULL)
    {
    SLJIT_FREE_EXEC(((sljit_u8*)(ptr)) + SLJIT_EXEC_OFFSET(ptr), NULL);
    executable_allocator_is_working = 1;
    }
  else executable_allocator_is_working = 0;
  }

if (!executable_allocator_is_working)
  return PCRE2_ERROR_NOMEMORY;

[larinsv]

done

[zherczeg]

Maybe you could use a switch:

switch (executable_allocator_is_working)
  {
  case 0:
    // Check the allocator. Set -1 and return with PCRE2_ERROR_NOMEMORY if it does not work at all.
  case 1:
    break;
  default:
    return PCRE2_ERROR_NOMEMORY;
  }

[carenas]

An alternative (WIP) provided in the external branch, which has the following advantages:

• consolidate the code, so it is easier to mantain and adapt as more systems implement their W^X restrictions
• extend the check to calls to pcre2_config so the answer to (is JIT available) is consistent with this, and which is also likely something that will be done once per process, therefore reducing the likelyhood of a race.

since it also expands the PCRE2 public API, this could be added to the guidelines and suggest doing it single threaded so it will be thread safe later when queried and without incurring in the costs of having to maintain a lock

[larinsv]

I rebased this PR branch on current master

[zherczeg]

Is there a problem with 0 value? Normally this can to .bss instead of initialized data.

[carenas]

the explicity/implicit initialization is not the problem here, I think this code change cames from the point of view of making the code easier to reason with by instead moving to a tri-state boolean type where the "unknown" state is -1 instead of 0

[zherczeg]

If clarify is that important for this code, enums should be used. I don't think a -1 is better than 0 to understand this code.

[carenas]

I disagree though, using -1 makes it simpler to write a test that says "fail with error if we either don't know or cannot allocate memory" by doing below:

if (!executable_allocator_is_working)
  return PCRE2_ERROR_NOMEMORY;

I have no strong opinions on either implementations though, but I think I happen to know the guy that wrote this code originally, and his choice of 0 for "dunno" and -1 for "no" came from being super lazy and trying to reuse the error response from the underlying functions to signal "no", and not having to initialize the static for "dunno"., and since we are touching this code, it might be worth changing it now as it is being inconsistently used that way in the codebase and is a little strange and unexpected when looking elsewhere in other codebases.

[zherczeg]

LGTM