Bug#772718: adns: test failure on Ubuntu ppc64el, with -O3

[Jeremy Bicha]

Ian Jackson  wrote:
> I don't intend (with my Debian hat on) to upload this
> to Debian right now since
> - Debian is frozen for jessie
> - The actual library and binaries are fine
> - AFAIAA the test suite failure does not affect Debian itself
>
> When Debian unfreezes, I will fix this with a new upstream version.

Ping!

Thanks,
Jeremy Bicha

Bug#772718: adns: test failure on Ubuntu ppc64el, with -O3

[Colin Watson]

On Wed, Dec 10, 2014 at 11:21:13PM +, Ian Jackson wrote:
 The attached patch should fix this.  It is in the upstream git and
 will be in the next upstream adns release.  Indeed I may bring that
 release forward.
 
 I don't intend (with my Debian hat on) to upload this to Debian right
 now since
  - Debian is frozen for jessie
  - The actual library and binaries are fine
  - AFAIAA the test suite failure does not affect Debian itself
 
 When Debian unfreezes, I will fix this with a new upstream version.

Thanks!  I've cherry-picked this patch into Ubuntu for now.

 BTW, building with -O3 by default seems a bit mad.  But IMO it ought
 to work, so this is a real bug.

Well, yes, I agree and argued against it, but ...

-- 
Colin Watson   [cjwat...@ubuntu.com]


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#772718: adns: test failure on Ubuntu ppc64el, with -O3

[Colin Watson]

Package: adns
Version: 1.5.0~rc1-1
Severity: normal
User: ubuntu-de...@lists.ubuntu.com
Usertags: origin-ubuntu vivid

