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

Martin Kletzander mkletzan at redhat.com
Thu Jul 13 11:30:43 UTC 2017


On Thu, Jul 13, 2017 at 12:28:41PM +0100, Daniel P. Berrange wrote:
>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.
>
>FYI, I intend to push these revert patches, so that virtlogd stops
>crashing to unblock other devs/users, while we focus on writing &
>reviewing the new approach we've discussed
>

Sure, ACK to that.
-------------- 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/20170713/537c3a5b/attachment-0001.sig>


More information about the libvir-list mailing list