GNOME Bugzilla – Bug 738863
Complete gcrypt initialization and set min version to 1.5.0
Last modified: 2014-10-22 08:41:09 UTC
I cooked this patch without realizing bug 732475 has already solved the crash I intended to fix. However, I believe this patch is more complete than current implementation since it adds gcrypt initialization at the beginning of the application (and the example), which is the recommended method as this will assure that we are initializing it with the configuration we want and will also assure that frogr will not be affected by changes in depending libraries not the internal flicksoup one: https://www.gnupg.org/documentation/manuals/gcrypt/Initializing-the-library.html#Initializing-the-library In addition, I add a check for gcrypt version to 1.5.0. This is not strictly needed by I think it would be safer from now one to specify a version whether it is 1.5.0 or a more recent/older one. 1.5.0 seems like a safe bet to me given the current state in gcrypt in the most popular GNU/Linux distributions.
Created attachment 288927 [details] [review] Complete gcrypt initialization and set min version to 1.5.0 Current code for initialization is a nice safe check when using the library but the recommended is to do so in application level which assures us to be using the configuration we want.
Review of attachment 288927 [details] [review]: Thanks for filing this bug, Andrés. You are right, the flicksoup layer is not the right place for that initialization to happen, as it's an internal library, but the applications themselves. I'd say the patch it's mostly good to go, just please consider my comments below... ::: configure.ac @@ +49,3 @@ AC_MSG_ERROR([libgcrypt not found, please install it]) else + LIBGCRYPT_CFLAGS="$LIBGCRYPT_CFLAGS -DLIBGCRYPT_MIN_VERSION='\"$LIBGCRYPT_MIN_VERSION\"'" Why not using AC_DEFINE() for this, instead of passing a -D extra flag? ::: src/flicksoup/fsp-session.c @@ +524,3 @@ + Because you can't know in a library whether another library has + already initialized the library */ + if (!gcry_control (GCRYCTL_ANY_INITIALIZATION_P)) After reading the docs again, I think we should put GCRYCTL_INITIALIZATION_FINISHED_P here
(In reply to comment #2) > Review of attachment 288927 [details] [review]: > > Thanks for filing this bug, Andrés. You are right, the flicksoup layer is not > the right place for that initialization to happen, as it's an internal library, > but the applications themselves. > > I'd say the patch it's mostly good to go, just please consider my comments > below... > > ::: configure.ac > @@ +49,3 @@ > AC_MSG_ERROR([libgcrypt not found, please install it]) > else > + LIBGCRYPT_CFLAGS="$LIBGCRYPT_CFLAGS > -DLIBGCRYPT_MIN_VERSION='\"$LIBGCRYPT_MIN_VERSION\"'" > > Why not using AC_DEFINE() for this, instead of passing a -D extra flag? Right! Will do ... > > ::: src/flicksoup/fsp-session.c > @@ +524,3 @@ > + Because you can't know in a library whether another library has > + already initialized the library */ > + if (!gcry_control (GCRYCTL_ANY_INITIALIZATION_P)) > > After reading the docs again, I think we should put > GCRYCTL_INITIALIZATION_FINISHED_P here I'm a bit unsure about this. Reading: https://www.gnupg.org/documentation/manuals/gcrypt-devel/Controlling-the-library.html The difference from GCRYCTL_ANY_INITIALIZATION_P and GCRYCTL_INITIALIZATION_FINISHED_P is that ANY checks that gcry_check_version has been called, although GCRYCTL_INITIALIZATION_FINISHED may have not been called and the second checks that GCRYCTL_INITIALIZATION_FINISHED has been actually called. Then, if a basic initialization has been performed, with GCRYCTL_INITIALIZATION_FINISHED_P we will be doing a completely initialization once again. I don't know what would happen then. Probably, nothing really bad but I will check and see if, then, we have, at least, to remove the assert. Any comment?
(In reply to comment #3) > > ::: src/flicksoup/fsp-session.c > > @@ +524,3 @@ > > + Because you can't know in a library whether another library has > > + already initialized the library */ > > + if (!gcry_control (GCRYCTL_ANY_INITIALIZATION_P)) > > > > After reading the docs again, I think we should put > > GCRYCTL_INITIALIZATION_FINISHED_P here > > I'm a bit unsure about this. After some checks, it seems to be OK to use GCRYCTL_INITIALIZATION_FINISHED_P instead of GCRYCTL_ANY_INITIALIZATION_P. New version of the patch coming ...
Created attachment 289036 [details] [review] Complete gcrypt initialization and set min version to 1.5.0 Current code for initialization is a nice safe check when using the library but the recommended is to do so in application level which assures us to be using the configuration we want.
Comment on attachment 289036 [details] [review] Complete gcrypt initialization and set min version to 1.5.0 Looks good to me now. Please go ahead and commit when you can. Thanks!
Attachment 289036 [details] pushed as 3b28477 - Complete gcrypt initialization and set min version to 1.5.0