On Wed, Jul 12, 2017 at 01:49:04PM +0200, Martin Kletzander wrote:
On Wed, Jul 12, 2017 at 01:10:08PM +0200, Martin Kletzander wrote:On Wed, Jul 12, 2017 at 11:14:16AM +0100, Daniel P. Berrange wrote:This reverts commit e4b980c853d2114b25fa805a84ea288384416221. When a binary links against a .a archive (as opposed to a shared library), any symbols which are marked as 'weak' get silently dropped. As a result when the binary later runs, those 'weak' functions have an address of 0x0 and thus crash when run. This happened with virtlogd and virtlockd because they don't link to libvirt.so, but instead just libvirt_util.a and libvirt_rpc.a. The virRandomBits symbols was weak and so left out of the virtlogd & virtlockd binaries, despite being required by virHashTable functions. Various other binaries like libvirt_lxc, libvirt_iohelper, etc also link directly to .a files instead of libvirt.so, so are potentially at risk of dropping symbols leading to a later runtime crash. This is normal linker behaviour because a weak symbol is not treated as undefined, so nothing forces it to be pulled in from the .a You have to force the linker to pull in weak symbols using -u$SYMNAME which is not a practical approach.How is this done by gnulib (or libc) when most their functions are weak aliases anyway? Can't we use the same approach they have? virtlo{g,ck}d link with libgnu.la as well and there is no problem with that, right? So I guess this _must_ be solvable somehow, IMHO. I'm just curious how that works. MartinI guess we would have to do something like the following, but for every function. diff --git i/src/util/virrandom.c w/src/util/virrandom.c index 41daa404b273..3d9fe7f85d97 100644 --- i/src/util/virrandom.c +++ w/src/util/virrandom.c @@ -99,7 +99,7 @@ VIR_ONCE_GLOBAL_INIT(virRandom) * * Return: a random number with @nbits entropy */ -uint64_t virRandomBits(int nbits) +static uint64_t __virRandomBits(int nbits) { uint64_t ret = 0; int32_t bits; @@ -125,6 +125,7 @@ uint64_t virRandomBits(int nbits) virMutexUnlock(&randomLock); return ret; } +uint64_t virRandomBits(int nbits) ATTRIBUTE_MOCKABLE __attribute__((alias("__virRandomBits"))); /** diff --git i/src/util/virrandom.h w/src/util/virrandom.h index 990a456addf7..abda95aef506 100644 --- i/src/util/virrandom.h +++ w/src/util/virrandom.h @@ -24,7 +24,7 @@ # include "internal.h" -uint64_t virRandomBits(int nbits) ATTRIBUTE_MOCKABLE; +uint64_t virRandomBits(int nbits); double virRandom(void); uint32_t virRandomInt(uint32_t max); int virRandomBytes(unsigned char *buf, size_t buflen) -- And of course that could be macrofied so that ATTRIBUTE_MOCKABLE takes function or something, etc. I like this more than reverting the patches.
Also, having the weak alias we can drop all the mocks and the problems with them and just redefine the functions we would like to mock in the tests (see tests/virfilewrapper.c), it would work on win32, it would not compile if we would forgot to make the function as an alias (so no need to check that in the syntax-check) and maybe provide other benefits that I can't think of right now.
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
