[libvirt] [PATCH] util: virrandom: make virRandomBits endian-safe
Make the generation of random bits in virRandomBits independent of the endianness of the running architecture. This also solves problems with the mocked random byte generation on big-endian machines. Suggested-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com> --- This goes on top of Michal's fix: https://www.redhat.com/archives/libvir-list/2018-August/msg00080.html src/util/virrandom.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 7915f653..26ff68f5 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -34,6 +34,7 @@ # include <gnutls/crypto.h> #endif +#include "virendian.h" #include "virrandom.h" #include "virthread.h" #include "count-one-bits.h" @@ -61,13 +62,16 @@ VIR_LOG_INIT("util.random"); uint64_t virRandomBits(int nbits) { uint64_t ret = 0; + uint8_t val[8]; - if (virRandomBytes((unsigned char *) &ret, sizeof(ret)) < 0) { + if (virRandomBytes((unsigned char *) &val, sizeof(val)) < 0) { /* You're already hosed, so this particular non-random value * isn't any worse. */ return 0; } + ret = virReadBufInt64LE(val); + if (nbits < 64) ret &= (1ULL << nbits) - 1; -- 2.17.0
On Thursday, 2 August 2018 09:56:49 CEST Bjoern Walk wrote:
Make the generation of random bits in virRandomBits independent of the endianness of the running architecture.
This also solves problems with the mocked random byte generation on big-endian machines.
Suggested-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com> --- This goes on top of Michal's fix: https://www.redhat.com/archives/libvir-list/2018-August/msg00080.html
src/util/virrandom.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 7915f653..26ff68f5 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -34,6 +34,7 @@ # include <gnutls/crypto.h> #endif
+#include "virendian.h" #include "virrandom.h" #include "virthread.h" #include "count-one-bits.h" @@ -61,13 +62,16 @@ VIR_LOG_INIT("util.random"); uint64_t virRandomBits(int nbits) { uint64_t ret = 0; + uint8_t val[8];
- if (virRandomBytes((unsigned char *) &ret, sizeof(ret)) < 0) { + if (virRandomBytes((unsigned char *) &val, sizeof(val)) < 0) { /* You're already hosed, so this particular non-random value * isn't any worse. */ return 0; }
+ ret = virReadBufInt64LE(val); +
I do not think this patch is correct: we are dealing with random bytes, so there is no "endianness" for them. -- Pino Toscano
Pino Toscano <ptoscano@redhat.com> [2018-08-02, 10:02AM +0200]:
I do not think this patch is correct: we are dealing with random bytes, so there is no "endianness" for them.
Well, it's not incorrect either, isn't it? I agree that endianness doesn't matter for random data, but in the same time, it doesn't hurt to have the same random data generated on big- and little-endian machines. And it gives an easy and future-proof fix for the mocked tests.
On Thu, Aug 02, 2018 at 10:17:32AM +0200, Bjoern Walk wrote:
Pino Toscano <ptoscano@redhat.com> [2018-08-02, 10:02AM +0200]:
I do not think this patch is correct: we are dealing with random bytes, so there is no "endianness" for them.
Well, it's not incorrect either, isn't it? I agree that endianness doesn't matter for random data, but in the same time, it doesn't hurt to have the same random data generated on big- and little-endian machines. And it gives an easy and future-proof fix for the mocked tests.
Lets just modijfy tests/virrandommock.c to add mocking of virRandomBits alongside virRandomBytes. 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 :|
On Thursday, 2 August 2018 12:28:45 CEST Daniel P. Berrangé wrote:
On Thu, Aug 02, 2018 at 10:17:32AM +0200, Bjoern Walk wrote:
Pino Toscano <ptoscano@redhat.com> [2018-08-02, 10:02AM +0200]:
I do not think this patch is correct: we are dealing with random bytes, so there is no "endianness" for them.
Well, it's not incorrect either, isn't it? I agree that endianness doesn't matter for random data, but in the same time, it doesn't hurt to have the same random data generated on big- and little-endian machines.
Not sure I understand -- since it's random data, you cannot have it "the same", no matter which endianness the machine has.
And it gives an easy and future-proof fix for the mocked tests.
IMHO every mocked test has its own behaviour, and thus the mocking needs to reflect that.
Lets just modijfy tests/virrandommock.c to add mocking of virRandomBits alongside virRandomBytes.
I don't see how it will help, since all virRandomBits does is calling virRandomBytes. I still did not see any complaints about my patch to fix viriscsitest (since the problem is specific for it), what about ACKing it then? -- Pino Toscano
On Thu, Aug 02, 2018 at 01:12:02PM +0200, Pino Toscano wrote:
On Thursday, 2 August 2018 12:28:45 CEST Daniel P. Berrangé wrote:
On Thu, Aug 02, 2018 at 10:17:32AM +0200, Bjoern Walk wrote:
Pino Toscano <ptoscano@redhat.com> [2018-08-02, 10:02AM +0200]:
I do not think this patch is correct: we are dealing with random bytes, so there is no "endianness" for them.
Well, it's not incorrect either, isn't it? I agree that endianness doesn't matter for random data, but in the same time, it doesn't hurt to have the same random data generated on big- and little-endian machines.
Not sure I understand -- since it's random data, you cannot have it "the same", no matter which endianness the machine has.
And it gives an easy and future-proof fix for the mocked tests.
IMHO every mocked test has its own behaviour, and thus the mocking needs to reflect that.
Lets just modijfy tests/virrandommock.c to add mocking of virRandomBits alongside virRandomBytes.
I don't see how it will help, since all virRandomBits does is calling virRandomBytes.
That's exactly what it would fix. A virRandomBits mock would simply return a fixed number and not call the mocked virRandomBytes at all thus avoiding the endianness problem
I still did not see any complaints about my patch to fix viriscsitest (since the problem is specific for it), what about ACKing it then?
The virrandommock.c was intended to provide "stable" random numbers across multiple tests. It didn't mock virRandomBits because it was mistakenly assumed that mocking virRandomBytes was enough. Changing virscsitest just fixes the one occurrance that happens to hit today - we want to fix the general problem as virrandommock.c intended. 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 :|
participants (3)
-
Bjoern Walk -
Daniel P. Berrangé -
Pino Toscano