Probable thread-safety issue in pcre2 10.43+

[lbonn]

Our application is matching elements against patterns in several concurrent threads and we had some crash reports from users who updated to recent pcre2: davatorium/rofi#1966.

After a collective effort, we tracked down a probable culprit as a regression introduced in d84f255, in particular the locking logic in the jit allocator.

From an educated guess, the fix could be to protect (again) some fields access in next_header, as these are also read in the free() function:

diff --git a/src/sljit/allocator_src/sljitExecAllocatorCore.c b/src/sljit/allocator_src/sljitExecAllocatorCore.c
index 85f3a9d..2fc91c2 100644
--- a/src/sljit/allocator_src/sljitExecAllocatorCore.c
+++ b/src/sljit/allocator_src/sljitExecAllocatorCore.c
@@ -237,12 +237,12 @@ SLJIT_API_FUNC_ATTRIBUTE void* sljit_malloc_exec(sljit_uw size)
                header->size = chunk_size;
                next_header = AS_BLOCK_HEADER(header, chunk_size);
        }
-       SLJIT_ALLOCATOR_UNLOCK();
        next_header->size = 1;
        next_header->prev_size = chunk_size;
 #ifdef SLJIT_HAS_EXECUTABLE_OFFSET
        next_header->executable_offset = executable_offset;
 #endif /* SLJIT_HAS_EXECUTABLE_OFFSET */
+       SLJIT_ALLOCATOR_UNLOCK();
        return MEM_START(header);
 }

It stopped crashing after this change for at least one of the testers.

CC: @christian-heusel, @DaveDavenport, @ravomavain, @dennisschagt

[zherczeg]

Just the other day I was also wondering why the unlock mas moved before the end of the function. It just felt wrong. @carenas do you remember?

zherczeg/sljit@7de0fee#diff-249fa9ceb0eb2b074dbaa38dfca020158e15e36e8a7b5d0aca1443226bf4b63aR243

[carenas]

It just felt wrong. @carenas do you remember?

It was likely unintentional and therefore a bug

[easyteacher]

https://bugs.kde.org/show_bug.cgi?id=484514 has a similar backtrace

[lbonn]

@zherczeg @carenas should I open a PR to sljit and pcre2 with this small change or do you prefer to handle it yourself?

A fair number of users seem affected and the bug might have security implications as it causes memory corruption.

[zherczeg]

Up to you. Let me know your decision.

[lbonn]

@zherczeg thanks for the very quick merge :)

[lbonn]

Fix is now in the master branch e466d41, thank you!

[dangelog]

Hi! Do you think this warrants a release, given the bug causes crashes?

[PhilipHazel]

Probably fairly soon, but I am currently chasing down an obscure bug that oss-fuzz found and it would be nice to have that cleared up as well.