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.

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

Avoid deadlock with PERL_MEM_LOG #18359

Merged
merged 1 commit into from Nov 27, 2020
Merged

Avoid deadlock with PERL_MEM_LOG #18359

merged 1 commit into from Nov 27, 2020

Conversation

khwilliamson
Copy link
Contributor

@khwilliamson khwilliamson commented Nov 26, 2020

 Avoid deadlock with PERL_MEM_LOG
 
 This fixes GH #18341
 
 The Perl wrapper for getenv() was changed in 5.32 to allocate memory to
 squirrel safely away the result of the wrapped getenv() call.  It does
 this while in a critical section so as to make sure another thread can't
 interrupt it and destroy it.
 
 Unfortunately, when Perl is compiled for debugging memory problems and
 has PERL_MEM_LOG enabled, that allocation causes a recursive call to
 getenv() for the purpose of checking an environment variable to see how
 to log that allocation.  And hence it deadlocks trying to enter the
 critical section.
 
 There are various solutions.  One is to use or emulate a general semaphore
 instead of a binary one.  This is effectively what
 PL_lc_numeric_mutex_depth does for another mutex, and the code for that
 could be used as a template.
 
 But given that this is an extreme edge case which requires Perl to be
 specially compiled to enable this feature which is used only for
 debugging, a much simpler, if less safe if it were to ever be used in
 production, solution should suffice.  Tony Cook suggested just avoiding
 the wrapper for this particular purpose.

util.c Outdated
@@ -5008,6 +5008,9 @@ S_mem_log_common(enum mem_log_type mlt, const UV n,

PERL_ARGS_ASSERT_MEM_LOG_COMMON;

/* Note that 'pmlenv' is not protected from other threads overwriting it by
* calling getenv() themselves on platforms where getenv() returns an
* internal static pointer. */
pmlenv = PerlEnv_getenv("PERL_MEM_LOG");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just call getenv() here? That's what happens anyway, and doesn't cost every other Perl_mortal_getenv() the (minor) cost of a string comparison.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a much better solution than mine. Updated to do what you say.

This fixes GH #18341

The Perl wrapper for getenv() was changed in 5.32 to allocate memory to
squirrel safely away the result of the wrapped getenv() call.  It does
this while in a critical section so as to make sure another thread can't
interrupt it and destroy it.

Unfortunately, when Perl is compiled for debugging memory problems and
has PERL_MEM_LOG enabled, that allocation causes a recursive call to
getenv() for the purpose of checking an environment variable to see how
to log that allocation.  And hence it deadlocks trying to enter the
critical section.

There are various solutions.  One is to use or emulate a general semaphore
instead of a binary one.  This is effectively what
PL_lc_numeric_mutex_depth does for another mutex, and the code for that
could be used as a template.

But given that this is an extreme edge case which requires Perl to be
specially compiled to enable this feature which is used only for
debugging, a much simpler, if less safe if it were to ever be used in
production, solution should suffice.  Tony Cook suggested just avoiding
the wrapper for this particular purpose.
@khwilliamson khwilliamson merged commit 0cc28fe into blead Nov 27, 2020
@khwilliamson khwilliamson deleted the smoke-me/khw-memlog branch November 27, 2020 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants