FTBFS on powerpc and armhf due to uninitialized struct

[Robie Basak]

See:

https://launchpadlibrarian.net/198709456/buildlog_ubuntu-vivid-powerpc.percona-galera-3_3.8-3390-0ubuntu2_BUILDING.txt.gz

and

https://launchpadlibrarian.net/198709796/buildlog_ubuntu-vivid-armhf.percona-galera-3_3.8-3390-0ubuntu2_BUILDING.txt.gz

[Robie Basak (racb)]

gcc -o galerautils/src/gu_rand.os -c -std=c99 -fno-strict-aliasing -pipe -g -O3 -DNDEBUG -Wall -Wextra -Wno-unused-parameter -pedantic -fPIC -D_FORTIFY_SOURCE=2 -pthread -D_XOPEN_SOURCE=600 -DHAVE_COMMON_H -DGALERA_USE_GU_NETWORK -DHAVE_BYTESWAP_H -DHAVE_ENDIAN_H -DHAVE_BOOST_SHARED_PTR_HPP -DHAVE_TR1_UNORDERED_MAP -DBOOST_DATE_TIME_POSIX_TIME_STD_CONFIG=1 -DHAVE_ASIO_HPP -DHAVE_ASIO_SSL_HPP -Werror -I. -Iasio -Icommon -Igalerautils/src -Igcomm/src -Igcomm/src/gcomm -Igcache/src -Igcs/src -Iwsdb/src -Igalera/src galerautils/src/gu_rand.c
In file included from galerautils/src/gu_hash.h:30:0,
                 from galerautils/src/gu_rand.c:15:
galerautils/src/gu_rand.c: In function 'gu_rand_seed_long':
galerautils/src/gu_mmh3.h:198:21: error: '*((void *)&rse+23)' is used uninitialized in this function [-Werror=uninitialized]
     case 8: k1 ^= ((uint64_t)tail[ 7]) << 56;
                     ^
galerautils/src/gu_rand.c:32:14: note: '*((void *)&rse+23)' was declared here
     gu_rse_t rse = { time, heap_ptr, &time, pid };
              ^
In file included from galerautils/src/gu_hash.h:30:0,
                 from galerautils/src/gu_rand.c:15:
galerautils/src/gu_mmh3.h:199:21: error: '*((void *)&rse+22)' is used uninitialized in this function [-Werror=uninitialized]
     case 7: k1 ^= ((uint64_t)tail[ 6]) << 48;
                     ^
galerautils/src/gu_rand.c:32:14: note: '*((void *)&rse+22)' was declared here
     gu_rse_t rse = { time, heap_ptr, &time, pid };
              ^
In file included from galerautils/src/gu_hash.h:30:0,
                 from galerautils/src/gu_rand.c:15:
galerautils/src/gu_mmh3.h:200:21: error: '*((void *)&rse+21)' is used uninitialized in this function [-Werror=uninitialized]
     case 6: k1 ^= ((uint64_t)tail[ 5]) << 40;
                     ^
galerautils/src/gu_rand.c:32:14: note: '*((void *)&rse+21)' was declared here
     gu_rse_t rse = { time, heap_ptr, &time, pid };
              ^
In file included from galerautils/src/gu_hash.h:30:0,
                 from galerautils/src/gu_rand.c:15:
galerautils/src/gu_mmh3.h:201:21: error: '*((void *)&rse+20)' is used uninitialized in this function [-Werror=uninitialized]
     case 5: k1 ^= ((uint64_t)tail[ 4]) << 32;
                     ^
galerautils/src/gu_rand.c:32:14: note: '*((void *)&rse+20)' was declared here
     gu_rse_t rse = { time, heap_ptr, &time, pid };
              ^
cc1: all warnings being treated as errors

[Robie Basak (racb)]

My hypothesis:

On these architectures, struct gu_rse (defined at galerautils/src/gu_rand.c:19) has holes in it due to alignment. In function gu_rand_seed_long(galerautils/src/gu_rand.c:30), a struct of this type is initialised on a by-member basis. But its address is then passed to gu_fast_hash64_medium which reads it as a buffer. So any holes in that buffer remain uninitialized.

I presume that powerpc and armhf are the architectures where struct gu_rse end up with holes in it, and the other architectures do not. However, gcc is free to change this in the future, so I think this bug applies to all architectures, and a proper fix would not be architecture-specific.

Applying the following patch fixes the issue and so is consistent with my hypothesis:

--- percona-galera-3-3.8-3390.orig/galerautils/src/gu_rand.c
+++ percona-galera-3-3.8-3390/galerautils/src/gu_rand.c
@@ -16,7 +16,7 @@

 /*! Structure to hold entropy data.
  * Should be at least 20 bytes on 32-bit systems and 28 bytes on 64-bit */
-struct gu_rse
+struct __attribute__((__packed__)) gu_rse
 {
     long long time;
     const void* heap_ptr;

It would seem cleaner to me to memset(&rse, 0, sizeof(rse)) though. gcc would probably optimise the initialization fully, leaving architectures without the holes unchanged.

Can I have some feedback from upstream on this though please? Is this code being used for anything that needs to be cryptographically secure? Will you accept a memset fix, and will this have any unintended side-effects?

[Robie Basak (racb)]

The patch I'm proposing for upstream is attached. I've verified that this builds successfully on all arches on Vivid (i386, amd64, powerpc, ppc64el, armhf, arm64).

I would like a review from upstream though please, since I'm messing with the entropy that enters a random number generator by properly initalising the input, and I don't understand the uses of the random numbers that come out.

[George Ormond Lorch III (gl-az)]

Patch was reviewed by one of our (Percona) senior devs responsible for Galera/PXC:

"I reviewed the patch and it looks good as a short-term solution. As in, it is correct for Linux, it has no security implications, and it will not break interoperability with unpatched galera instances.
As a long-term solution I would rewrite code in gu_rand.c to not depend on any specific data layout in memory and thus, specific architecture details." - Alexey Kopytov

[Launchpad Janitor (janitor)]

This bug was fixed in the package percona-galera-3 - 3.8-3390-0ubuntu3

---------------
percona-galera-3 (3.8-3390-0ubuntu3) vivid; urgency=medium

  * Ubuntu upload from Github VCS commit 0ce363a for unreleased Debian
    changes.

percona-galera-3 (3.8-3390-1) UNRELEASED; urgency=medium

  [ George Ormond Lorch III ]
  * New upstream version.
  * Removed unnecessary line(S) from .lintian-overrides
  * d/p/fix_garbd_stop.patch: Addresses upstream issue where on package rm,
    an attempt is made to stop garbd even if it not running, resulting in an
    rm error.
  * d/p/fix_arm64_ftb.patch: Add in some architecture details for arm64 in
    chromium/build_config.h to allow basic compilation to proceed through
    macro tests.
  * d/p/fix_i386_ftb.patch: Fixed i386 builds attempting to use intrinsics
    for crc32. i386 should be using manual crc32 calculations and not
    non-existing instructions.

  [ James Page ]
  * Rename package inline with agreed package naming for Galera variants.
  * Bump debhelper compat level to 9.
  * Drop symlink for defaults file, no longer required.
  * Correct path to garb defaults file, refresh existing patches.
  * Enable support for systemd.

  [ Robie Basak ]
  * d/p/fix_non-intel_ftb.patch: fix FTBFS on non-Intel architectures.
  * d/p/gu_rand-struct-holes.patch: correctly initialise gu_rse_t struct
    (LP: #1430874).
 -- Robie Basak <email address hidden> Wed, 18 Mar 2015 15:56:44 +0000