Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upext/XS-APItest/t/locale.t crashes on Windows after passing a couple tests #16922
Comments
From @steve-m-hayCreated by @steve-m-hayBlead is currently crashing at the end of ext/XS-APItest/t/locale.t on I get the same result using VC14 and VC14.1, but VC12 and MinGW-w64 Verbose output: ../ext/XS-APItest/t/locale.t .. Test Summary Report Perl Info |
From @khwilliamsonOn 4/3/19 11:00 AM, Steve Hay (via RT) wrote:
Since I don't have access to a box where this is failing, this will take Let's start by running this single test manually on a DEBUGGING build |
|
The RT System itself - Status changed from 'new' to 'open' |
From @steve-m-hay(And again, with gzipped attachment, since the plain text was too large.) On Thu, 4 Apr 2019 at 22:09, Steve Hay <steve.m.hay@googlemail.com> wrote:
|
From @steve-m-haySummary of my perl5 (revision 5 version 29 subversion 10) configuration: Characteristics of this binary (from libperl): |
From @steve-m-hay |
From @khwilliamsonOn 4/4/19 3:09 PM, Steve Hay wrote:
Thanks for that. I think I now know what the problem is. To confirm, https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/khw-133981 |
From @khwilliamsonOn 4/4/19 3:40 PM, Karl Williamson wrote:
In looking further, I'm not confident that will help. It looks now like From the trace you gave, that looks like it will print 3 lines, the |
From @khwilliamson#include <stdio.h>
#include <locale.h>
int
main()
{
const char * result = setlocale(LC_NUMERIC, "iso_latin_16");
if (result == NULL) result = "NULL";
fprintf(stderr, "%d: %s\n", __LINE__, result);
result = setlocale(LC_ALL, "iso_latin_16");
if (result == NULL) result = "NULL";
fprintf(stderr, "%d: %s\n", __LINE__, result);
result = setlocale(LC_NUMERIC, "iso_latin_16");
if (result == NULL) result = "NULL";
fprintf(stderr, "%d: %s\n", __LINE__, result);
}
|
From @steve-m-hayOn Thu, 4 Apr 2019 at 23:31, Karl Williamson <public@khwilliamson.com> wrote:
You are correct: With VC14, VC14.1 and VC14.2 (VS2015, VS2017 and VS2019): 9: NULL With VC6 thru VC12 (VC++ 6.0 thru VS2013): 9: NULL So, being a Windows (actually, recent VC++?) problem, I'll report it Thanks for diagnosing. Meanwhile, we should skip the tests (or mark them as TODO) for VC14+. |
From @steve-m-hayOn Thu, 4 Apr 2019 at 22:40, Karl Williamson <public@khwilliamson.com> wrote:
Just for completeness, though we're not expecting success here now |
From @steve-m-hay |
From @steve-m-hayOn Fri, 5 Apr 2019 at 08:25, Steve Hay <steve.m.hay@googlemail.com> wrote:
Now reported here: |
From @steve-m-hayOn Fri, 05 Apr 2019 01:06:09 -0700, shay wrote:
An update on that page acknowledges this as a regression in the Universal CRT introduced in the Windows 10 April 2018 Update. It will be fixed in the May 2019 update. |
From @khwilliamsonOn Fri, 26 Apr 2019 00:09:18 -0700, shay wrote:
So what do we do about this ticket? Its out of our hands. I don't see a work around possible. I'm guessing everybody gets this fix as part of Windows Update. |
From @steve-m-hayOn Mon, 29 Apr 2019 17:06:17 -0700, khw wrote:
I think so. I've updated README.win32 in commit 7115365 to note the impending fix. I will leave the ticket open for now. Hopefully I'll get the update the next month or so and if it works then I'll close the ticket then. (I don't think a suggestion I made earlier in this ticket to skip the tests for VC++ 14 and above is a good idea since it would now require testing for the Windows 10 Update version in the skip logic, which isn't worth the hassle with a fix on the way soon anyway.) |
From @khwilliamsonOn Tue, 30 Apr 2019 09:44:23 -0700, shay wrote:
I believe the fix is out. Can someone verify if this is still an issue? |
From @steve-m-hayOn Thu, 27 Jun 2019 09:25:27 -0700, khw wrote:
I've updated to Windows 10 May 2019 (Version 1903) and now the shay.c program that you sent previously produces the same output with VC14.2 as it does with VC6-VC12, namely: 9: NULL However, our test script still crashes as before!: D:\Dev\Git\perl\t>.\perl harness ..\ext\XS-APItest\t\locale.t Test Summary Report ../ext/XS-APItest/t/locale.t (Wstat: 2304 Tests: 2 Failed: 0) New -DL output attached. The crash is here: ucrtbase.dll!00007ff88794bcf8() Unknown
The code is: The exception text is: I'm not sure what the problem is here now. If I build and run this: #include <stdio.h> int then there is no crash. The output is simply: 9: NULL |
From @steve-m-hay |
From @tonycozCreated by @tonycozWhile testing perl on Windows 10 x64 I saw ext/XS-APItest/t/locale.t On further tracing this appeared to be aborting on a setlocale() call in On running the test in the debugger (after rebuild with CFG=DebugFull, and This appears to be failing when the CRT is internally trying to convert a From the CRT source in setlocale.cpp: extern "C" static wchar_t* __cdecl call_wsetlocale(int const category, char const* const narrow_locale) size_t size; __crt_unique_heap_ptr<wchar_t> wide_locale(_calloc_crt_t(wchar_t, size)); -->> exception thrown by this check return _wsetlocale(category, wide_locale.get()); At the point of the exception size is zero, and wide_locale's pointer is I suspect the first sizing mbstowcs_s() call is failing because the current locale It may be the the win32 specific code should be using _wsetlocal() instead of Perl Info |
From @steve-m-hayOn Mon, 22 Jul 2019 23:29:59 -0700, tonyc wrote:
This is a duplicate of https://rt.perl.org/Ticket/Display.html?id=133981, which is documented in README.win32: https://perl5.git.perl.org/perl.git/commit/7115365105 You've debugged it further than me though :-) Do you have the May 2019 Update on your machine? |
|
The RT System itself - Status changed from 'new' to 'open' |
From @tonycozOn Tue, 23 Jul 2019 00:07:47 -0700, shay wrote:
Oops, I've merged them.
I don't think I'd used it since I set up this machine in February. I'm updating now, though that might require a reboot. Tony |
From @tonycozOn Tue, 23 Jul 2019 04:09:07 -0700, tonyc wrote:
Found a chance to reboot. Rebuilt with gmake -j3 CCTYPE=MSVC142 CFG=DebugFull test TEST_FILES=../ext/XS-APItest/t/locale.t but it still fails, on the same name: Invalid parameter detected in function _mbstowcs_s_l. File: minkernel\crts\ucrt\src\appcrt\convert\mbstowcs.cpp, line: 246 I'll have a play with a fix tomorrow, probably using _wsetlocale(). Tony |
From @tonycozOn Tue, 23 Jul 2019 04:54:07 -0700, tonyc wrote:
It turns out I misunderstood the question - I'm running version 1803 of Windows 10. I'm on the "Semi-Annual Channel" rather than the oh so clearly named "Semi-Annual Channel (Targeted)". So I still have the buggy crt, I'm not sure this is worth us fixing. Tony |
From @tonycozOn Tue, 23 Jul 2019 17:12:25 -0700, tonyc wrote:
A reply didn't make it to the ticket: https://www.nntp.perl.org/group/perl.perl5.porters/2019/07/msg255688.html indicating that 1903 didn't fix it. So here's a rough patch. Tony |
From @tonycoz0001-perl-133981-rough-fix-for-strange-setlocale-abort.patchFrom 0048db520669535dcd911bfe5dbc3029a8a7cf45 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Wed, 24 Jul 2019 15:56:08 +1000
Subject: (perl #133981) rough fix for strange setlocale() abort
The part that aborts is the wrapper that converts the CRT setlocale()
call into a call to _wsetlocale(), so replace that.
Rough in that:
- I haven't tested on non-MSVC 2019 (yet)
- it always uses CP_UTF8 - is that the correct solution?
- the return value is freed on the next LEAVE
---
locale.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/locale.c b/locale.c
index db83d993de..ebd1533425 100644
--- a/locale.c
+++ b/locale.c
@@ -2084,6 +2084,57 @@ S_new_collate(pTHX_ const char *newcoll)
#ifdef WIN32
+#define USE_WSETLOCALE
+
+#ifdef USE_WSETLOCALE
+
+STATIC char *
+S_wrap_wsetlocale(pTHX_ int category, const char *locale) {
+ wchar_t *wlocale;
+ wchar_t *wresult;
+ char *result;
+
+ if (locale) {
+ int req_size =
+ MultiByteToWideChar(CP_UTF8, 0, locale, -1, NULL, 0);
+
+ if (!req_size) {
+ errno = EINVAL;
+ return NULL;
+ }
+
+ Newx(wlocale, req_size, wchar_t);
+ if (!MultiByteToWideChar(CP_UTF8, 0, locale, -1, wlocale, req_size)) {
+ Safefree(wlocale);
+ errno = EINVAL;
+ return NULL;
+ }
+ }
+ else {
+ wlocale = NULL;
+ }
+ wresult = _wsetlocale(category, wlocale);
+ Safefree(wlocale);
+ if (wresult) {
+ int req_size =
+ WideCharToMultiByte(CP_UTF8, 0, wresult, -1, NULL, 0, NULL, NULL);
+ Newx(result, req_size, char);
+ SAVEFREEPV(result); /* is there something better we can do here? */
+ if (!WideCharToMultiByte(CP_UTF8, 0, wresult, -1,
+ result, req_size, NULL, NULL)) {
+ errno = EINVAL;
+ return NULL;
+ }
+ }
+ else {
+ result = NULL;
+ }
+
+ return result;
+}
+
+#endif
+
STATIC char *
S_win32_setlocale(pTHX_ int category, const char* locale)
{
@@ -2141,7 +2192,11 @@ S_win32_setlocale(pTHX_ int category, const char* locale)
}
+#ifdef USE_WSETLOCALE
+ result = S_wrap_wsetlocale(aTHX_ category, locale);
+#else
result = setlocale(category, locale);
+#endif
DEBUG_L(STMT_START {
dSAVE_ERRNO;
PerlIO_printf(Perl_debug_log, "%s:%d: %s\n", __FILE__, __LINE__,
@@ -2162,7 +2217,11 @@ S_win32_setlocale(pTHX_ int category, const char* locale)
for (i = 0; i < LC_ALL_INDEX; i++) {
result = PerlEnv_getenv(category_names[i]);
if (result && strNE(result, "")) {
+#ifdef USE_WSETLOCALE
+ S_wrap_wsetlocale(aTHX_ category, locale);
+#else
setlocale(categories[i], result);
+#endif
DEBUG_Lv(PerlIO_printf(Perl_debug_log, "%s:%d: %s\n",
__FILE__, __LINE__,
setlocale_debug_string(categories[i], result, "not captured")));
--
2.20.1.windows.1
|
From @tonycozOn Tue, 23 Jul 2019 22:59:04 -0700, tonyc wrote:
Oops, forgot to mention, while the test no longer crashes, later tests produce Invalid parameter messages from the CRT: ok 23 - Returns expected value('AM') for AM_STRInvalid parameter dete Invalid parameter detected in function ok 27 - Returns expected value('Monday') for DAY_2_ Expression: ok 33 - Returns a value (in this case '%x') for D_FMTt (and more) This appears to be unrelated to this change. Tony |
From @steve-m-hayOn Tue, 23 Jul 2019 22:59:04 -0700, tonyc wrote:
They did change something, though - see my comment here: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=133981#txn-1645106. Maybe there are two separate issues here - one CRT problem which they have fixed (khw's shay.c program now behaves correctly), and one other CRT problem which your patch works around. We should probably tell them about that too and hopefully they'll fix it in some future update (though we still need the workaround for now, of course). Thanks for the patch. I'll give it a try with some different compiler/OS versions. |
From @steve-m-hayOn Wed, 24 Jul 2019 00:14:12 -0700, shay wrote:
The patch looks good to me, except that the second S_wrap_wsetlocale(aTHX_ category, locale); looks like it should be S_wrap_wsetlocale(aTHX_ categories[i], result); ? Regardless, it fixes the failures on Windows 10, using VC14, 14.1 and 14.2. I've also tried VC12 on Win10 and all is OK. On Windows 7 I've tried VC7 and VC14 and all is well. (I wouldn't worry about the invalid parameter warnings coming out, at least not now, because they're probably not new and there are many other invalid parameter warnings when running the full test suite in a DebugFull build, and always have been ever since that build option was added.) |
From @tonycozOn Sat, 10 Aug 2019 05:53:14 -0700, shay wrote:
Updated patch attached.
Thanks. Since this fixes tests that are failing, I don't think it needs extra tests.
Ok. Tony |
From @tonycoz0001-perl-133981-rough-fix-for-strange-setlocale-abort.patchFrom 6891872d0c871fe138f992653be06bd771f6c58a Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Wed, 24 Jul 2019 15:56:08 +1000
Subject: (perl #133981) rough fix for strange setlocale() abort
The part that aborts is the wrapper that converts the CRT setlocale()
call into a call to _wsetlocale(), so replace that.
Rough in that:
- I haven't tested on non-MSVC 2019 (yet)
- it always uses CP_UTF8 - is that the correct solution?
- the return value is freed on the next LEAVE
---
locale.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/locale.c b/locale.c
index af7af60038..b7c7a3eceb 100644
--- a/locale.c
+++ b/locale.c
@@ -2084,6 +2084,57 @@ S_new_collate(pTHX_ const char *newcoll)
#ifdef WIN32
+#define USE_WSETLOCALE
+
+#ifdef USE_WSETLOCALE
+
+STATIC char *
+S_wrap_wsetlocale(pTHX_ int category, const char *locale) {
+ wchar_t *wlocale;
+ wchar_t *wresult;
+ char *result;
+
+ if (locale) {
+ int req_size =
+ MultiByteToWideChar(CP_UTF8, 0, locale, -1, NULL, 0);
+
+ if (!req_size) {
+ errno = EINVAL;
+ return NULL;
+ }
+
+ Newx(wlocale, req_size, wchar_t);
+ if (!MultiByteToWideChar(CP_UTF8, 0, locale, -1, wlocale, req_size)) {
+ Safefree(wlocale);
+ errno = EINVAL;
+ return NULL;
+ }
+ }
+ else {
+ wlocale = NULL;
+ }
+ wresult = _wsetlocale(category, wlocale);
+ Safefree(wlocale);
+ if (wresult) {
+ int req_size =
+ WideCharToMultiByte(CP_UTF8, 0, wresult, -1, NULL, 0, NULL, NULL);
+ Newx(result, req_size, char);
+ SAVEFREEPV(result); /* is there something better we can do here? */
+ if (!WideCharToMultiByte(CP_UTF8, 0, wresult, -1,
+ result, req_size, NULL, NULL)) {
+ errno = EINVAL;
+ return NULL;
+ }
+ }
+ else {
+ result = NULL;
+ }
+
+ return result;
+}
+
+#endif
+
STATIC char *
S_win32_setlocale(pTHX_ int category, const char* locale)
{
@@ -2141,7 +2192,11 @@ S_win32_setlocale(pTHX_ int category, const char* locale)
}
+#ifdef USE_WSETLOCALE
+ result = S_wrap_wsetlocale(aTHX_ category, locale);
+#else
result = setlocale(category, locale);
+#endif
DEBUG_L(STMT_START {
dSAVE_ERRNO;
PerlIO_printf(Perl_debug_log, "%s:%d: %s\n", __FILE__, __LINE__,
@@ -2162,7 +2217,11 @@ S_win32_setlocale(pTHX_ int category, const char* locale)
for (i = 0; i < LC_ALL_INDEX; i++) {
result = PerlEnv_getenv(category_names[i]);
if (result && strNE(result, "")) {
+#ifdef USE_WSETLOCALE
+ S_wrap_wsetlocale(aTHX_ categories[i], locale);
+#else
setlocale(categories[i], result);
+#endif
DEBUG_Lv(PerlIO_printf(Perl_debug_log, "%s:%d: %s\n",
__FILE__, __LINE__,
setlocale_debug_string(categories[i], result, "not captured")));
--
2.20.1.windows.1
|
From @steve-m-hayOn Mon, 12 Aug 2019 22:49:19 -0700, tonyc wrote:
You've changed category to categories[i], but not locale to result. |
From @steve-m-hayOn Thu, 15 Aug 2019 at 08:05, Steve Hay via RT
I've retested the updated patch with that extra change that you missed |
From @tonycozOn Sun, 18 Aug 2019 04:04:52 -0700, shay wrote:
Thanks, I've applied it with that other fix and a shiny commit message Tony |
|
@tonycoz - Status changed from 'open' to 'pending release' |
From @steve-m-hayOn Mon, 02 Sep 2019 18:03:28 -0700, tonyc wrote:
Your commit isn't quite what I was expecting. When I noted previously that "You've changed category to categories[i], but not locale to result" I meant that the second S_wrap_wsetlocale() call should be S_wrap_wsetlocale(aTHX_ categories[i], result) -- to match the arguments of the existing second setlocale() call, which was setlocale(categories[i], result). Instead, you've left that S_wrap_wsetlocale() as S_wrap_wsetlocale(aTHX_ categories[i], locale) and changed the existing setlocale() call to setlocale(categories[i], locale). Did you mean to do that? I've belatedly reproduced the failure in a small test program: #include <stdio.h> int result = setlocale(LC_ALL, "Català"); This prints: 9: Japanese_Japan.932 and then causes an invalid parameter exception to be raised. There is no invalid parameter handler in this program to deal with it, so the program crashes:
I don't think it's a(nother) CRT bug. It's just garbage-in garbage-out: We have successfully switched to Japanese locale and then use six bytes ("Català") that aren't a valid Japanese encoding, so the CRT is correct to complain it's an invalid parameter. The fix is to deal with the encoding correctly so as not to pass garbage in, which is hopefully what your patch takes care of. |
From @tonycozOn Tue, 03 Sep 2019 00:48:47 -0700, shay wrote:
Yeah, it was a dumb mistake on my part. The attached should fix it. Tony |
From @tonycoz0001-perl-133981-fix-my-stupid-mistake.patchFrom 17a1e9ac2917a0f8cdb3bf899724e07190e3d8ec Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Thu, 5 Sep 2019 15:37:30 +1000
Subject: (perl #133981) fix my stupid mistake
---
locale.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/locale.c b/locale.c
index 0029c8023a..cdf125cee5 100644
--- a/locale.c
+++ b/locale.c
@@ -2218,9 +2218,9 @@ S_win32_setlocale(pTHX_ int category, const char* locale)
result = PerlEnv_getenv(category_names[i]);
if (result && strNE(result, "")) {
#ifdef USE_WSETLOCALE
- S_wrap_wsetlocale(aTHX_ categories[i], locale);
+ S_wrap_wsetlocale(aTHX_ categories[i], result);
#else
- setlocale(categories[i], locale);
+ setlocale(categories[i], result);
#endif
DEBUG_Lv(PerlIO_printf(Perl_debug_log, "%s:%d: %s\n",
__FILE__, __LINE__,
--
2.23.0.windows.1
|
|
@tonycoz - Status changed from 'pending release' to 'open' |
From @steve-m-hayOn Wed, 04 Sep 2019 22:39:51 -0700, tonyc wrote:
Yes, that looks good now. |
From @tonycozOn Fri, 06 Sep 2019 00:02:05 -0700, shay wrote:
Applied as 17a1e9a. Sorry about the mess. Tony |
|
@tonycoz - Status changed from 'open' to 'pending release' |
Migrated from rt.perl.org#133981 (status was 'pending release')
Searchable as RT133981$