Fix test failure in POSIX/t/mb.t with semi-broken locales #17039
Comments
From @jmdhCreated by @jmdhThis is a bug report for perl from dom@earth.li, ----------------------------------------------------------------- Perl Info |
From @jmdh0001-Fix-edge-case-test-failure-in-ext-POSIX-t-mb.t.patchFrom ba80ce1f59e6aa82532d84627b8c5d094eeda1a4 Mon Sep 17 00:00:00 2001
From: Dominic Hargreaves <dom@earth.li>
Date: Fri, 7 Jun 2019 10:04:26 +0100
Subject: [PATCH] Fix edge case test failure in ext/POSIX/t/mb.t
This new test fails in an environment where LANG is set to one thing and
LC_ALL is set to another, and where LANG is set to a locale which is
not installed in the environment in question.
Such a test environment is arguably broken, but appears in common
chroot setups such as Debian's sbuild tool where LANG is inherited from
the parent environment, and LC_ALL is used to override it.
---
ext/POSIX/t/mb.t | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/ext/POSIX/t/mb.t b/ext/POSIX/t/mb.t
index 053693e611..3312b0d737 100644
--- a/ext/POSIX/t/mb.t
+++ b/ext/POSIX/t/mb.t
@@ -34,9 +34,13 @@ SKIP: {
my $utf8_locale = find_utf8_ctype_locale();
skip("no utf8 locale available", 3) unless $utf8_locale;
+ # Here we need to influence LC_CTYPE, but it's not enough to just
+ # set this because LC_ALL could override it. It's also not enough
+ # to delete LC_ALL because it could be used to override other
+ # variables such as LANG in the underlying test environment.
+ # Continue to set LC_CTYPE just in case...
local $ENV{LC_CTYPE} = $utf8_locale;
- local $ENV{LC_ALL};
- delete $ENV{LC_ALL};
+ local $ENV{LC_ALL} = $utf8_locale;
fresh_perl_like(
'use POSIX; print &POSIX::MB_CUR_MAX',
--
2.11.0
|
From @jkeenanOn Fri, 07 Jun 2019 10:06:58 GMT, dom wrote:
Pushed to blead in commit 69b89a0, with one committer's edit -- I had to remove a non-printing character in the patch: ##### Dom, since I doubt any of our smoke-testing rigs are set up to reproduce this problem, could you send us some sort of evidence that the problem has been fixed? Thank you very much. |
|
The RT System itself - Status changed from 'new' to 'open' |
From @jmdhOn Fri, Jun 07, 2019 at 05:11:10AM -0700, James E Keenan via RT wrote:
Before the patch was applied, this test failed in my Debian sbuild (The relevant detail here is that outside sbuild, my LANG is en_GB.UTF-8. sbuild itself corrects for this problem by setting LC_ALL, so the fact Thanks, |
From @jkeenanDom, Unfortunately I have to call your attention to 2 smoke-test failures in ext/POSIX/t/mb.t which were recorded *after* I applied your patch. http://perl5.test-smoke.org/report/89146 http://perl5.test-smoke.org/report/89211 (These can be tracked via this search: In each case the failures in mb.t occurred when blead was configured as follows: [stdio] -Dcc=clang -Accflags="-Werror=declaration-after-statement -g -fno-omit-frame-pointer -fsanitize=address -fno-common -fsanitize-blacklist=`pwd`/asan_ignore" -Aldflags="-fsanitize=address" With and without -DDEBUGGING. A couple of points: 1. Tester is using what I would guess is an advanced version of the Linux kernel: 5.0.9-200 versus my own 4.15.0-51 (Ubuntu 18.04 LTS). OTOH, we are getting smoke-test reports from rigs with even higher-numbered Linux kernels. 2. I myself don't understand all those compiler switches the tester is using. In particular, 'make' fails for me on FreeBSD-11.2 when I use those compiler switches. 3. Nonetheless, when I build a perl with all those switches (except -DDEBUGGING), I get those test same failures. See attachments. 4. When I build blead with those same compiler switches at the commit immediately prior to the one where I applied your patch, I get a PASS. ##### ok 1 - mblen() basically works So your patch has triggered test failures, albeit under these very obscure conditions. I'm going to revert your patch from blead and then re-apply it in a branch so that we can continue to gather smoke-test reports. Thank you very much. -- |
From @jkeenan# Failed test 3 - mblen() recognizes invalid multibyte characters at ../../t/test.pl line 1062 Test Summary Report ../ext/POSIX/t/mb.t (Wstat: 0 Tests: 4 Failed: 2) |
From @jkeenanSummary of my perl5 (revision 5 version 31 subversion 1) configuration: Characteristics of this binary (from libperl): |
From @jkeenanOn Sat, 08 Jun 2019 21:43:16 GMT, jkeenan wrote:
The smoke-test branch is: smoke-me/jkeenan/dom/134182-mb -- |
From @jmdhOn Sat, Jun 08, 2019 at 02:43:16PM -0700, James E Keenan via RT wrote:
Very curious, this looks like the original bug that the test was Cheers, |
From @ntyniOn Fri, Jun 14, 2019 at 06:21:16PM +0100, Dominic Hargreaves wrote:
It's a different thing that just happened to get triggered here; this I can reproduce it on 5.30.0. It seems to be related to version strings $ LC_NUMERIC=C.UTF-8 ./perl -l -Ilib -e 'require 5.006;'==21403==ERROR: AddressSanitizer: heap-use-after-free on address 0x602000000190 at pc 0x0000004813aa bp 0x7fff4f62ea90 sp 0x7fff4f62e230 0x602000000190 is located 0 bytes inside of 8-byte region [0x602000000190,0x602000000198) previously allocated by thread T0 here: SUMMARY: AddressSanitizer: heap-use-after-free (/tmp/perl-5.30.0/perl+0x4813a9) in __interceptor_setlocale -- |
From @ntyniOn Mon, Jun 17, 2019 at 09:47:04AM +0300, Niko Tyni wrote:
And further to this. It's not clear to me if this is a problem with asan $ cat t.c; clang -g -fsanitize=address t.c; ./a.out
|
From @ntyniOn Mon, Jun 17, 2019 at 02:49:43PM +0300, Niko Tyni wrote:
Presumably the intervening setlocale() call clobbers the buffer The attached patch to vutil.c seems to fix this issue for me, |
From @ntyni0001-Copy-setlocale-return-value-in-case-it-gets-clobbere.patchFrom 2357c65fd9559dd0852d1cf3febb3a4e468151ed Mon Sep 17 00:00:00 2001
From: Niko Tyni <ntyni@debian.org>
Date: Mon, 17 Jun 2019 16:21:20 +0300
Subject: [PATCH] Copy setlocale() return value in case it gets clobbered by
later calls
Flagged by AddressSanitizer in [perl #134182]
Quoting IEEE Std 1003.1, 2004 Edition
https://pubs.opengroup.org/onlinepubs/009695399/functions/setlocale.html
The string returned by setlocale() is such that a subsequent call with
that string and its associated category shall restore that part of the
program's locale. The application shall not modify the string returned
which may be overwritten by a subsequent call to setlocale().
Bug: https://rt.perl.org/Public/Bug/Display.html?id=134182
---
vutil.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/vutil.c b/vutil.c
index 236748915..6814e59b0 100644
--- a/vutil.c
+++ b/vutil.c
@@ -639,7 +639,7 @@ VER_NV:
LC_NUMERIC_LOCK(0); /* Start critical section */
- locale_name_on_entry = setlocale(LC_NUMERIC, NULL);
+ locale_name_on_entry = savepv(setlocale(LC_NUMERIC, NULL));
if ( strNE(locale_name_on_entry, "C")
&& strNE(locale_name_on_entry, "POSIX"))
{
@@ -647,6 +647,7 @@ VER_NV:
}
else { /* This value indicates to the restore code that we didn't
change the locale */
+ Safefree(locale_name_on_entry);
locale_name_on_entry = NULL;
}
@@ -715,6 +716,7 @@ VER_NV:
if (locale_name_on_entry) {
setlocale(LC_NUMERIC, locale_name_on_entry);
+ Safefree(locale_name_on_entry);
}
LC_NUMERIC_UNLOCK; /* End critical section */
--
2.20.1
|
From @ntyniOn Mon, Jun 17, 2019 at 07:20:32PM +0300, Niko Tyni wrote:
I see vutil.c comes from the version.pm distribution so I've submitted -- |
From @tonycozOn Mon, 17 Jun 2019 09:21:04 -0700, ntyni@debian.org wrote:
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=134212 has a more complete fix (I didn't see this until I diagnosed it.) Tony |
|
I believe this ticket can be closed, since the patch referenced above has been applied. Any disagreement? |
|
Yes, looks good and closeable to me. Thanks! |
|
Uh, taking this back: the original issue @jmdh filed here about mb.t failing on semi-broken locales seems to be still present. @jkeenan reverted the proposed patch after smokers caught the separate issue with memory corruption around vutil.c that's now fixed, but the patch is not reinstated yet afaics. |
|
I rebased the patch, and am smoking it at https://git.io/Jvd6h |
|
I built perl in the smoke-me/khw-mb branch at v5.31.10-26-ge37211489e with these config_args: I then ran: That may resolve the test failures. But I can't be very confident of my results because (a) when I build perl with address sanitizer my computer slows to a halt during Thank you very much. |
|
@xsawyerx I would like permission to merge this patch for 5.32 It is very low risk, as it affects just one .t file that didn't even exist in 5.30, and makes life easier for our downstream Debian partners. And it fell through the cracks for months. I have tested that things fail before the patch is applied in the situation it applies to, and pass after it is applied. |
|
Approved! |
This new test fails in an environment where LANG is set to one thing and LC_ALL is set to another, and where LANG is set to a locale which is not installed in the environment in question. Such a test environment is arguably broken, but appears in common chroot setups such as Debian's sbuild tool where LANG is inherited from the parent environment, and LC_ALL is used to override it. (Committer rebased the patch) This fixes GH #17039
|
Fixed by 8f8f6a1 |
Migrated from rt.perl.org#134182 (status was 'open')
Searchable as RT134182$
The text was updated successfully, but these errors were encountered: