Skip to content
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

[PATCH] Configure leaves garbage in $Config{longdblinfbytes} #15725

Closed
p5pRT opened this issue Nov 18, 2016 · 18 comments
Closed

[PATCH] Configure leaves garbage in $Config{longdblinfbytes} #15725

p5pRT opened this issue Nov 18, 2016 · 18 comments

Comments

@p5pRT
Copy link

p5pRT commented Nov 18, 2016

Migrated from rt.perl.org#130133 (status was 'resolved')

Searchable as RT130133$

@p5pRT
Copy link
Author

p5pRT commented Nov 18, 2016

From @ntyni

The Configure test for long double implementation details probes the
contents of "long double inf", and they get stored in the Config module​:

% perl -V​:longdblinfbytes
longdblinfbytes='0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, 0xff, 0x7f, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00';

Some of these bytes are currently essentially random and vary between
builds on at least amd64 and i386, because their long doubles are only
80-bit and the remaining six bytes stay uninitialized.

The relevant Configure probe tries to initialize these bytes, but has
several bugs that defeat the purpose. Patch attached.

The randomness seems to only have started to show on Debian with GCC-6
for one reason or another; the bytes are zeroed out on at least GCC-5.

Perl Info

Flags:
    category=core
    severity=low
    Type=Patch
    PatchStatus=HasPatch

Site configuration information for perl 5.25.7:

Configured by niko at Fri Nov 18 19:58:21 EET 2016.

Summary of my perl5 (revision 5 version 25 subversion 7) configuration:
  Commit id: 97eaa8936166129d88f778562e587a9151ce15b7
  Platform:
    osname=linux
    osvers=4.8.0-1-amd64
    archname=x86_64-linux
    uname='linux estella 4.8.0-1-amd64 #1 smp debian 4.8.5-1 (2016-10-28) x86_64 gnulinux '
    config_args='-des -Dusedevel'
    hint=recommended
    useposix=true
    d_sigaction=define
    useithreads=undef
    usemultiplicity=undef
    use64bitint=define
    use64bitall=define
    uselongdouble=undef
    usemymalloc=n
    bincompat5005=undef
  Compiler:
    cc='cc'
    ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'
    optimize='-O2'
    cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
    ccversion=''
    gccversion='6.2.0 20161103'
    gccosandvers=''
    intsize=4
    longsize=8
    ptrsize=8
    doublesize=8
    byteorder=12345678
    doublekind=3
    d_longlong=define
    longlongsize=8
    d_longdbl=define
    longdblsize=16
    longdblkind=3
    ivtype='long'
    ivsize=8
    nvtype='double'
    nvsize=8
    Off_t='off_t'
    lseeksize=8
    alignbytes=8
    prototype=define
  Linker and Libraries:
    ld='cc'
    ldflags =' -fstack-protector-strong -L/usr/local/lib'
    libpth=/usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/6/include-fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib
    libs=-lpthread -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
    perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
    libc=libc-2.24.so
    so=so
    useshrplib=false
    libperl=libperl.a
    gnulibc_version='2.24'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs
    dlext=so
    d_dlsymun=undef
    ccdlflags='-Wl,-E'
    cccdlflags='-fPIC'
    lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector-strong'



@INC for perl 5.25.7:
    lib
    /usr/local/lib/perl5/site_perl/5.25.7/x86_64-linux
    /usr/local/lib/perl5/site_perl/5.25.7
    /usr/local/lib/perl5/5.25.7/x86_64-linux
    /usr/local/lib/perl5/5.25.7


Environment for perl 5.25.7:
    HOME=/home/niko
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LC_CTYPE=fi_FI.UTF-8
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/niko/bin:/home/niko/bin:/home/niko/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games:/sbin:/usr/sbin:/sbin:/usr/sbin
    PERL_BADLANG (unset)
    SHELL=/bin/zsh

@p5pRT
Copy link
Author

p5pRT commented Nov 18, 2016

From @ntyni

0001-Configure-fix-garbage-filtering-with-80-bit-long-dou.patch
From 70b5974daa4281a98ab22ba2cadc1801088e2e75 Mon Sep 17 00:00:00 2001
From: Niko Tyni <ntyni@debian.org>
Date: Fri, 18 Nov 2016 18:36:34 +0200
Subject: [PATCH] Configure: fix garbage filtering with 80-bit long doubles

The test had several problems that resulted in the excess
bytes not getting zeroed out. This caused random contents in
$Config{longdblinfbytes}, observed on Debian with GCC 6.2.0 (but not
5.4.1).

Bug-Debian: https://bugs.debian.org/844752
---
 Configure | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/Configure b/Configure
index 6a3d617..f561a87 100755
--- a/Configure
+++ b/Configure
@@ -20661,8 +20661,8 @@ $cat >try.c <<EOP
 #define DOUBLESIZE $doublesize
 #$d_longdbl HAS_LONG_DOUBLE
 #ifdef HAS_LONG_DOUBLE
-#define LONGDBLSIZE $longdblsize
-#define LONGDBLKIND $longdblkind
+#define LONG_DOUBLESIZE $longdblsize
+#define LONG_DOUBLEKIND $longdblkind
 #endif
 #$i_math I_MATH
 #ifdef I_MATH
@@ -20699,16 +20699,14 @@ int main(int argc, char *argv[]) {
 #ifdef HAS_LONG_DOUBLE
    long double ldinf = (long double)exp(1e9);
    long double ldnan = (long double)sqrt(-1.0);
-#endif
-  if (argc == 2) {
-    switch (argv[1][0]) {
-    case '1': bytes(&dinf, sizeof(dinf)); break;
-    case '2': bytes(&dnan, sizeof(dnan)); break;
-#ifdef HAS_LONG_DOUBLE
 # if LONG_DOUBLEKIND == 3 || LONG_DOUBLEKIND == 4
 /* the 80-bit long doubles might have garbage in their excess bytes */
     memset((char *)&ldinf + 10, '\0', LONG_DOUBLESIZE - 10);
 # endif
+  if (argc == 2) {
+    switch (argv[1][0]) {
+    case '1': bytes(&dinf, sizeof(dinf)); break;
+    case '2': bytes(&dnan, sizeof(dnan)); break;
     case '3': bytes(&ldinf, sizeof(ldinf)); break;
     case '4': bytes(&ldnan, sizeof(ldnan)); break;
 #endif
-- 
2.10.2

@p5pRT
Copy link
Author

p5pRT commented Nov 18, 2016

From @jkeenan

On Fri, 18 Nov 2016 19​:18​:07 GMT, ntyni@​debian.org wrote​:

This is a bug report for perl from Niko Tyni <ntyni@​debian.org>,
generated with the help of perlbug 1.40 running under perl 5.25.7.

-----------------------------------------------------------------
[Please describe your issue here]

The Configure test for long double implementation details probes the
contents of "long double inf", and they get stored in the Config
module​:

% perl -V​:longdblinfbytes
longdblinfbytes='0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, 0xff,
0x7f, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00';

Some of these bytes are currently essentially random and vary between
builds on at least amd64 and i386, because their long doubles are only
80-bit and the remaining six bytes stay uninitialized.

The relevant Configure probe tries to initialize these bytes, but has
several bugs that defeat the purpose. Patch attached.

The randomness seems to only have started to show on Debian with GCC-6
for one reason or another; the bytes are zeroed out on at least GCC-5.

Do you expect that the impact of this will only show up if someone is using 'gcc' rather than, say, g++ or clang?

I ask because I suspect we'll have to hunt up people to test this on multiple versions of multiple C compilers.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Nov 18, 2016

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Nov 18, 2016

From @jkeenan

On Fri, 18 Nov 2016 21​:43​:46 GMT, jkeenan wrote​:

On Fri, 18 Nov 2016 19​:18​:07 GMT, ntyni@​debian.org wrote​:

This is a bug report for perl from Niko Tyni <ntyni@​debian.org>,
generated with the help of perlbug 1.40 running under perl 5.25.7.

-----------------------------------------------------------------
[Please describe your issue here]

The Configure test for long double implementation details probes the
contents of "long double inf", and they get stored in the Config
module​:

% perl -V​:longdblinfbytes
longdblinfbytes='0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80,
0xff,
0x7f, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00';

Some of these bytes are currently essentially random and vary between
builds on at least amd64 and i386, because their long doubles are
only
80-bit and the remaining six bytes stay uninitialized.

The relevant Configure probe tries to initialize these bytes, but has
several bugs that defeat the purpose. Patch attached.

The randomness seems to only have started to show on Debian with GCC-
6
for one reason or another; the bytes are zeroed out on at least GCC-
5.

Do you expect that the impact of this will only show up if someone is
using 'gcc' rather than, say, g++ or clang?

I ask because I suspect we'll have to hunt up people to test this on
multiple versions of multiple C compilers.

Thank you very much.

FWIW, here's the diff I got when applying your patch to blead using this 'gcc'​:

#####
gcc --version
gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609
#####
$ diff before.txt after.txt
1c1
< longdblinfbytes='0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, 0xff, 0x7f, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00';


longdblinfbytes='0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, 0xff, 0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00';
#####

Is that what you would expect?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Nov 18, 2016

From pipcet@gmail.com

That patch fixes things for ldinf, but doesn't appear to do anything for ldnan.

Instead of memsetting after the fact, we could clear an area of memory
before storing the (long) doubles there, that would catch any future
formats with undefined bytes, too.

And maybe Configure C programs should be compiled with warnings
enabled to catch other bugs like this (if I'm looking at the code
right, it's unreachable code in a switch statement, which I think gcc
warns about.)

@p5pRT
Copy link
Author

p5pRT commented Nov 18, 2016

From @tonycoz

On Fri, Nov 18, 2016 at 10​:23​:10PM +0000, Pip Cet wrote​:

Instead of memsetting after the fact, we could clear an area of memory
before storing the (long) doubles there, that would catch any future
formats with undefined bytes, too.

That isn't reliable.

While we might hope that the compiler generates code that stores
directly from the FPU into our pristine buffer, depending on the
compiler and optimization level it may end up making extra copies,
including random rubbish from other buffers.

See​:

https://rt-archive.perl.org/perl5/Ticket/Display.html?id=123971

for a case where this bit us.

Tony

@p5pRT
Copy link
Author

p5pRT commented Nov 19, 2016

From @ntyni

On Fri, Nov 18, 2016 at 01​:59​:35PM -0800, James E Keenan via RT wrote​:

Do you expect that the impact of this will only show up if someone is
using 'gcc' rather than, say, g++ or clang?

My guess is that the garbage doesn't have any functional impact.
The bytes seem to end up in PL_inf when long doubles are used, and I'd
expect any value with the right 80 bits will do for infinity there.

Perhaps Jarkko (cc'd) knows more? I think he's the long double expert
here :)

I noticed this because the Config.pm contents started to vary between
builds, breaking build reproducibility. We're not currently even using
long doubles in Debian, but these things apparently get probed for
regardless of that.

FWIW, here's the diff I got when applying your patch to blead using this 'gcc'​:

#####
gcc --version
gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609
#####
$ diff before.txt after.txt
1c1
< longdblinfbytes='0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, 0xff, 0x7f, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00';
---

longdblinfbytes='0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, 0xff, 0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00';
#####

Is that what you would expect?

Yes; all of the last six bytes now get zeroed out like the code
seems to intend.
--
Niko Tyni ntyni@​debian.org

@p5pRT
Copy link
Author

p5pRT commented Nov 19, 2016

From @ntyni

On Fri, Nov 18, 2016 at 02​:24​:23PM -0800, Pip Cet via RT wrote​:

That patch fixes things for ldinf, but doesn't appear to do anything for ldnan.

I only patched the obvious bugs. I don't see randomness (or even non-zero
high bytes) in ldnan here so I didn't touch that part.

But yes, ldnan should probably get the same treatment. Patch attached.
--
Niko

@p5pRT
Copy link
Author

p5pRT commented Nov 19, 2016

From @ntyni

0001-Configure-also-zero-out-high-bytes-of-80-bit-ldnan.patch
From a45661a63a22630ba3578bc3854f3a2446a28c84 Mon Sep 17 00:00:00 2001
From: Niko Tyni <ntyni@debian.org>
Date: Sat, 19 Nov 2016 09:45:45 +0200
Subject: [PATCH] Configure: also zero out high bytes of 80-bit ldnan

These are currently zero anyway, but things are probably not guaranteed
to stay so.
---
 Configure | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Configure b/Configure
index f561a87..845fc43 100755
--- a/Configure
+++ b/Configure
@@ -20702,6 +20702,7 @@ int main(int argc, char *argv[]) {
 # if LONG_DOUBLEKIND == 3 || LONG_DOUBLEKIND == 4
 /* the 80-bit long doubles might have garbage in their excess bytes */
     memset((char *)&ldinf + 10, '\0', LONG_DOUBLESIZE - 10);
+    memset((char *)&ldnan + 10, '\0', LONG_DOUBLESIZE - 10);
 # endif
   if (argc == 2) {
     switch (argv[1][0]) {
-- 
2.10.2

@p5pRT
Copy link
Author

p5pRT commented Nov 19, 2016

From @ntyni

In-Reply-To​: <rt-4.0.24-6473-1479505426-193.130133-94-0@​perl.org>

On Fri, Nov 18, 2016 at 01​:43​:46PM -0800, James E Keenan via RT wrote​:

Do you expect that the impact of this will only show up if someone is using 'gcc' rather than, say, g++ or clang?

I realize I probably didn't answer your question. Sorry.

I've only tried with gcc 5 and 6 from Debian unstable. I'd expect g++
to be the same. No idea about clang.

As for seeing the impact​: with gcc 6 on Debian unstable/amd64 (=x86_64),
where the randomness started to show up here, manually running the
unpatched probe extracted from Configure gives​:

% for i in $(seq 1 5); do ./a.out 3; done
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, 0xff, 0x7f, 0x82, 0x83, 0x53, 0x56, 0x00, 0x00
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, 0xff, 0x7f, 0x8c, 0xe8, 0x9f, 0x55, 0x00, 0x00
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, 0xff, 0x7f, 0x0f, 0x99, 0x8f, 0x55, 0x00, 0x00
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, 0xff, 0x7f, 0xe9, 0x86, 0x8f, 0x55, 0x00, 0x00
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, 0xff, 0x7f, 0xcd, 0x64, 0xb3, 0x55, 0x00, 0x00

while the patched one consistently gives
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, 0xff, 0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00

I'm attaching the (buggy) probe program for convenience.

gcc is 'gcc version 6.2.0 20161103 (Debian 6.2.0-11)'.
--
Niko Tyni ntyni@​debian.org

@p5pRT
Copy link
Author

p5pRT commented Nov 19, 2016

From @ntyni

#define DOUBLESIZE 8
#define HAS_LONG_DOUBLE
#ifdef HAS_LONG_DOUBLE
#define LONG_DOUBLESIZE 16
#define LONG_DOUBLEKIND 3
#endif
#define I_MATH
#ifdef I_MATH
#include <math.h>
#endif
#include <stdio.h>
#include <string.h>
/* Note that whether the sign bit is on or off
 * for NaN depends on the CPU/FPU, and possibly
 * can be affected by the build toolchain.
 *
 * For example for older MIPS and HP-PA 2.0 the quiet NaN is:
 * 0x7f, 0xf7, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff
 * 0x7f, 0xf4, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
 * (respectively) as opposed to the more usual
 * 0x7f, 0xf8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
 */
static void bytes(unsigned char *p, unsigned int n) {
  int i;
  for (i = 0; i < n; i++) {
    printf("0x%02x%s", p[i], i < n - 1 ? ", " : "\n");
  }
}
int main(int argc, char *argv[]) {
   /* We cannot use 1.0/0.0 and 0.0/0.0 (with L suffixes for long double)
    * because some compilers are 'smart' and not only warn but refuse to
    * compile such 'illegal' values. */
   double dinf = exp(1e9);
   double dnan = sqrt(-1.0);
#ifdef HAS_LONG_DOUBLE
   long double ldinf = (long double)exp(1e9);
   long double ldnan = (long double)sqrt(-1.0);
#endif
  if (argc == 2) {
    switch (argv[1][0]) {
    case '1': bytes(&dinf, sizeof(dinf)); break;
    case '2': bytes(&dnan, sizeof(dnan)); break;
#ifdef HAS_LONG_DOUBLE
# if LONG_DOUBLEKIND == 3 || LONG_DOUBLEKIND == 4
/* the 80-bit long doubles might have garbage in their excess bytes */
    memset((char *)&ldinf + 10, '\0', LONG_DOUBLESIZE - 10);
# endif
    case '3': bytes(&ldinf, sizeof(ldinf)); break;
    case '4': bytes(&ldnan, sizeof(ldnan)); break;
#endif
    }
  }
  return 0;
}

@p5pRT
Copy link
Author

p5pRT commented Nov 19, 2016

From @jhi

Perhaps Jarkko (cc'd) knows more? I think he's the long double expert
here :)

Yeah, beyond the 80 bits there might be any random garbage, and the
random bits might appear there at any point. I am guessing at the
discretion of whatever the compiler thinks work best (most probably
fast-est) at any given time those bytes are looked at / manipulated.
And the garbage doesn't affect the long double computations.

(FWIW, I think there have been in the past x86-platform compilers
where the long doubles have been 12 bytes, on 32-bit platforms,
so those had only two garbage bytes.)

Perhaps Jarkko (cc'd) knows more? I think he's the long double expert
here

Didn't stop me getting the long doubles probing code wrong, though.

I noticed this because the Config.pm contents started to vary between
builds, breaking build reproducibility. We're not currently even using
long doubles in Debian, but these things apparently get probed for
regardless of that.

@p5pRT
Copy link
Author

p5pRT commented Nov 19, 2016

From pipcet@gmail.com

On Sat, Nov 19, 2016 at 7​:51 AM, Niko Tyni <ntyni@​debian.org> wrote​:

On Fri, Nov 18, 2016 at 02​:24​:23PM -0800, Pip Cet via RT wrote​:

That patch fixes things for ldinf, but doesn't appear to do anything for ldnan.
I only patched the obvious bugs. I don't see randomness (or even non-zero
high bytes) in ldnan here so I didn't touch that part.

I think that's just luck, though. The code generated by gcc here just
leaves the high six bytes alone, so it's whatever was on the stack
when main() was called—which doesn't just depend on the compiler, it
depends on the precise build of libc and any preload libraries. glibc,
at least, does quite a few things before finally calling main().

But yes, ldnan should probably get the same treatment. Patch attached.

I think that's the best we can do, given the rather unruly way some
compilers appear to behave.

@p5pRT
Copy link
Author

p5pRT commented Nov 22, 2016

From @tonycoz

On Fri, 18 Nov 2016 23​:52​:12 -0800, ntyni@​debian.org wrote​:

On Fri, Nov 18, 2016 at 02​:24​:23PM -0800, Pip Cet via RT wrote​:

That patch fixes things for ldinf, but doesn't appear to do anything
for ldnan.

I only patched the obvious bugs. I don't see randomness (or even non-
zero
high bytes) in ldnan here so I didn't touch that part.

But yes, ldnan should probably get the same treatment. Patch attached.

Thanks, applied as dd68853 and 6b2c747.

Tony

@p5pRT
Copy link
Author

p5pRT commented Nov 22, 2016

@tonycoz - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Author

p5pRT commented May 30, 2017

From @khwilliamson

Thank you for filing this report. You have helped make Perl better.

With the release today of Perl 5.26.0, this and 210 other issues have been
resolved.

Perl 5.26.0 may be downloaded via​:
https://metacpan.org/release/XSAWYERX/perl-5.26.0

If you find that the problem persists, feel free to reopen this ticket.

@p5pRT
Copy link
Author

p5pRT commented May 30, 2017

@khwilliamson - Status changed from 'pending release' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant