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.
This risk is silent bad linkage that affects runtime behaviour is
not acceptable for a fix that was merely trying to fix the test
suite. So stop using __weak__ again.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
build-aux/mock-noinline.pl | 2 +-
src/check-symfile.pl | 2 +-
src/internal.h | 23 +++++------------------
src/qemu/qemu_capspriv.h | 2 +-
src/rpc/virnetsocket.h | 4 ++--
src/util/vircommand.h | 2 +-
src/util/vircrypto.h | 2 +-
src/util/virfile.h | 2 +-
src/util/virhostcpu.h | 4 ++--
src/util/virmacaddr.h | 2 +-
src/util/virnetdev.h | 8 ++++----
src/util/virnetdevip.h | 2 +-
src/util/virnetdevopenvswitch.h | 2 +-
src/util/virnetdevtap.h | 6 +++---
src/util/virnuma.h | 16 ++++++++--------
src/util/virrandom.h | 6 +++---
src/util/virscsi.h | 2 +-
src/util/virscsivhost.h | 2 +-
src/util/virtpm.h | 2 +-
src/util/virutil.h | 10 +++++-----
src/util/viruuid.h | 2 +-
21 files changed, 45 insertions(+), 58 deletions(-)
diff --git a/build-aux/mock-noinline.pl b/build-aux/mock-noinline.pl
index 2745d4b..eafe20d 100644
--- a/build-aux/mock-noinline.pl
+++ b/build-aux/mock-noinline.pl
@@ -43,7 +43,7 @@ sub scan_annotations {
} elsif (/^\s*$/) {
$func = undef;
}
- if (/ATTRIBUTE_MOCKABLE/) {
+ if (/ATTRIBUTE_NOINLINE/) {
if (defined $func) {
$noninlined{$func} = 1;
}
diff --git a/src/check-symfile.pl b/src/check-symfile.pl
index 3b062d0..d59a213 100755
--- a/src/check-symfile.pl
+++ b/src/check-symfile.pl
@@ -52,7 +52,7 @@ foreach my $elflib (@elflibs) {
open NM, "-|", "nm", $elflib or die "cannot run 'nm $elflib': $!";
while (<NM>) {
- next unless /^\S+\s(?:[TBDW])\s(\S+)\s*$/;
+ next unless /^\S+\s(?:[TBD])\s(\S+)\s*$/;
$gotsyms{$1} = 1;
}
diff --git a/src/internal.h b/src/internal.h
index 00edd4f..c29f20f 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -113,26 +113,13 @@
# endif
/**
- * ATTRIBUTE_MOCKABLE:
- *
- * Ensure that the symbol can be overridden in a mock
- * library preload. This implies a number of attributes
- *
- * - noinline: prevents the body being inlined to
- * callers,
- * - noclone: prevents specialized copies of the
- * function body being created for different
- * callers
- * - weak: prevents the compiler making optimizations
- * such as constant return value propagation
+ * ATTRIBUTE_NOINLINE:
*
+ * Force compiler not to inline a method. Should be used if
+ * the method need to be overridable by test mocks.
*/
-# ifndef ATTRIBUTE_MOCKABLE
-# if __GNUC_PREREQ(4, 5)
-# define ATTRIBUTE_MOCKABLE __attribute__((__noinline__, __noclone__, __weak__))
-# else
-# define ATTRIBUTE_MOCKABLE __attribute__((__noinline__, __weak__))
-# endif
+# ifndef ATTRIBUTE_NOINLINE
+# define ATTRIBUTE_NOINLINE __attribute__((__noinline__))
# endif
/**
diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h
index 6cc189e..94fa75b 100644
--- a/src/qemu/qemu_capspriv.h
+++ b/src/qemu/qemu_capspriv.h
@@ -95,7 +95,7 @@ virQEMUCapsSetCPUModelInfo(virQEMUCapsPtr qemuCaps,
virCPUDefPtr
virQEMUCapsProbeHostCPUForEmulator(virCapsPtr caps,
virQEMUCapsPtr qemuCaps,
- virDomainVirtType type) ATTRIBUTE_MOCKABLE;
+ virDomainVirtType type) ATTRIBUTE_NOINLINE;
void
virQEMUCapsSetGICCapabilities(virQEMUCapsPtr qemuCaps,
diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h
index 3c2945e..de795af 100644
--- a/src/rpc/virnetsocket.h
+++ b/src/rpc/virnetsocket.h
@@ -137,10 +137,10 @@ int virNetSocketGetUNIXIdentity(virNetSocketPtr sock,
gid_t *gid,
pid_t *pid,
unsigned long long *timestamp)
- ATTRIBUTE_MOCKABLE;
+ ATTRIBUTE_NOINLINE;
int virNetSocketGetSELinuxContext(virNetSocketPtr sock,
char **context)
- ATTRIBUTE_MOCKABLE;
+ ATTRIBUTE_NOINLINE;
int virNetSocketSetBlocking(virNetSocketPtr sock,
bool blocking);
diff --git a/src/util/vircommand.h b/src/util/vircommand.h
index c042a53..e7c2e51 100644
--- a/src/util/vircommand.h
+++ b/src/util/vircommand.h
@@ -58,7 +58,7 @@ enum {
void virCommandPassFD(virCommandPtr cmd,
int fd,
- unsigned int flags) ATTRIBUTE_MOCKABLE;
+ unsigned int flags) ATTRIBUTE_NOINLINE;
void virCommandPassListenFDs(virCommandPtr cmd);
diff --git a/src/util/vircrypto.h b/src/util/vircrypto.h
index 50400c6..068602f 100644
--- a/src/util/vircrypto.h
+++ b/src/util/vircrypto.h
@@ -55,6 +55,6 @@ int virCryptoEncryptData(virCryptoCipher algorithm,
ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6)
ATTRIBUTE_NONNULL(8) ATTRIBUTE_NONNULL(9) ATTRIBUTE_RETURN_CHECK;
-uint8_t *virCryptoGenerateRandom(size_t nbytes) ATTRIBUTE_MOCKABLE;
+uint8_t *virCryptoGenerateRandom(size_t nbytes) ATTRIBUTE_NOINLINE;
#endif /* __VIR_CRYPTO_H__ */
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 32c9115..57ceb80 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -188,7 +188,7 @@ void virFileActivateDirOverride(const char *argv0)
off_t virFileLength(const char *path, int fd) ATTRIBUTE_NONNULL(1);
bool virFileIsDir (const char *file) ATTRIBUTE_NONNULL(1);
-bool virFileExists(const char *file) ATTRIBUTE_NONNULL(1) ATTRIBUTE_MOCKABLE;
+bool virFileExists(const char *file) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NOINLINE;
bool virFileIsExecutable(const char *file) ATTRIBUTE_NONNULL(1);
enum {
diff --git a/src/util/virhostcpu.h b/src/util/virhostcpu.h
index 3b30a0d..67033de 100644
--- a/src/util/virhostcpu.h
+++ b/src/util/virhostcpu.h
@@ -38,7 +38,7 @@ bool virHostCPUHasBitmap(void);
virBitmapPtr virHostCPUGetPresentBitmap(void);
virBitmapPtr virHostCPUGetOnlineBitmap(void);
int virHostCPUGetCount(void);
-int virHostCPUGetThreadsPerSubcore(virArch arch) ATTRIBUTE_MOCKABLE;
+int virHostCPUGetThreadsPerSubcore(virArch arch) ATTRIBUTE_NOINLINE;
int virHostCPUGetMap(unsigned char **cpumap,
unsigned int *online,
@@ -51,7 +51,7 @@ int virHostCPUGetInfo(virArch hostarch,
unsigned int *cores,
unsigned int *threads);
-int virHostCPUGetKVMMaxVCPUs(void) ATTRIBUTE_MOCKABLE;
+int virHostCPUGetKVMMaxVCPUs(void) ATTRIBUTE_NOINLINE;
int virHostCPUStatsAssign(virNodeCPUStatsPtr param,
const char *name,
diff --git a/src/util/virmacaddr.h b/src/util/virmacaddr.h
index 79492cd..f4f5e2c 100644
--- a/src/util/virmacaddr.h
+++ b/src/util/virmacaddr.h
@@ -48,7 +48,7 @@ void virMacAddrGetRaw(const virMacAddr *src, unsigned char dst[VIR_MAC_BUFLEN]);
const char *virMacAddrFormat(const virMacAddr *addr,
char *str);
void virMacAddrGenerate(const unsigned char prefix[VIR_MAC_PREFIX_BUFLEN],
- virMacAddrPtr addr) ATTRIBUTE_MOCKABLE;
+ virMacAddrPtr addr) ATTRIBUTE_NOINLINE;
int virMacAddrParse(const char* str,
virMacAddrPtr addr) ATTRIBUTE_RETURN_CHECK;
int virMacAddrParseHex(const char* str,
diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
index 2e9a9c4..51fcae5 100644
--- a/src/util/virnetdev.h
+++ b/src/util/virnetdev.h
@@ -156,7 +156,7 @@ int virNetDevExists(const char *brname)
int virNetDevSetOnline(const char *ifname,
bool online)
- ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_MOCKABLE;
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE;
int virNetDevGetOnline(const char *ifname,
bool *online)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
@@ -164,7 +164,7 @@ int virNetDevGetOnline(const char *ifname,
int virNetDevSetMAC(const char *ifname,
const virMacAddr *macaddr)
- ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_MOCKABLE;
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE;
int virNetDevGetMAC(const char *ifname,
virMacAddrPtr macaddr)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
@@ -303,8 +303,8 @@ int virNetDevSysfsFile(char **pf_sysfs_device_link,
const char *ifname,
const char *file)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
- ATTRIBUTE_RETURN_CHECK ATTRIBUTE_MOCKABLE;
+ ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE;
int virNetDevRunEthernetScript(const char *ifname, const char *script)
- ATTRIBUTE_MOCKABLE;
+ ATTRIBUTE_NOINLINE;
#endif /* __VIR_NETDEV_H__ */
diff --git a/src/util/virnetdevip.h b/src/util/virnetdevip.h
index 972a49a..6b509ea 100644
--- a/src/util/virnetdevip.h
+++ b/src/util/virnetdevip.h
@@ -67,7 +67,7 @@ int virNetDevIPAddrAdd(const char *ifname,
virSocketAddr *addr,
virSocketAddr *peer,
unsigned int prefix)
- ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_MOCKABLE;
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE;
int virNetDevIPRouteAdd(const char *ifname,
virSocketAddrPtr addr,
unsigned int prefix,
diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h
index dc677ca..51bb1dd 100644
--- a/src/util/virnetdevopenvswitch.h
+++ b/src/util/virnetdevopenvswitch.h
@@ -59,6 +59,6 @@ int virNetDevOpenvswitchInterfaceStats(const char *ifname,
int virNetDevOpenvswitchGetVhostuserIfname(const char *path,
char **ifname)
- ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_MOCKABLE;
+ ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE;
#endif /* __VIR_NETDEV_OPENVSWITCH_H__ */
diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h
index 1c4343e..0b17feb 100644
--- a/src/util/virnetdevtap.h
+++ b/src/util/virnetdevtap.h
@@ -39,7 +39,7 @@ int virNetDevTapCreate(char **ifname,
int *tapfd,
size_t tapfdSize,
unsigned int flags)
- ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_MOCKABLE;
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE;
int virNetDevTapDelete(const char *ifname,
const char *tunpath)
@@ -49,7 +49,7 @@ int virNetDevTapGetName(int tapfd, char **ifname)
ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
char* virNetDevTapGetRealDeviceName(char *ifname)
- ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_MOCKABLE;
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE;
typedef enum {
VIR_NETDEV_TAP_CREATE_NONE = 0,
@@ -89,7 +89,7 @@ int virNetDevTapCreateInBridgePort(const char *brname,
unsigned int *actualMTU,
unsigned int flags)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
- ATTRIBUTE_RETURN_CHECK ATTRIBUTE_MOCKABLE;
+ ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE;
int virNetDevTapInterfaceStats(const char *ifname,
virDomainInterfaceStatsPtr stats)
diff --git a/src/util/virnuma.h b/src/util/virnuma.h
index 62b89e9..e4e1fd0 100644
--- a/src/util/virnuma.h
+++ b/src/util/virnuma.h
@@ -34,20 +34,20 @@ int virNumaSetupMemoryPolicy(virDomainNumatuneMemMode mode,
virBitmapPtr nodeset);
virBitmapPtr virNumaGetHostMemoryNodeset(void);
-bool virNumaNodesetIsAvailable(virBitmapPtr nodeset) ATTRIBUTE_MOCKABLE;
-bool virNumaIsAvailable(void) ATTRIBUTE_MOCKABLE;
-int virNumaGetMaxNode(void) ATTRIBUTE_MOCKABLE;
-bool virNumaNodeIsAvailable(int node) ATTRIBUTE_MOCKABLE;
+bool virNumaNodesetIsAvailable(virBitmapPtr nodeset) ATTRIBUTE_NOINLINE;
+bool virNumaIsAvailable(void) ATTRIBUTE_NOINLINE;
+int virNumaGetMaxNode(void) ATTRIBUTE_NOINLINE;
+bool virNumaNodeIsAvailable(int node) ATTRIBUTE_NOINLINE;
int virNumaGetDistances(int node,
int **distances,
- int *ndistances) ATTRIBUTE_MOCKABLE;
+ int *ndistances) ATTRIBUTE_NOINLINE;
int virNumaGetNodeMemory(int node,
unsigned long long *memsize,
- unsigned long long *memfree) ATTRIBUTE_MOCKABLE;
+ unsigned long long *memfree) ATTRIBUTE_NOINLINE;
unsigned int virNumaGetMaxCPUs(void);
-int virNumaGetNodeCPUs(int node, virBitmapPtr *cpus) ATTRIBUTE_MOCKABLE;
+int virNumaGetNodeCPUs(int node, virBitmapPtr *cpus) ATTRIBUTE_NOINLINE;
int virNumaGetPageInfo(int node,
unsigned int page_size,
@@ -59,7 +59,7 @@ int virNumaGetPages(int node,
unsigned int **pages_avail,
unsigned int **pages_free,
size_t *npages)
- ATTRIBUTE_NONNULL(5) ATTRIBUTE_MOCKABLE;
+ ATTRIBUTE_NONNULL(5) ATTRIBUTE_NOINLINE;
int virNumaSetPagePoolSize(int node,
unsigned int page_size,
unsigned long long page_count,
diff --git a/src/util/virrandom.h b/src/util/virrandom.h
index 990a456..7a984ee 100644
--- a/src/util/virrandom.h
+++ b/src/util/virrandom.h
@@ -24,11 +24,11 @@
# include "internal.h"
-uint64_t virRandomBits(int nbits) ATTRIBUTE_MOCKABLE;
+uint64_t virRandomBits(int nbits) ATTRIBUTE_NOINLINE;
double virRandom(void);
uint32_t virRandomInt(uint32_t max);
int virRandomBytes(unsigned char *buf, size_t buflen)
- ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_MOCKABLE;
-int virRandomGenerateWWN(char **wwn, const char *virt_type) ATTRIBUTE_MOCKABLE;
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE;
+int virRandomGenerateWWN(char **wwn, const char *virt_type) ATTRIBUTE_NOINLINE;
#endif /* __VIR_RANDOM_H__ */
diff --git a/src/util/virscsi.h b/src/util/virscsi.h
index eed563d..9f8b3ec 100644
--- a/src/util/virscsi.h
+++ b/src/util/virscsi.h
@@ -37,7 +37,7 @@ char *virSCSIDeviceGetSgName(const char *sysfs_prefix,
const char *adapter,
unsigned int bus,
unsigned int target,
- unsigned long long unit) ATTRIBUTE_MOCKABLE;
+ unsigned long long unit) ATTRIBUTE_NOINLINE;
char *virSCSIDeviceGetDevName(const char *sysfs_prefix,
const char *adapter,
unsigned int bus,
diff --git a/src/util/virscsivhost.h b/src/util/virscsivhost.h
index f9272a6..21887dd 100644
--- a/src/util/virscsivhost.h
+++ b/src/util/virscsivhost.h
@@ -61,6 +61,6 @@ void virSCSIVHostDeviceGetUsedBy(virSCSIVHostDevicePtr dev,
const char **drv_name,
const char **dom_name);
void virSCSIVHostDeviceFree(virSCSIVHostDevicePtr dev);
-int virSCSIVHostOpenVhostSCSI(int *vhostfd) ATTRIBUTE_MOCKABLE;
+int virSCSIVHostOpenVhostSCSI(int *vhostfd) ATTRIBUTE_NOINLINE;
#endif /* __VIR_SCSIHOST_H__ */
diff --git a/src/util/virtpm.h b/src/util/virtpm.h
index 7067bb5..b21fc05 100644
--- a/src/util/virtpm.h
+++ b/src/util/virtpm.h
@@ -22,6 +22,6 @@
#ifndef __VIR_TPM_H__
# define __VIR_TPM_H__
-char *virTPMCreateCancelPath(const char *devpath) ATTRIBUTE_MOCKABLE;
+char *virTPMCreateCancelPath(const char *devpath) ATTRIBUTE_NOINLINE;
#endif /* __VIR_TPM_H__ */
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 35e5ca3..4938255 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -139,8 +139,8 @@ char *virGetUserConfigDirectory(void);
char *virGetUserCacheDirectory(void);
char *virGetUserRuntimeDirectory(void);
char *virGetUserShell(uid_t uid);
-char *virGetUserName(uid_t uid) ATTRIBUTE_MOCKABLE;
-char *virGetGroupName(gid_t gid) ATTRIBUTE_MOCKABLE;
+char *virGetUserName(uid_t uid) ATTRIBUTE_NOINLINE;
+char *virGetGroupName(gid_t gid) ATTRIBUTE_NOINLINE;
int virGetGroupList(uid_t uid, gid_t group, gid_t **groups)
ATTRIBUTE_NONNULL(3);
int virGetUserID(const char *name,
@@ -201,12 +201,12 @@ verify((int)VIR_TRISTATE_BOOL_ABSENT == (int)VIR_TRISTATE_SWITCH_ABSENT);
unsigned int virGetListenFDs(void);
-long virGetSystemPageSize(void) ATTRIBUTE_MOCKABLE;
-long virGetSystemPageSizeKB(void) ATTRIBUTE_MOCKABLE;
+long virGetSystemPageSize(void) ATTRIBUTE_NOINLINE;
+long virGetSystemPageSizeKB(void) ATTRIBUTE_NOINLINE;
unsigned long long virMemoryLimitTruncate(unsigned long long value);
bool virMemoryLimitIsSet(unsigned long long value);
-unsigned long long virMemoryMaxValue(bool ulong) ATTRIBUTE_MOCKABLE;
+unsigned long long virMemoryMaxValue(bool ulong) ATTRIBUTE_NOINLINE;
/**
* VIR_ASSIGN_IS_OVERFLOW:
diff --git a/src/util/viruuid.h b/src/util/viruuid.h
index 3b41b42..1d67e9e 100644
--- a/src/util/viruuid.h
+++ b/src/util/viruuid.h
@@ -49,7 +49,7 @@ int virGetHostUUID(unsigned char *host_uuid) ATTRIBUTE_NONNULL(1);
int virUUIDIsValid(unsigned char *uuid);
-int virUUIDGenerate(unsigned char *uuid) ATTRIBUTE_MOCKABLE;
+int virUUIDGenerate(unsigned char *uuid) ATTRIBUTE_NOINLINE;
int virUUIDParse(const char *uuidstr,
unsigned char *uuid)
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.
gnulib doesn't use weak functions, it uses pre-processor black magic.
eg gnulib when replacing 'mkdir', gnulib provides a 'rpl_mkdir' function.
Its replacement header file contains a #define mkdir rpl_mkdir, so that
libvirt explicitly calls rpl_mkdir.
glibc's static libc.a file doesn't contain any weak symbols, only
its libc.so does, so that avoids the problem.
So we could avoid it by compiling our code twice, but that is not
acceptable imho because of the time penalty
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.
Martin
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 12, 2017 at 01:56:47PM +0200, Martin Kletzander 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.
> >
>
> 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.
Ooohh, not having to use LD_PRELOAD is a now benefit. I'll try this
approach.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 12, 2017 at 01:56:47PM +0200, Martin Kletzander 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.
> >
>
> 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.
Empirically I'm not seeing any compiler warnings if i take this approach
and don't mark the function as an alias.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 12, 2017 at 05:07:56PM +0100, Daniel P. Berrange wrote: >On Wed, Jul 12, 2017 at 01:56:47PM +0200, Martin Kletzander wrote: >> 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. > >Empirically I'm not seeing any compiler warnings if i take this approach >and don't mark the function as an alias. > Oooh, me too :( That's a pity; yet another thing that proves I still don't _fully_ understand the details behind everything. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
----- Original Message -----
> 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
That's what I do locally atm, so ack
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange
> |:|
> |: https://libvirt.org -o- https://fstop138.berrange.com
> |:|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange
> |:|
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jul 13, 2017 at 07:52:56AM -0400, Marc-André Lureau wrote: > > > ----- Original Message ----- > > > > 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 > > That's what I do locally atm, so ack Ok, pushed now. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.