adns 1.5.0~rc1-1 fails to build on Ubuntu ppc64el; 1.4-2ubuntu1 (which
was modified to use dh-autoreconf in order to build on newer
architectures;
http://launchpadlibrarian.net/126377262/adns_1.4-2build2_1.4-2ubuntu1.diff.gz)
built cleanly.  Here's the build log:

  
https://launchpadlibrarian.net/191985562/buildlog_ubuntu-vivid-ppc64el.adns_1.5.0~rc1-1_FAILEDTOBUILD.txt.gz

To reproduce this it appears to be sufficient to build with -O3.  Feel
free to use the cjwatson-adns session on pastel.debian.net to
reproduce this; DEB_CFLAGS_APPEND=-O3 debian/rules build in an
unpacked copy of the latest adns source package does the job.

Thanks,

-- 
Colin Watson   [cjwat...@ubuntu.com]


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#772718: adns: test failure on Ubuntu ppc64el, with -O3

[Ian Jackson]

Colin Watson writes (Bug#772718: adns: test failure on Ubuntu ppc64el, with 
-O3):
 To reproduce this it appears to be sufficient to build with -O3.  Feel
 free to use the cjwatson-adns session on pastel.debian.net to
 reproduce this; DEB_CFLAGS_APPEND=-O3 debian/rules build in an
 unpacked copy of the latest adns source package does the job.

Thanks.  I have repro'd this and traced the root cause.

The problem is a malicious optimisation of the following function in
the test suite:

  void *Hrealloc(void *op, size_t nsz) {
struct malloced *oldnode;
void *np;
size_t osz;

if (op) {
  oldnode= (void*)((char*)op - MALLOCHSZ); osz= oldnode-sz;
} else {
  osz= 0;
 }
np= Hmalloc(nsz);
memcpy(np,op, osznsz ? nsz : osz);
Hfree(op);
return np;
  }

The standards bodies have decided that (contrary to traditional
practice) memcpy(0,0,0) is UB.  Hence, GCC reasons as follows:

  If !op, we still call memcpy(np,op,0). 
  Which is UB.
  Therefore !!op.
  Therefore I don't need to actually compile the !op branches.

I suspect that this only shows up with -O3 because in that case a copy
of Hfree is inlined into Hrealloc.  (gcc evidently fails to remove the
also wrongfully-provably-unnecessary osz=0 branch, since
osz=oldnode-sz would crash.)

With the debugging printf patch below, and building upstream adns
1.5.0 with gcc 4.9.1-19 on i386 and with XCFLAGS='-O3
-fno-builtin-cpp', I see this output:

 Hmalloc 8 - 0x8299018 (node=0x8299008 next=(nil) back=(nil) count=1)
 Hmalloc 16 - 0x8299038 (node=0x8299028 next=(nil) back=0x8299008 count=2)
 Hmalloc 1452 - 0x8299060 (node=0x8299050 next=(nil) back=0x8299028 count=3)
 Hmalloc 8 - 0x8299620 (node=0x8299610 next=(nil) back=0x8299050 count=4)
 Hmalloc 41 - 0x8299640 (node=0x8299630 next=(nil) back=0x8299610 count=5)
 Hfree((nil))
 Hmalloc 40 - 0x8299b98 (node=0x8299b88 next=(nil) back=0x8299630 count=6)
 Hrealloc((nil), 40) - 0x8299b98
 Hfree((nil))
 Hfree((nil)) (node=0xfff0)
 Segmentation fault (core dumped)

Notice that in Hfree, which looks like this:

  void Hfree(void *ptr) {
struct malloced *oldnode;
  fprintf(stderr,Hfree(%p)\n,ptr);
if (!ptr) return;
oldnode= (void*)((char*)ptr - MALLOCHSZ);
  fprintf(stderr,Hfree(%p) (node=%p)\n,ptr, oldnode);
  fprintf(stderr,Hfree(%p) (node=%p next=%p back=%p count=%lu)\n,ptr,
oldnode, 
oldnode-next, oldnode-back,

The if (!ptr) check is removed.  gdb shows this stack trace:

  #0  0x0804e129 in Hrealloc () at hcommon.c:278
  #1  0x080595c9 in adns__vbuf_appendstr ()
  #2  0x0804d1c6 in Qsocket () at hcommon.c:209
  #3  0x0804b0c4 in Hsocket ()
  #4  0x0805c21e in init_finish () at ../src/setup.c:670
  #5  0x0805cbba in adns_init_strcfg () at ../src/setup.c:761
  #6  0x0804909a in main () at ../client/adnstest.c:244

The fix is obvious and will follow in my next email.

Ian.


diff --git a/regress/hcommon.c b/regress/hcommon.c
index ebbef94..d59104b 100644
--- a/regress/hcommon.c
+++ b/regress/hcommon.c
@@ -265,12 +265,20 @@ void *Hmalloc(size_t sz) {
   }
   assert(newnode-count != mallocfailat);
   memset(newnode-data,0xc7,sz);
+fprintf(stderr,Hmalloc %lu - %p (node=%p next=%p back=%p count=%lu)\n,
+   (unsigned long)sz,newnode-data,
+   newnode, newnode-next, newnode-back, newnode-count);
   return newnode-data;
 }
 void Hfree(void *ptr) {
   struct malloced *oldnode;
+fprintf(stderr,Hfree(%p)\n,ptr);
   if (!ptr) return;
   oldnode= (void*)((char*)ptr - MALLOCHSZ);
+fprintf(stderr,Hfree(%p) (node=%p)\n,ptr, oldnode);
+fprintf(stderr,Hfree(%p) (node=%p next=%p back=%p count=%lu)\n,ptr,
+ oldnode, 
+ oldnode-next, oldnode-back, oldnode-count);
   LIST_UNLINK(mallocedlist,oldnode);
   memset(oldnode-data,0x38,oldnode-sz);
   free(oldnode);
@@ -281,6 +289,7 @@ void *Hrealloc(void *op, size_t nsz) {
   size_t osz;
   if (op) { oldnode= (void*)((char*)op - MALLOCHSZ); osz= oldnode-sz; } else 
{ osz= 0; }
   np= Hmalloc(nsz);
+fprintf(stderr,Hrealloc(%p, %lu) - %p\n, op, (unsigned long)nsz, np);
   memcpy(np,op, osznsz ? nsz : osz);
   Hfree(op);
   return np;
diff --git a/regress/hcommon.c.m4 b/regress/hcommon.c.m4
index c5069ee..3290596 100644
--- a/regress/hcommon.c.m4
+++ b/regress/hcommon.c.m4
@@ -280,15 +280,28 @@ void *Hmalloc(size_t sz) {
   }
   assert(newnode-count != mallocfailat);
   memset(newnode-data,0xc7,sz);
+
+fprintf(stderr,Hmalloc %lu - %p (node=%p next=%p back=%p count=%lu)\n,
+   (unsigned long)sz,newnode-data,
+   newnode, newnode-next, newnode-back, newnode-count);
+
   return newnode-data;
 }
 
 void Hfree(void *ptr) {
   struct malloced *oldnode;
 
+fprintf(stderr,Hfree(%p)\n,ptr);
+
   if (!ptr) return;
 
   oldnode= (void*)((char*)ptr - MALLOCHSZ);
+
+fprintf(stderr,Hfree(%p) (node=%p)\n,ptr, oldnode);
+fprintf(stderr,Hfree(%p) (node=%p next=%p back=%p count=%lu

Bug#772718: adns: test failure on Ubuntu ppc64el, with -O3

[Ian Jackson]

Control: tags -1 + patch

Colin Watson writes (Bug#772718: adns: test failure on Ubuntu ppc64el, with 
-O3):
 adns 1.5.0~rc1-1 fails to build on Ubuntu ppc64el; 1.4-2ubuntu1 (which
 was modified to use dh-autoreconf in order to build on newer
 architectures;

The attached patch should fix this.  It is in the upstream git and
will be in the next upstream adns release.  Indeed I may bring that
release forward.

I don't intend (with my Debian hat on) to upload this to Debian right
now since
 - Debian is frozen for jessie
 - The actual library and binaries are fine
 - AFAIAA the test suite failure does not affect Debian itself

When Debian unfreezes, I will fix this with a new upstream version.


BTW, building with -O3 by default seems a bit mad.  But IMO it ought
to work, so this is a real bug.

Thanks,
Ian.


commit d8fa191ed7774818862febd6ade774cb7e149ab9
Author: Ian Jackson ijack...@chiark.greenend.org.uk
Date:   Wed Dec 10 23:16:37 2014 +

Fix for malicious optimisation of memcpy in test suite, which causes 
failure with gcc-4.1.9 -O3.  See Debian bug #772718.

diff --git a/changelog b/changelog
index a3e7dbb..fedd31a 100644
--- a/changelog
+++ b/changelog
@@ -1,6 +1,8 @@
 adns (1.5.1~~) unstable; urgency=low
 
   * Portability fix for systems where socklen_t is bigger than int.
+  * Fix for malicious optimisation of memcpy in test suite, which
+causes failure with gcc-4.1.9 -O3.  See Debian bug #772718.
 
  --
 
diff --git a/regress/hcommon.c b/regress/hcommon.c
index ebbef94..e63a8cd 100644
--- a/regress/hcommon.c
+++ b/regress/hcommon.c
@@ -281,7 +281,7 @@ void *Hrealloc(void *op, size_t nsz) {
   size_t osz;
   if (op) { oldnode= (void*)((char*)op - MALLOCHSZ); osz= oldnode-sz; } else 
{ osz= 0; }
   np= Hmalloc(nsz);
-  memcpy(np,op, osznsz ? nsz : osz);
+  if (osz) memcpy(np,op, osznsz ? nsz : osz);
   Hfree(op);
   return np;
 }
diff --git a/regress/hcommon.c.m4 b/regress/hcommon.c.m4
index c5069ee..330da4d 100644
--- a/regress/hcommon.c.m4
+++ b/regress/hcommon.c.m4
@@ -301,7 +301,7 @@ void *Hrealloc(void *op, size_t nsz) {
 
   if (op) { oldnode= (void*)((char*)op - MALLOCHSZ); osz= oldnode-sz; } else 
{ osz= 0; }
   np= Hmalloc(nsz);
-  memcpy(np,op, osznsz ? nsz : osz);
+  if (osz) memcpy(np,op, osznsz ? nsz : osz);
   Hfree(op);
   return np;
 }

-- 


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org