[libvirt] [PATCH 2/2] Revert "Prevent more compiler optimization of mockable functions"

[Martin Kletzander]

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.
>>
>>Martin
>
>I 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170712/56a54f7c/attachment-0001.sig>