| Message ID | 20200527093409.3588189-1-rjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | crypto: Remove use of GCRYPT_VERSION macro. | expand |
On Wed, May 27, 2020 at 10:34:09AM +0100, Richard W.M. Jones wrote: > According to the gcrypt documentation it's intended that > gcry_check_version() is called with the minimum version of gcrypt > needed by the program, not the version from the <gcrypt.h> header file > that happened to be installed when qemu was compiled. Indeed the > gcrypt.h header says that you shouldn't use the GCRYPT_VERSION macro. That's awesome, because the API docs illustrate gcry_check_version with passing GCRYPT_VERSION ! > This causes the following failure: > > qemu-img: Unable to initialize gcrypt > > if a slightly older version of libgcrypt is installed with a newer > qemu, even though the slightly older version works fine. This can > happen with RPM packaging which uses symbol versioning to determine > automatically which libgcrypt is required by qemu, which caused the > following bug in RHEL 8: > > https://bugzilla.redhat.com/show_bug.cgi?id=1840485 > > qemu actually requires libgcrypt >= 1.5.0, so we might put the string > "1.5.0" here. However since 1.5.0 was released in 2011, it hardly > seems we need to check that. So I replaced GCRYPT_VERSION with NULL. > Perhaps in future if we move to requiring a newer version of gcrypt we > could put a literal string here. I checked that v1.5.0 still accepts NULL and it does, so we're fine. We validate the 1.5.0 version in configure, and any runtime usage would be hanlded by ELF symbol versioning as you say. > > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> > --- > crypto/init.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/crypto/init.c b/crypto/init.c > index b305381ec5..ea233b9192 100644 > --- a/crypto/init.c > +++ b/crypto/init.c > @@ -122,7 +122,7 @@ int qcrypto_init(Error **errp) > #endif > > #ifdef CONFIG_GCRYPT > - if (!gcry_check_version(GCRYPT_VERSION)) { > + if (!gcry_check_version(NULL)) { > error_setg(errp, "Unable to initialize gcrypt"); > return -1; > } Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> and queued. Regards, Daniel
On Wed, May 27, 2020 at 10:42:06AM +0100, Daniel P. Berrangé wrote: > On Wed, May 27, 2020 at 10:34:09AM +0100, Richard W.M. Jones wrote: > > According to the gcrypt documentation it's intended that > > gcry_check_version() is called with the minimum version of gcrypt > > needed by the program, not the version from the <gcrypt.h> header file > > that happened to be installed when qemu was compiled. Indeed the > > gcrypt.h header says that you shouldn't use the GCRYPT_VERSION macro. > > That's awesome, because the API docs illustrate gcry_check_version > with passing GCRYPT_VERSION ! They also do it on their internal tests, but I guess that's because they want to ensure those tests run with the currently compiled version of libgcrypt (and not the installed package). However the info file does make it clear that it's the needed version not the current version. The example from the info file is: /* Version check should be the very first call because it makes sure that important subsystems are initialized. #define NEED_LIBGCRYPT_VERSION to the minimum required version. */ if (!gcry_check_version (NEED_LIBGCRYPT_VERSION)) { fprintf (stderr, "libgcrypt is too old (need %s, have %s)\n", NEED_LIBGCRYPT_VERSION, gcry_check_version (NULL)); exit (2); } Rich. > > This causes the following failure: > > > > qemu-img: Unable to initialize gcrypt > > > > if a slightly older version of libgcrypt is installed with a newer > > qemu, even though the slightly older version works fine. This can > > happen with RPM packaging which uses symbol versioning to determine > > automatically which libgcrypt is required by qemu, which caused the > > following bug in RHEL 8: > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1840485 > > > > qemu actually requires libgcrypt >= 1.5.0, so we might put the string > > "1.5.0" here. However since 1.5.0 was released in 2011, it hardly > > seems we need to check that. So I replaced GCRYPT_VERSION with NULL. > > Perhaps in future if we move to requiring a newer version of gcrypt we > > could put a literal string here. > > I checked that v1.5.0 still accepts NULL and it does, so we're > fine. We validate the 1.5.0 version in configure, and any > runtime usage would be hanlded by ELF symbol versioning as > you say. > > > > > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> > > --- > > crypto/init.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/crypto/init.c b/crypto/init.c > > index b305381ec5..ea233b9192 100644 > > --- a/crypto/init.c > > +++ b/crypto/init.c > > @@ -122,7 +122,7 @@ int qcrypto_init(Error **errp) > > #endif > > > > #ifdef CONFIG_GCRYPT > > - if (!gcry_check_version(GCRYPT_VERSION)) { > > + if (!gcry_check_version(NULL)) { > > error_setg(errp, "Unable to initialize gcrypt"); > > return -1; > > } > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > and queued. > > 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 :|
diff --git a/crypto/init.c b/crypto/init.c index b305381ec5..ea233b9192 100644 --- a/crypto/init.c +++ b/crypto/init.c @@ -122,7 +122,7 @@ int qcrypto_init(Error **errp) #endif #ifdef CONFIG_GCRYPT - if (!gcry_check_version(GCRYPT_VERSION)) { + if (!gcry_check_version(NULL)) { error_setg(errp, "Unable to initialize gcrypt"); return -1; }
According to the gcrypt documentation it's intended that gcry_check_version() is called with the minimum version of gcrypt needed by the program, not the version from the <gcrypt.h> header file that happened to be installed when qemu was compiled. Indeed the gcrypt.h header says that you shouldn't use the GCRYPT_VERSION macro. This causes the following failure: qemu-img: Unable to initialize gcrypt if a slightly older version of libgcrypt is installed with a newer qemu, even though the slightly older version works fine. This can happen with RPM packaging which uses symbol versioning to determine automatically which libgcrypt is required by qemu, which caused the following bug in RHEL 8: https://bugzilla.redhat.com/show_bug.cgi?id=1840485 qemu actually requires libgcrypt >= 1.5.0, so we might put the string "1.5.0" here. However since 1.5.0 was released in 2011, it hardly seems we need to check that. So I replaced GCRYPT_VERSION with NULL. Perhaps in future if we move to requiring a newer version of gcrypt we could put a literal string here. Signed-off-by: Richard W.M. Jones <rjones@redhat.com> --- crypto/init.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)