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
Coredump in call_sv under threads #15229
Comments
From @dur-randirCreated by @dur-randir%cat t/80-threads.t my @threads = map +threads->create(sub { for (1..5_000) { $_->join for splice @threads; LC_MESSAGES="en_US.UTF-8" perl t/80-threads.t Stack traces are following. (lldb) t 1 (lldb) t 2 (lldb) t 3 Both t 2 and t 3 are on the following line: (lldb) f 2 Perl Info
|
From @tonycozOn Mon Mar 14 05:24:12 2016, randir wrote:
I suspect we have two bugs here: a) setlocale() isn't re-entrant, and we call it from multiple threads without any sort of guard b) setlocale() sets the process-wide locale, so we have possible race conditions between threads. The first could be fixed by guarding use of setlocale() with a mutex, but the second is more complex to fix. On system where they're supported we could use newlocale(), uselocale(), freelocale() etc, but we still support systems that don't have those functions and never will. Tony |
The RT System itself - Status changed from 'new' to 'open' |
From @khwilliamsonOn Mon Mar 14 16:11:44 2016, tonyc wrote:
I have tried to reproduce this on a Darwin Kernel Version 15.3.0, and can't. The particular machine doesn't have LC_MESSAGES translated into other than English, and that could explain it. There really is only one LC_MESSAGES locale on our machine. You can easily see if yours are translated, for example with LC_MESSAGES=de_DE.UTF-8 ./perl -le 'use locale; and see if English or German comes out. I have written some code that hopefully fixes this problem. It is available here: http://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/khw-regcomp If it's possible, could you compile and try this to see if your problem is solved by it. |
From @khwilliamsonOn Fri Mar 18 22:03:37 2016, khw wrote:
The current state of affairs is that the messages are displayed in English on the OP's machine, so my speculation that was behind the difference in results on the Darwin machine available to me, was wrong. However, I have experimental code that uses mutexes in the area that was failing, and there have been no failures on that. I also tried using the thread-only locale functions, but we got failures with those even so. I do not know why at this point. The whole design of perl locale handling can be considered flawed in this regard, but this is the only failure anyone has ever reported like this. This area was changed in 5.22, and now it's showing up. But it was based on how perl was intended to operate in other areas. Some of that was broken, and did not operate as intended, and that could avoid this bug. But the code was changed to operate as intended, and this was done before 5.22. And there have been no reports of failure. |
From @tonycozOn Sat Mar 19 22:20:16 2016, khw wrote:
I managed to reproduce the crash on the VM: grannysmith:perl tony$ ulimit -c unlimited Tony |
From @tonycozOn Sat Mar 19 22:20:16 2016, khw wrote:
The current code in khw-locale has a race, at least in Perl_my_strerror(): #ifdef USE_LOCALE_MESSAGES SAVE_AND_SWITCH_LOCALE(LC_MESSAGES, save_locale, "C"); /* This points to the static space in Strerror, with all its RESTORE_LOCALE; return errstr; Here you're fetching the current locale without holding the mutex, this means that another thread could be in the process of changing the locale while we're calling setlocale(). This showed as a bad save_local value (in one case "enDIE__" followed by more junk) and a failed call to setlocale() when resetting the locale. Also, if we setlocale() to an external locale, it's possible the string returned by strerror() could be released or overwritten when we switch locale back to "C", so it should probably savepv() the result (and SAVEFREEPV() to free it). If the croaks() in the macros are caught it's possible for the mutex to remain locked, leading to horrible things happening (which I haven't tried to fix.) Tony |
From @tonycoz0001-fix-some-races-in-the-new-locale-code.patchFrom 0a75df19a87b13915b2538b3d25bcff881715270 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Mon, 21 Mar 2016 11:33:28 +1100
Subject: fix some races in the new locale code
---
locale.c | 36 ++++++++++++++++++++++++++----------
1 file changed, 26 insertions(+), 10 deletions(-)
diff --git a/locale.c b/locale.c
index 1bb3687..d000d93 100644
--- a/locale.c
+++ b/locale.c
@@ -67,6 +67,9 @@
freelocale(LoCaLe_ObJeCt); \
} STMT_END
+#define LOCALE_LOCK
+#define LOCALE_UNLOCK
+
#else
#define LOCALE_LOCK MUTEX_LOCK(&PL_locale_mutex)
@@ -79,16 +82,14 @@
STMT_START { \
SaVe_LoCaLe = savepv(old_locale); \
CaTeGoRy = category; \
- LOCALE_LOCK; \
if (! setlocale(category, new_locale)) Perl_croak(aTHX_ \
- "panic: setlocale failed; errno=%d", errno); \
+ "panic: setlocale(%d, %s) failed; errno=%d", category, new_locale, errno); \
} STMT_END
# define RESTORE_LOCALE \
STMT_START { \
if (! setlocale(CaTeGoRy, SaVe_LoCaLe)) Perl_croak(aTHX_ \
- "panic: setlocale failed; errno=%d", errno); \
- LOCALE_UNLOCK; \
+ "panic: setlocale(%d, %s) failed; errno=%d", CaTeGoRy, SaVe_LoCaLe, errno); \
Safefree(SaVe_LoCaLe); \
} STMT_END
@@ -447,12 +448,14 @@ Perl_new_ctype(pTHX_ const char *newctype)
if (IN_LC(LC_CTYPE)) {
DECLARATION_FOR_SAVE_AND_SWITCH_LOCALE;
+ LOCALE_LOCK;
SAVE_AND_SWITCH_LOCALE(LC_CTYPE, newctype, "C");
/* The '0' below suppresses a bogus gcc compiler warning */
Perl_warner(aTHX_ packWARN(WARN_LOCALE), SvPVX(PL_warn_locale), 0);
RESTORE_LOCALE;
+ LOCALE_UNLOCK;
SvREFCNT_dec_NN(PL_warn_locale);
PL_warn_locale = NULL;
}
@@ -1858,21 +1861,34 @@ Perl_my_strerror(pTHX_ const int errnum) {
#ifdef USE_LOCALE_MESSAGES
if (! IN_LC(LC_MESSAGES)) {
- char * save_locale = setlocale(LC_MESSAGES, NULL);
+ char * save_locale;
+ char *errstr;
+
+ LOCALE_LOCK;
+
+ save_locale = setlocale(LC_MESSAGES, NULL);
if (! isNAME_C_OR_POSIX(save_locale)) {
DECLARATION_FOR_SAVE_AND_SWITCH_LOCALE;
- char *errstr;
SAVE_AND_SWITCH_LOCALE(LC_MESSAGES, save_locale, "C");
/* This points to the static space in Strerror, with all its
- * limitations */
- errstr = Strerror(errnum);
+ * limitations.
+ * Or in a loaded locale, into something that might go away
+ * when we switch the locale back */
+ errstr = savepv(Strerror(errnum));
RESTORE_LOCALE;
-
- return errstr;
}
+ else {
+ errstr = savepv(Strerror(errnum));
+ }
+
+ LOCALE_UNLOCK;
+
+ SAVEFREEPV(errstr);
+
+ return errstr;
}
#endif
--
2.5.4 (Apple Git-61)
|
From @tonycozOn Fri Mar 18 22:03:37 2016, khw wrote:
With the my_strerror() fixes and changing to xlocale.h (probe patch attached) it seems to work for me. Tony |
From @tonycoz0002-add-d_duplocale-and-i_locale-Configure-probes.patchFrom c0a1b6c30e2074ad7fef7bb3f2bdeab5ebb92b2d Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Mon, 21 Mar 2016 12:12:58 +1100
Subject: add d_duplocale and i_locale Configure probes
---
Configure | 12 ++++++++++++
Cross/config.sh-arm-linux | 2 ++
NetWare/config.wc | 2 ++
Porting/Glossary | 9 +++++++++
Porting/config.sh | 2 ++
config_h.SH | 11 +++++++++++
configure.com | 2 ++
plan9/config_sh.sample | 2 ++
symbian/config.sh | 2 ++
uconfig.h | 15 +++++++++++++--
uconfig.sh | 2 ++
uconfig64.sh | 2 ++
win32/config.ce | 2 ++
win32/config.gc | 2 ++
win32/config.vc | 2 ++
15 files changed, 67 insertions(+), 2 deletions(-)
diff --git a/Configure b/Configure
index f8d4abd..06d3dbe 100755
--- a/Configure
+++ b/Configure
@@ -443,6 +443,7 @@ d_drand48_r=''
drand48_r_proto=''
d_drand48proto=''
d_dup2=''
+d_duplocale=''
d_eaccess=''
d_endgrent=''
d_endgrent_r=''
@@ -1073,6 +1074,7 @@ i_stdarg=''
i_varargs=''
i_varhdr=''
i_vfork=''
+i_xlocale=''
d_inc_version_list=''
inc_version_list=''
inc_version_list_init=''
@@ -16621,6 +16623,10 @@ eval $inlibc
set nearbyint d_nearbyint
eval $inlibc
+: see if this is an xlocale.h system
+set xlocale.h i_xlocale
+eval $inhdr
+
: see if newlocale exists
set newlocale d_newlocale
eval $inlibc
@@ -16633,6 +16639,10 @@ eval $inlibc
set uselocale d_uselocale
eval $inlibc
+: see if duplocale exists
+set duplocale d_duplocale
+eval $inlibc
+
: see if nextafter exists
set nextafter d_nextafter
eval $inlibc
@@ -24303,6 +24313,7 @@ d_dosuid='$d_dosuid'
d_drand48_r='$d_drand48_r'
d_drand48proto='$d_drand48proto'
d_dup2='$d_dup2'
+d_duplocale='$d_duplocale'
d_eaccess='$d_eaccess'
d_endgrent='$d_endgrent'
d_endgrent_r='$d_endgrent_r'
@@ -24942,6 +24953,7 @@ i_values='$i_values'
i_varargs='$i_varargs'
i_varhdr='$i_varhdr'
i_vfork='$i_vfork'
+i_xlocale='$i_xlocale'
ignore_versioned_solibs='$ignore_versioned_solibs'
inc_version_list='$inc_version_list'
inc_version_list_init='$inc_version_list_init'
diff --git a/Cross/config.sh-arm-linux b/Cross/config.sh-arm-linux
index 1a4718b..3a719d3 100644
--- a/Cross/config.sh-arm-linux
+++ b/Cross/config.sh-arm-linux
@@ -172,6 +172,7 @@ d_dosuid='undef'
d_drand48_r='undef'
d_drand48proto='define'
d_dup2='define'
+d_duplocale='undef'
d_eaccess='undef'
d_endgrent='define'
d_endgrent_r='undef'
@@ -802,6 +803,7 @@ i_values='define'
i_varargs='undef'
i_varhdr='stdarg.h'
i_vfork='undef'
+i_xlocale='undef'
ignore_versioned_solibs='y'
inc_version_list=' '
inc_version_list_init='0'
diff --git a/NetWare/config.wc b/NetWare/config.wc
index bf8dc11..f0ac374 100644
--- a/NetWare/config.wc
+++ b/NetWare/config.wc
@@ -159,6 +159,7 @@ d_dosuid='undef'
d_drand48_r='undef'
d_drand48proto='undef'
d_dup2='define'
+d_duplocale='undef'
d_eaccess='undef'
d_endgrent='undef'
d_endgrent_r='undef'
@@ -784,6 +785,7 @@ i_values='undef'
i_varargs='undef'
i_varhdr='varargs.h'
i_vfork='undef'
+i_xlocale='undef'
ignore_versioned_solibs=''
inc_version_list=''
inc_version_list_init='0'
diff --git a/Porting/Glossary b/Porting/Glossary
index 4ff252f..316e195 100644
--- a/Porting/Glossary
+++ b/Porting/Glossary
@@ -714,6 +714,11 @@ d_dup2 (d_dup2.U):
This variable conditionally defines HAS_DUP2 if dup2() is
available to duplicate file descriptors.
+d_duplocale (d_newlocale.U):
+ This variable conditionally defines the HAS_DUPLOCALE symbol, which
+ indicates to the C program that the duplocale() routine is available
+ to duplicate a locale object.
+
d_eaccess (d_eaccess.U):
This variable conditionally defines the HAS_EACCESS symbol, which
indicates to the C program that the eaccess() routine is available.
@@ -3692,6 +3697,10 @@ i_vfork (i_vfork.U):
This variable conditionally defines the I_VFORK symbol, and indicates
whether a C program should include vfork.h.
+i_xlocale (newlocale.U):
+ This symbol, if defined, indicates to the C program that it should
+ include <xlocale.h> to get uselocale() and it's friends
+
ignore_versioned_solibs (libs.U):
This variable should be non-empty if non-versioned shared
libraries (libfoo.so.x.y) are to be ignored (because they
diff --git a/Porting/config.sh b/Porting/config.sh
index b123dc3..eec626a 100644
--- a/Porting/config.sh
+++ b/Porting/config.sh
@@ -181,6 +181,7 @@ d_dosuid='undef'
d_drand48_r='undef'
d_drand48proto='define'
d_dup2='define'
+d_duplocale='undef'
d_eaccess='define'
d_endgrent='define'
d_endgrent_r='undef'
@@ -817,6 +818,7 @@ i_values='define'
i_varargs='undef'
i_varhdr='stdarg.h'
i_vfork='undef'
+i_xlocale='undef'
ignore_versioned_solibs='y'
inc_version_list=''
inc_version_list_init='0'
diff --git a/config_h.SH b/config_h.SH
index 532238e..74f18c4 100755
--- a/config_h.SH
+++ b/config_h.SH
@@ -4163,9 +4163,20 @@ sed <<!GROK!THIS! >$CONFIG_H -e 's!^#undef\(.*/\)\*!/\*#define\1 \*!' -e 's!^#un
* This symbol, if defined, indicates that the uselocale routine is
* available to set the current locale for the calling thread.
*/
+/* HAS_DUPLOCALE:
+ * This symbol, if defined, indicates that the duplocale routine is
+ * available to duplicate a locale object.
+ */
#$d_newlocale HAS_NEWLOCALE /**/
#$d_freelocale HAS_FREELOCALE /**/
#$d_uselocale HAS_USELOCALE /**/
+#$d_duplocale HAS_DUPLOCALE /**/
+
+ /* I_XLOCALE:
+ * This symbol, if defined, indicates to the C program that it should
+ * include <xlocale.h> to get uselocale() and it's friends
+ */
+#$i_xlocale I_XLOCALE /**/
/* HAS_NEXTAFTER:
* This symbol, if defined, indicates that the nextafter routine is
diff --git a/configure.com b/configure.com
index 0128c64..2478057 100644
--- a/configure.com
+++ b/configure.com
@@ -6576,6 +6576,7 @@ $ WC "i_values='undef'"
$ WC "i_varargs='undef'"
$ WC "i_varhdr='stdarg.h'"
$ WC "i_vfork='undef'"
+$ WC "i_xlocale='undef'"
$ WC "inc_version_list='0'"
$ WC "inc_version_list_init='0'"
$ WC "installarchlib='" + installarchlib + "'"
@@ -6860,6 +6861,7 @@ $ ENDIF
$ WC "d_crypt_r='undef'"
$ WC "d_ctermid_r='undef'"
$ WC "d_drand48_r='undef'"
+$ WC "d_duplocale='undef'"
$ WC "d_endgrent_r='undef'"
$ WC "d_endhostent_r='undef'"
$ WC "d_endnetent_r='undef'"
diff --git a/plan9/config_sh.sample b/plan9/config_sh.sample
index a0b0bf3..2e14bf4 100644
--- a/plan9/config_sh.sample
+++ b/plan9/config_sh.sample
@@ -172,6 +172,7 @@ d_dosuid='undef'
d_drand48_r='undef'
d_drand48proto='undef'
d_dup2='define'
+d_duplocale='undef'
d_eaccess='undef'
d_endgrent='define'
d_endgrent_r='undef'
@@ -796,6 +797,7 @@ i_values='undef'
i_varargs='undef'
i_varhdr='stdarg.h'
i_vfork='undef'
+i_xlocale='undef'
ignore_versioned_solibs=''
inc_version_list=' '
inc_version_list_init='0'
diff --git a/symbian/config.sh b/symbian/config.sh
index 603ef44..459e7e0 100644
--- a/symbian/config.sh
+++ b/symbian/config.sh
@@ -116,6 +116,7 @@ d_dosuid='undef'
d_drand48_r='undef'
d_drand48proto='undef'
d_dup2='undef'
+d_duplocale='undef'
d_eaccess='undef'
d_endgrent='undef'
d_endgrent_r='undef'
@@ -723,6 +724,7 @@ i_values='undef'
i_varargs='undef'
i_varhdr='stdarg.h'
i_vfork='undef'
+i_xlocale='undef'
ignore_versioned_solibs='y'
inc_version_list=''
inc_version_list_init='0'
diff --git a/uconfig.h b/uconfig.h
index f87fb1a..2418a53 100644
--- a/uconfig.h
+++ b/uconfig.h
@@ -4128,9 +4128,20 @@
* This symbol, if defined, indicates that the uselocale routine is
* available to set the current locale for the calling thread.
*/
+/* HAS_DUPLOCALE:
+ * This symbol, if defined, indicates that the duplocale routine is
+ * available to duplicate a locale object.
+ */
/*#define HAS_NEWLOCALE / **/
/*#define HAS_FREELOCALE / **/
/*#define HAS_USELOCALE / **/
+/*#define HAS_DUPLOCALE / **/
+
+ /* I_XLOCALE:
+ * This symbol, if defined, indicates to the C program that it should
+ * include <xlocale.h> to get uselocale() and it's friends
+ */
+/*#define I_XLOCALE / **/
/* HAS_NEXTAFTER:
* This symbol, if defined, indicates that the nextafter routine is
@@ -5241,6 +5252,6 @@
#endif
/* Generated from:
- * 01a33ec4d20289fa524203757339606daef1a014ff6b693d38234495023ac9e7 config_h.SH
- * d2f05caf5dc56031d3338c8f42e9e317ae1e53faa7b51285d0d6ebc343f8a333 uconfig.sh
+ * 7ed39513c0949dfd5f9cfe8d639eb5ad61bc71d6ac6b0b524a4f95ed2197e38b config_h.SH
+ * e38c64797e19f0328be3383bd28b402329af4d771163f7596ec5e6357235698d uconfig.sh
* ex: set ro: */
diff --git a/uconfig.sh b/uconfig.sh
index 12bbfd1..98c471d 100644
--- a/uconfig.sh
+++ b/uconfig.sh
@@ -110,6 +110,7 @@ d_dosuid='undef'
d_drand48_r='undef'
d_drand48proto='undef'
d_dup2='undef'
+d_duplocale='undef'
d_eaccess='undef'
d_endgrent='undef'
d_endgrent_r='undef'
@@ -709,6 +710,7 @@ i_values='undef'
i_varargs='undef'
i_varhdr='stdarg.h'
i_vfork='undef'
+i_xlocale='undef'
ignore_versioned_solibs='y'
inc_version_list_init='NULL'
installstyle='lib/perl5'
diff --git a/uconfig64.sh b/uconfig64.sh
index 4b8c3ac..bb512f2 100644
--- a/uconfig64.sh
+++ b/uconfig64.sh
@@ -111,6 +111,7 @@ d_dosuid='undef'
d_drand48_r='undef'
d_drand48proto='undef'
d_dup2='undef'
+d_duplocale='undef'
d_eaccess='undef'
d_endgrent='undef'
d_endgrent_r='undef'
@@ -710,6 +711,7 @@ i_values='undef'
i_varargs='undef'
i_varhdr='stdarg.h'
i_vfork='undef'
+i_xlocale='undef'
ignore_versioned_solibs='y'
inc_version_list_init='NULL'
installstyle='lib/perl5'
diff --git a/win32/config.ce b/win32/config.ce
index 7e6fe40..37c0a43 100644
--- a/win32/config.ce
+++ b/win32/config.ce
@@ -157,6 +157,7 @@ d_dosuid='undef'
d_drand48_r='undef'
d_drand48proto='undef'
d_dup2='define'
+d_duplocale='undef'
d_eaccess='undef'
d_endgrent='undef'
d_endgrent_r='undef'
@@ -780,6 +781,7 @@ i_values='undef'
i_varargs='undef'
i_varhdr='varargs.h'
i_vfork='undef'
+i_xlocale='undef'
ignore_versioned_solibs=''
inc_version_list=''
inc_version_list_init='0'
diff --git a/win32/config.gc b/win32/config.gc
index e8179cc..09290b6 100644
--- a/win32/config.gc
+++ b/win32/config.gc
@@ -159,6 +159,7 @@ d_dosuid='undef'
d_drand48_r='undef'
d_drand48proto='undef'
d_dup2='define'
+d_duplocale='undef'
d_eaccess='undef'
d_endgrent='undef'
d_endgrent_r='undef'
@@ -792,6 +793,7 @@ i_values='undef'
i_varargs='undef'
i_varhdr='varargs.h'
i_vfork='undef'
+i_xlocale='undef'
ignore_versioned_solibs=''
inc_version_list=''
inc_version_list_init='0'
diff --git a/win32/config.vc b/win32/config.vc
index 4972db8..04592e8 100644
--- a/win32/config.vc
+++ b/win32/config.vc
@@ -159,6 +159,7 @@ d_dosuid='undef'
d_drand48_r='undef'
d_drand48proto='undef'
d_dup2='define'
+d_duplocale='undef'
d_eaccess='undef'
d_endgrent='undef'
d_endgrent_r='undef'
@@ -791,6 +792,7 @@ i_values='undef'
i_varargs='undef'
i_varhdr='varargs.h'
i_vfork='undef'
+i_xlocale='undef'
ignore_versioned_solibs=''
inc_version_list=''
inc_version_list_init='0'
--
2.5.4 (Apple Git-61)
|
From @tonycoz0003-it-seems-to-work-for-me.patchFrom d167b16949bc7747fd47d8607a212088112f26ad Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Mon, 21 Mar 2016 14:28:47 +1100
Subject: it seems to work for me
I expected to need duplocale(), but I guess we don't
---
locale.c | 4 ++++
perl.h | 1 -
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/locale.c b/locale.c
index d000d93..7518897 100644
--- a/locale.c
+++ b/locale.c
@@ -42,6 +42,10 @@
# include <langinfo.h>
#endif
+#ifdef I_XLOCALE
+# include <xlocale.h>
+#endif
+
#include "reentr.h"
#ifdef USE_LOCALE
diff --git a/perl.h b/perl.h
index 32d8419..abf4f37 100644
--- a/perl.h
+++ b/perl.h
@@ -6043,7 +6043,6 @@ typedef struct am_table_short AMTS;
# define HAS_THREAD_SAFE_LOCALE
#endif
-#undef HAS_THREAD_SAFE_LOCALE /* Not seeming to work */
#else /* No locale usage */
# define IN_LOCALE_RUNTIME 0
--
2.5.4 (Apple Git-61)
|
From @tonycozThere's further discussion of this issue at: http://www.nntp.perl.org/group/perl.perl5.porters/2016/03/msg235289.html |
From @khwilliamsonThis should now be fixed by commit a0b5329 I don't know how to add a test for this, so am leaving the ticket open for now, but not a 5.24 blocker. The script that fails is required to be run multiple times until a failure appears or you think enough tries have happened that it isn't going to fail. The solution committed has been tested for lengthy periods by both the OP and Tony Cook, so we know it works. I would appreciate hearing suggestions for a test. |
From @iabynOn Sat, Apr 09, 2016 at 01:15:39PM -0700, Karl Williamson via RT wrote:
I'd suggest just adding a test that loops for a bit doing the sort of -- |
From @khwilliamsonThanks for reporting this and helping me debug the solution. Commit 706055c added a test for this, so am now closing. This bug was fixed in 5.24, but re-broken recently, and fixed finally by ffdde30 |
@khwilliamson - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#127708 (status was 'resolved')
Searchable as RT127708$
The text was updated successfully, but these errors were encountered: