Quadmath builds fail porting/libperl.t #15436
Comments
From @dcollinsnGreetings, Configuring blead with: ./Configure -Dusedevel -des -Dusequadmath Results in a test failure in libperl.t: dcollins@nightshade64:~/perlquad$ ./perl -Ilib -V Characteristics of this binary (from libperl): Test Summary Report If this is relevant, in the locale.o section of `nm libperl.a` I found: 0000000000000000 t S_stdize_locale -- |
From @tonycozOn Sun Jul 10 20:11:51 2016, dcollinsn@gmail.com wrote:
I think it's optimizing this code: *sans_nuls = '\0'; /* Replace each NUL with the lowest collating control. Loop until have /* Do the actual replacement */ /* Move past the input NUL */ At entry it can assume s points at the end of the string, and replaces movb $0, (%rax) Building with optimization disabled removes the reference to strcpy(). Tony |
|
The RT System itself - Status changed from 'new' to 'open' |
From @khwilliamsonOn Sun Jul 10 21:52:17 2016, tonyc wrote:
It seems to me the best course would be to disable this test if optimization is being done. The point is to catch places in the code where a programmer has made a mistake, and is using unsafe C paradigms. One generally assumes that the optimizer knows what it is doing, and we don't have to check up on it. By skipping the test on optimized builds, we still will get an indication in our smokes that the programmer made a potential mistake, so that is adequate for our purpose. |
From @tonycozOn Thu, Jul 14, 2016 at 03:17:46PM -0700, Karl Williamson via RT wrote:
strcat() is about as safe as strcpy(). The test gives locale.c an explicit pass on using strcat(), perhaps it Or revoke allowing strcat() in locale.c. Tony |
From @dcollinsnOn Thu Jul 14 17:18:14 2016, tonyc wrote:
locale.c specifically uses strcat, and that's special-cased in porting/libperl.t. Since we see that the optimizer may use the "unsafe" builtins, but does so in a safe way, I think that this new exception shouldn't be explicitly for strcpy in locale.c, but for any such functions under -O2 or higher. Otherwise we will be changing porting/libperl.t whenever the optimizer gets a little bit better, or after unrelated changes elsewhere in the codebase. Is this patch OK with you guys? -- |
From @dcollinsn0001-RT-128598-libperl.t-optimizer-may-use-any-builtins.patchFrom 892c31e4484bf06a42d4bc8dba7258bad33299e9 Mon Sep 17 00:00:00 2001
From: Dan Collins <dcollinsn@gmail.com>
Date: Thu, 14 Jul 2016 19:51:13 -0400
Subject: [PATCH] [RT #128598] libperl.t: optimizer may use any builtins
porting/libperl.t makes sure that programmers don't accidentally
use unsafe builtins like strcpy. However, the optimizer is able
to use these on its own. For example, quadmath builds with -O2
or higher with gcc will use strcpy in locale.c, even though that
function isn't used in the source code.
This would skip those tests only under -O2 or -O3 and only if they
would fail. That way, any errant appearances of these symbols will
be visible in test output, and if those functions are used directly,
the test will still fail under -O0 or -O1.
---
t/porting/libperl.t | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/t/porting/libperl.t b/t/porting/libperl.t
index 21e7edb..e82cb8a 100644
--- a/t/porting/libperl.t
+++ b/t/porting/libperl.t
@@ -547,6 +547,10 @@ for my $symbol (sort keys %unexpected) {
SKIP: {
skip("locale.o legitimately uses strcat");
}
+ } elsif (@o > 0 && $Config{optimize} =~ /-O[23]/) {
+ SKIP: {
+ skip("uses $symbol (@o), but optimizer may use it legitimately");
+ }
} else {
is(@o, 0, "uses no $symbol (@o)");
}
--
2.8.1
|
From @khwilliamsonOn 07/14/2016 07:23 PM, Dan Collins via RT wrote:
I believe we call some compilers on some platforms with just "-O". I |
From @dcollinsnWell, we might also call MSVC with /Og or /Ox - but I don't have any On Thu, Jul 14, 2016 at 10:58 PM, karl williamson via RT <
|
From @craigberryOn Thu, Jul 14, 2016 at 10:45 PM, Dan Collins <dcollinsn@gmail.com> wrote:
Is there any particular reason to avoid just switching to my_strlcat |
From @khwilliamsonOn 07/15/2016 07:23 AM, Craig A. Berry wrote:
Sure, I can do that. But our point is that the test is vulnerable to Also, I claim that it is sufficient for the test to fail on any single |
From @andk6696cfa is the first bad commit Change mem_collxfrm() algorithm for embedded NULs diagnostics: porting/libperl.t (Wstat: 0 Tests: 35 Failed: 1) Running verbose: not ok 26 - uses no strcpy (locale.o) % ./perl -Ilib -V Characteristics of this binary (from libperl): -- |
From @tonycozOn Mon Aug 08 21:32:04 2016, andreas.koenig.7os6VVqR@franz.ak.mind.de wrote:
Duplicate of 128598, I'll merge them. Tony |
|
The RT System itself - Status changed from 'new' to 'open' |
From @tonycozOn Thu Jul 14 19:58:13 2016, public@khwilliamson.com wrote:
I believe the real problem is using strcat() at all, I suspect from this + * This is one of the few places in the perl core, where we can use that you misunderstood why we avoid strcpy(), strcat(). We have my_strlcat() and my_strlcpy() that deal with NUL terminated strings The attached replaces strcat() with my_strlcat() and removes the exception in Tony |
From @tonycoz0001-perl-128598-don-t-use-strcat-even-in-locale.c.patchFrom f994a3cf78cfa38658a37e58c7c85278ff609d98 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Tue, 9 Aug 2016 16:27:16 +1000
Subject: [perl #128598] don't use strcat(), even in locale.c
All the arguments againt strcat() apply even to locale.c
---
locale.c | 16 +++++++---------
t/porting/libperl.t | 7 -------
2 files changed, 7 insertions(+), 16 deletions(-)
diff --git a/locale.c b/locale.c
index f698377..aa6682d 100644
--- a/locale.c
+++ b/locale.c
@@ -1462,14 +1462,11 @@ Perl__mem_collxfrm(pTHX_ const char *input_string,
* This will give as good as possible results on strings that don't
* otherwise contain that character, but otherwise there may be
* less-than-perfect results with that character and NUL. This is
- * unavoidable unless we replace strxfrm with our own implementation.
- *
- * This is one of the few places in the perl core, where we can use
- * standard functions like strlen() and strcat(). It's because we're
- * looking for NULs. */
+ * unavoidable unless we replace strxfrm with our own implementation. */
if (s_strlen < len) {
char * e = s + len;
char * sans_nuls;
+ STRLEN sans_nuls_size;
STRLEN cur_min_char_len;
int try_non_controls;
@@ -1576,16 +1573,17 @@ Perl__mem_collxfrm(pTHX_ const char *input_string,
* character in it is NUL. Multiply that by the length of each
* replacement, and allow for a trailing NUL */
cur_min_char_len = strlen(PL_strxfrm_min_char);
- Newx(sans_nuls, (len * cur_min_char_len) + 1, char);
+ sans_nuls_size = (len * cur_min_char_len) + 1;
+ Newx(sans_nuls, sans_nuls_size, char);
*sans_nuls = '\0';
/* Replace each NUL with the lowest collating control. Loop until have
* exhausted all the NULs */
while (s + s_strlen < e) {
- strcat(sans_nuls, s);
+ my_strlcat(sans_nuls, s, sans_nuls_size);
/* Do the actual replacement */
- strcat(sans_nuls, PL_strxfrm_min_char);
+ my_strlcat(sans_nuls, PL_strxfrm_min_char, sans_nuls_size);
/* Move past the input NUL */
s += s_strlen + 1;
@@ -1593,7 +1591,7 @@ Perl__mem_collxfrm(pTHX_ const char *input_string,
}
/* And add anything that trails the final NUL */
- strcat(sans_nuls, s);
+ my_strlcat(sans_nuls, s, sans_nuls_size);
/* Switch so below we transform this modified string */
s = sans_nuls;
diff --git a/t/porting/libperl.t b/t/porting/libperl.t
index 21e7edb..4a3e568 100644
--- a/t/porting/libperl.t
+++ b/t/porting/libperl.t
@@ -540,13 +540,6 @@ for my $symbol (sort keys %unexpected) {
SKIP: {
skip("uses sprintf for Gconvert in sv.o");
}
- }
- elsif ( $symbol eq 'strcat'
- && @o == 1 && $o[0] eq 'locale.o')
- {
- SKIP: {
- skip("locale.o legitimately uses strcat");
- }
} else {
is(@o, 0, "uses no $symbol (@o)");
}
--
2.1.4
|
From @cpansproutOn Mon Aug 08 23:35:02 2016, tonyc wrote:
Can you explain why? Or is it documented somewhere? -- Father Chrysostomos |
From @khwilliamsonOn 08/09/2016 03:34 PM, Father Chrysostomos via RT wrote:
Maybe my comment is a little cheeky, but I do believe I understand why First, in Perl, they don't generally work as C strings are Second, in general, they are fragile, with the possibility of reading If there are other reasons, I don't know them. In this case, I used them because I was dealing with what they are It's easiest to fix this ticket by changing to use strlcpy() and |
From @tonycozOn Tue Aug 09 16:08:23 2016, public@khwilliamson.com wrote:
strncat() is probably a bad example, but you've got the right idea. We could say "we've validated this use and it's safe" and keep using it, my_strlcpy() and my_strlcat() (which have saner behaviour than strncpy() and strncat()) mitigate any issues that such changes introduce. Versions of my_strlcpy() and my_strlcat() that assert on overflow might
The test is fragile in both directions - the compiler could define strcpy() Strangely, we declare strcat(), strcpy() in some cases (perl.h): #ifndef STANDARD_C Tony |
From jjuran@gmail.comOn Aug 9, 2016, at 7:07 PM, Karl Williamson <public@khwilliamson.com> wrote:
In order to avoid buffer overflows with strcat(), you generally need to know the length of the destination string in advance. Since strcat() is basically strlen() + strcpy(), the length measurement is duplicated. If strcat() is called in a loop with the same destination string, the complexity is quadratic, whereas tracking the increasing length and using strcpy() is merely linear. Of course, since for safety you also have to know the length of the copied string in advance, one may as well use memcpy(), which might be more efficient than strcpy() (since it can potentially copy multiple bytes at once). Josh |
From @khwilliamsonTaking the path of least resistance, commit fdc080f |
|
@khwilliamson - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#128598 (status was 'resolved')
Searchable as RT128598$
The text was updated successfully, but these errors were encountered: