Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
May 18, 2022
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions src/pcre2_jit_compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14384,7 +14384,7 @@ pcre2_jit_compile(pcre2_code *code, uint32_t options)
pcre2_real_code *re = (pcre2_real_code *)code;
#ifdef SUPPORT_JIT
executable_functions *functions;
static int executable_allocator_is_working = 0;
static int executable_allocator_is_working = -1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#endif

if (code == NULL)
Expand Down Expand Up @@ -14447,23 +14447,21 @@ return PCRE2_ERROR_JIT_BADOPTION;

if ((re->flags & PCRE2_NOJIT) != 0) return 0;

if (executable_allocator_is_working == 0)
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);

executable_allocator_is_working = -1;

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 < 0)
if (!executable_allocator_is_working)
return PCRE2_ERROR_NOMEMORY;

if ((re->overall_options & PCRE2_MATCH_INVALID_UTF) != 0)
Expand Down