Assert fail in vutil.c with very large version numbers in warnings in quadmath builds: use B.9E90
#15347
Comments
From @dcollinsnI have compiled bleadperl with the afl-gcc compiler using: ./Configure -Dusedevel -Dprefix='/usr/local/perl-afl' -Dcc='ccache afl-gcc' -Uuselongdouble -Duse64bitall -Doptimize=-g -Uversiononly -Uman1dir -Uman3dir -Dusequadmath -DDEBUGGING -des And then fuzzed the resulting binary using: AFL_NO_VAR_CHECK=1 afl-fuzz -i in -o out bin/perl -t -W @@ After reducing testcases using `afl-tmin` and performing additional minimization by hand, I have located the following testcase that triggers a assert fail in debugging builds of the perl interpreter. The testcase is the file below. On most perls, this exits with the correct errors - version v.Inf required. However, on debugging builds, with quadmath enabled, with -W, the following assert failure is emitted. This seems to be harmless? use B.9E90 DEBUG PERL: REGULAR PERL: dcollins@nightshade64:~/perlquad$ ./perl -W -e 'use B.9E90' Debugging tool output is below. A git bisect was attempted. In very old perls (5.14.0), this emitted a "panic: snprintf buffer overflow". With that marked as "good" and the assert fail marked as "bad", bisect reports the following commit, which is the commit where the assert was first added, so, not too helpful. 7738054 is the first bad commit Allow dynamic lock of LC_NUMERIC When processing version strings, the radix character must be a dot even vutil.c is cpan-upstream, but already blead and cpan have diverged, so :100644 100644 17b2551773d01d16fd1fbf6f903a0e33ac469a98 3e7d4a36f7eecdd94c3ac1bb afd5e50fdcdea909 M intrpvar.h **GDB** dcollins@nightshade64:~/perlquad$ gdb --args ./perl -Ilib -W -e 'use B.9E90' Program received signal SIGABRT, Aborted. Inferior 1 [process 20534] will be killed. Quit anyway? (y or n) y **VALGRIND** ==13582== **PERL -V** dcollins@nightshade64:~/perlquad$ ./perl -Ilib -V Characteristics of this binary (from libperl): |
From @tonycozOn Sat, 21 May 2016 07:56:26 -0700, dcollinsn@gmail.com wrote:
This is caused by two different issues: a) Perl_upg_version() can be called in a context with warnings enabled, and for large version numbers it attempts to append to an undefined sv. This is trivially fixable (use Perl_sv_setpvf() instead of Perl_sv_catpvf()). b) the LOCK_NUMERIC_STANDARD() (aka LOCK_LC_NUMERIC_STANDARD() ) and UNLOCK_NUMERIC_STANDARD() (aka UNLOCK_LC_NUMERIC_STANDARD() ) macros don't handle recursion correctly. The -MB.9e90 is converted to: use B.9e90; which ends up calling Exporter->import(.9e90). Since that value is Because of a) a warning is generated while the numeric standard is "locked" and the __WARN__ handler set in Exportor/Heavy.pm is called. This attempted to load Carp, eventually leading to the nested call to Perl_upg_version(), where the numeric standard is "locked" and "unlocked" again leaving PL_numeric_standard as 1. When we return back to the original call to Perl_upg_version() the assertion in UNLOCK_NUMERIC_STANDARD() fires since PL_numeric_standard is no longer 2. So these macros should probably handle this nesting in some way. Tony |
|
The RT System itself - Status changed from 'new' to 'open' |
From @khwilliamsonI believe the attached two patches do the trick When I run the reduced test case with these applied, the output is Integer overflow in version at 128207.pl line 1. |
From @khwilliamsonOn Sat, 03 Jun 2017 08:12:08 -0700, khw wrote:
Oops. forgot to attach |
From @khwilliamson0002-Make-LOCK_LC_NUMERIC_STANDARD-recursive.patchFrom f495555b9a2acc588c709cf5d88390dd049a03bd Mon Sep 17 00:00:00 2001
From: Karl Williamson <khw@cpan.org>
Date: Sat, 3 Jun 2017 09:08:50 -0600
Subject: [PATCH 2/3] Make LOCK_LC_NUMERIC_STANDARD recursive
Same for UNLOCK_LC_NUMERIC_STANDARD.
---
perl.h | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/perl.h b/perl.h
index ee98d40f07..0ad1ca711c 100644
--- a/perl.h
+++ b/perl.h
@@ -6191,14 +6191,14 @@ expression, but with an empty argument list, like this:
_restore_LC_NUMERIC_function = &Perl_set_numeric_standard; \
}
-/* Lock to the C locale until unlock is called */
+/* Lock/unlock to the C locale until unlock is called. This needs to be
+ * recursively callable. [perl #128207] */
#define LOCK_LC_NUMERIC_STANDARD() \
(__ASSERT_(PL_numeric_standard) \
- PL_numeric_standard = 2)
-
+ PL_numeric_standard++)
#define UNLOCK_LC_NUMERIC_STANDARD() \
- (__ASSERT_(PL_numeric_standard == 2) \
- PL_numeric_standard = 1)
+ (__ASSERT_(PL_numeric_standard > 1) \
+ PL_numeric_standard > 1 && PL_numeric_standard--)
#define RESTORE_LC_NUMERIC_UNDERLYING() \
if (_was_local) set_numeric_local();
--
2.11.0
|
From @khwilliamson0003-vutil.c-Use-setpvf-to-avoid-uninit.patchFrom aeadda286c2ef5466605047f688b8d37585556bf Mon Sep 17 00:00:00 2001
From: Karl Williamson <khw@cpan.org>
Date: Sat, 3 Jun 2017 09:09:38 -0600
Subject: [PATCH 3/3] vutil.c: Use setpvf to avoid uninit.
---
vutil.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/vutil.c b/vutil.c
index c033820fa5..727478e4f9 100644
--- a/vutil.c
+++ b/vutil.c
@@ -655,7 +655,7 @@ VER_NV:
STORE_NUMERIC_LOCAL_SET_STANDARD();
LOCK_NUMERIC_STANDARD();
if (sv) {
- Perl_sv_catpvf(aTHX_ sv, "%.9"NVff, SvNVX(ver));
+ Perl_sv_setpvf(aTHX_ sv, "%.9"NVff, SvNVX(ver));
len = SvCUR(sv);
buf = SvPVX(sv);
}
--
2.11.0
|
From @cpansproutOn Sat, 03 Jun 2017 08:13:54 -0700, khw wrote:
Please make sure that any changes to vutil.c get submitted to the version.pm repository/bugtracker and then they work with older versions of perl. Remember that this file is living a double life. -- Father Chrysostomos |
From @tonycozOn Sat, 03 Jun 2017 08:13:54 -0700, khw wrote:
They look good to me. Tony |
From @khwilliamsonI have created a PR for version for the appending to an undefined SV, The other fixes have been in blead for some time now, and prevent the assertion failure, so I'm closing this ticket |
|
@khwilliamson - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#128207 (status was 'resolved')
Searchable as RT128207$
The text was updated successfully, but these errors were encountered: