| Message ID | 1436531615-30183-1-git-send-email-rkrcmar@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10 July 2015 at 13:33, Radim Krčmář <rkrcmar@redhat.com> wrote: > In nettle 3, cbc_encrypt() accepts 'nettle_cipher_func' instead of > 'nettle_crypt_func' and these two differ in 'const' qualifier of the > first argument. The build fails with: > > In file included from crypto/cipher.c:71:0: > ./crypto/cipher-nettle.c: In function ‘qcrypto_cipher_encrypt’: > ./crypto/cipher-nettle.c:154:38: error: passing argument 2 of > ‘nettle_cbc_encrypt’ from incompatible pointer type > cbc_encrypt(ctx->ctx_encrypt, ctx->alg_encrypt, > ^ > In file included from ./crypto/cipher-nettle.c:24:0, > from crypto/cipher.c:71: > /usr/include/nettle/cbc.h:48:1: note: expected > ‘void (*)(const void *, size_t, uint8_t *, const uint8_t *) > but argument is of type > ‘void (*)( void *, size_t, uint8_t *, const uint8_t *) > > To allow both versions, we switch to the new definition and #if typedef > it for old versions. > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > --- > I don't know if we want to kill the #if compatibility after a while > (so QEMU doesn't become glibc-like) -- I could split this patch into > two, where the first one would just be reverted. We might eventually kill the compat, but we'd probably want to do it while retaining a version check in configure, so I'd just leave it as one patch. > @@ -83,8 +87,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg, > des_set_key(ctx->ctx_encrypt, rfbkey); > g_free(rfbkey); > > - ctx->alg_encrypt = (nettle_crypt_func *)des_encrypt; > - ctx->alg_decrypt = (nettle_crypt_func *)des_decrypt; > + ctx->alg_encrypt = (nettle_cipher_func *)des_encrypt; > + ctx->alg_decrypt = (nettle_cipher_func *)des_decrypt; > > ctx->niv = DES_BLOCK_SIZE; > break; > @@ -98,8 +102,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg, > aes_set_encrypt_key(ctx->ctx_encrypt, nkey, key); > aes_set_decrypt_key(ctx->ctx_decrypt, nkey, key); > > - ctx->alg_encrypt = (nettle_crypt_func *)aes_encrypt; > - ctx->alg_decrypt = (nettle_crypt_func *)aes_decrypt; > + ctx->alg_encrypt = (nettle_cipher_func *)aes_encrypt; > + ctx->alg_decrypt = (nettle_cipher_func *)aes_decrypt; Why do we need the casts here at all? If the functions we're passing around don't have the right signature anyway we're in big trouble and casting them is just going to hide the problem until runtime... thanks -- PMM
2015-07-10 13:56+0100, Peter Maydell: > On 10 July 2015 at 13:33, Radim Krčmář <rkrcmar@redhat.com> wrote: >> @@ -83,8 +87,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg, >> - ctx->alg_encrypt = (nettle_crypt_func *)des_encrypt; >> - ctx->alg_decrypt = (nettle_crypt_func *)des_decrypt; >> + ctx->alg_encrypt = (nettle_cipher_func *)des_encrypt; >> + ctx->alg_decrypt = (nettle_cipher_func *)des_decrypt; >> @@ -98,8 +102,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg, >> - ctx->alg_encrypt = (nettle_crypt_func *)aes_encrypt; >> - ctx->alg_decrypt = (nettle_crypt_func *)aes_decrypt; >> + ctx->alg_encrypt = (nettle_cipher_func *)aes_encrypt; >> + ctx->alg_decrypt = (nettle_cipher_func *)aes_decrypt; > > Why do we need the casts here at all? If the functions > we're passing around don't have the right signature > anyway we're in big trouble and casting them is > just going to hide the problem until runtime... Yes. We pass 'ctx' as a 'void *' in the code, but these functions accept specialized structures, which makes them incompatible: void nettle_cipher_func(const void *ctx, size_t length, [...]) void aes_decrypt(const struct aes_ctx *ctx, size_t length, [...]) void des_decrypt(const struct des_ctx *ctx, size_t length, [...]) We could take make struct QCryptoCipherNettle into an aes/des union (it's already tagged by QCryptoCipher->mode) to make coding a bit safer, but C types can't guarantee everything. Also, cbc_encrypt() takes nettle_cipher_func, so we need to directly cast at some point.
On 10 July 2015 at 14:31, Radim Krčmář <rkrcmar@redhat.com> wrote: > 2015-07-10 13:56+0100, Peter Maydell: >> On 10 July 2015 at 13:33, Radim Krčmář <rkrcmar@redhat.com> wrote: >>> @@ -83,8 +87,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg, >>> - ctx->alg_encrypt = (nettle_crypt_func *)des_encrypt; >>> - ctx->alg_decrypt = (nettle_crypt_func *)des_decrypt; >>> + ctx->alg_encrypt = (nettle_cipher_func *)des_encrypt; >>> + ctx->alg_decrypt = (nettle_cipher_func *)des_decrypt; >>> @@ -98,8 +102,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg, >>> - ctx->alg_encrypt = (nettle_crypt_func *)aes_encrypt; >>> - ctx->alg_decrypt = (nettle_crypt_func *)aes_decrypt; >>> + ctx->alg_encrypt = (nettle_cipher_func *)aes_encrypt; >>> + ctx->alg_decrypt = (nettle_cipher_func *)aes_decrypt; >> >> Why do we need the casts here at all? If the functions >> we're passing around don't have the right signature >> anyway we're in big trouble and casting them is >> just going to hide the problem until runtime... > > Yes. > > We pass 'ctx' as a 'void *' in the code, but these functions accept > specialized structures, which makes them incompatible: > > void nettle_cipher_func(const void *ctx, size_t length, [...]) > > void aes_decrypt(const struct aes_ctx *ctx, size_t length, [...]) > void des_decrypt(const struct des_ctx *ctx, size_t length, [...]) But aren't both the typedef and the aes/des_decrypt functions provided by the nettle library? Why is the library providing functions whose prototypes don't match its own typedef? -- PMM
On 10 July 2015 at 14:38, Peter Maydell <peter.maydell@linaro.org> wrote: > On 10 July 2015 at 14:31, Radim Krčmář <rkrcmar@redhat.com> wrote: >> We pass 'ctx' as a 'void *' in the code, but these functions accept >> specialized structures, which makes them incompatible: >> >> void nettle_cipher_func(const void *ctx, size_t length, [...]) >> >> void aes_decrypt(const struct aes_ctx *ctx, size_t length, [...]) >> void des_decrypt(const struct des_ctx *ctx, size_t length, [...]) > > But aren't both the typedef and the aes/des_decrypt functions > provided by the nettle library? Why is the library providing > functions whose prototypes don't match its own typedef? I had a suspicion that this was undefined behaviour, and I was right: http://stackoverflow.com/questions/559581/casting-a-function-pointer-to-another-type/14044244#14044244 If you have a function that takes "const struct foo *f" but the library code is calling it using a function pointer whose type says it's "const void *", then you can't just cast the function pointer before handing it to the library. You need to provide the obvious wrapper: void aes_decrypt_wrapper(const void *ctx, size_t length, ...) { aes_decrypt(ctx, length, ...); } -- PMM
2015-07-10 14:38+0100, Peter Maydell: > On 10 July 2015 at 14:31, Radim Krčmář <rkrcmar@redhat.com> wrote: >> 2015-07-10 13:56+0100, Peter Maydell: >>> On 10 July 2015 at 13:33, Radim Krčmář <rkrcmar@redhat.com> wrote: >>>> @@ -83,8 +87,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg, >>>> - ctx->alg_encrypt = (nettle_crypt_func *)des_encrypt; >>>> - ctx->alg_decrypt = (nettle_crypt_func *)des_decrypt; >>>> + ctx->alg_encrypt = (nettle_cipher_func *)des_encrypt; >>>> + ctx->alg_decrypt = (nettle_cipher_func *)des_decrypt; >>>> @@ -98,8 +102,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg, >>>> - ctx->alg_encrypt = (nettle_crypt_func *)aes_encrypt; >>>> - ctx->alg_decrypt = (nettle_crypt_func *)aes_decrypt; >>>> + ctx->alg_encrypt = (nettle_cipher_func *)aes_encrypt; >>>> + ctx->alg_decrypt = (nettle_cipher_func *)aes_decrypt; >>> >>> Why do we need the casts here at all? If the functions >>> we're passing around don't have the right signature >>> anyway we're in big trouble and casting them is >>> just going to hide the problem until runtime... >> >> Yes. >> >> We pass 'ctx' as a 'void *' in the code, but these functions accept >> specialized structures, which makes them incompatible: >> >> void nettle_cipher_func(const void *ctx, size_t length, [...]) >> >> void aes_decrypt(const struct aes_ctx *ctx, size_t length, [...]) >> void des_decrypt(const struct des_ctx *ctx, size_t length, [...]) > > But aren't both the typedef and the aes/des_decrypt functions > provided by the nettle library? Why is the library providing > functions whose prototypes don't match its own typedef? They are. Authors needed to sacrifice something to fit into the type system and I think they valued safety when using just a single cipher above safety when mixing them ... (The decision was probably biased by existing unabstracted code, if I were to guess how the library started.)
2015-07-10 14:56+0100, Peter Maydell: > On 10 July 2015 at 14:38, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 10 July 2015 at 14:31, Radim Krčmář <rkrcmar@redhat.com> wrote: >>> We pass 'ctx' as a 'void *' in the code, but these functions accept >>> specialized structures, which makes them incompatible: >>> >>> void nettle_cipher_func(const void *ctx, size_t length, [...]) >>> >>> void aes_decrypt(const struct aes_ctx *ctx, size_t length, [...]) >>> void des_decrypt(const struct des_ctx *ctx, size_t length, [...]) >> >> But aren't both the typedef and the aes/des_decrypt functions >> provided by the nettle library? Why is the library providing >> functions whose prototypes don't match its own typedef? > > I had a suspicion that this was undefined behaviour, > and I was right: > > http://stackoverflow.com/questions/559581/casting-a-function-pointer-to-another-type/14044244#14044244 > > If you have a function that takes "const struct foo *f" > but the library code is calling it using a function pointer > whose type says it's "const void *", then you can't just cast > the function pointer before handing it to the library. > You need to provide the obvious wrapper: > > void aes_decrypt_wrapper(const void *ctx, size_t length, ...) > { > aes_decrypt(ctx, length, ...); > } Makes sense, thanks. 'void *' shenanigans got me on this one. (Btw. the library isn't casting it back to the original type before calling and its test cases don't use wrappers.) I've posted v2 that addresses this problem.
diff --git a/configure b/configure index 33b945530e64..cc0338ddbd14 100755 --- a/configure +++ b/configure @@ -2183,6 +2183,7 @@ if test "$gnutls_nettle" != "no"; then if $pkg_config --exists "nettle"; then nettle_cflags=`$pkg_config --cflags nettle` nettle_libs=`$pkg_config --libs nettle` + nettle_version=`$pkg_config --modversion nettle` libs_softmmu="$nettle_libs $libs_softmmu" libs_tools="$nettle_libs $libs_tools" QEMU_CFLAGS="$QEMU_CFLAGS $nettle_cflags" @@ -4490,7 +4491,7 @@ echo "GTK support $gtk" echo "GNUTLS support $gnutls" echo "GNUTLS hash $gnutls_hash" echo "GNUTLS gcrypt $gnutls_gcrypt" -echo "GNUTLS nettle $gnutls_nettle" +echo "GNUTLS nettle $gnutls_nettle ${gnutls_nettle+($nettle_version)}" echo "VTE support $vte" echo "curses support $curses" echo "curl support $curl" @@ -4858,6 +4859,7 @@ if test "$gnutls_gcrypt" = "yes" ; then fi if test "$gnutls_nettle" = "yes" ; then echo "CONFIG_GNUTLS_NETTLE=y" >> $config_host_mak + echo "CONFIG_NETTLE_VERSION_MAJOR=${nettle_version%%.*}" >> $config_host_mak fi if test "$vte" = "yes" ; then echo "CONFIG_VTE=y" >> $config_host_mak diff --git a/crypto/cipher-nettle.c b/crypto/cipher-nettle.c index e5a14bc1393f..e61aaa29f049 100644 --- a/crypto/cipher-nettle.c +++ b/crypto/cipher-nettle.c @@ -23,12 +23,16 @@ #include <nettle/des.h> #include <nettle/cbc.h> +#if CONFIG_NETTLE_VERSION_MAJOR < 3 +typedef nettle_crypt_func nettle_cipher_func; +#endif + typedef struct QCryptoCipherNettle QCryptoCipherNettle; struct QCryptoCipherNettle { void *ctx_encrypt; void *ctx_decrypt; - nettle_crypt_func *alg_encrypt; - nettle_crypt_func *alg_decrypt; + nettle_cipher_func *alg_encrypt; + nettle_cipher_func *alg_decrypt; uint8_t *iv; size_t niv; }; @@ -83,8 +87,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg, des_set_key(ctx->ctx_encrypt, rfbkey); g_free(rfbkey); - ctx->alg_encrypt = (nettle_crypt_func *)des_encrypt; - ctx->alg_decrypt = (nettle_crypt_func *)des_decrypt; + ctx->alg_encrypt = (nettle_cipher_func *)des_encrypt; + ctx->alg_decrypt = (nettle_cipher_func *)des_decrypt; ctx->niv = DES_BLOCK_SIZE; break; @@ -98,8 +102,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg, aes_set_encrypt_key(ctx->ctx_encrypt, nkey, key); aes_set_decrypt_key(ctx->ctx_decrypt, nkey, key); - ctx->alg_encrypt = (nettle_crypt_func *)aes_encrypt; - ctx->alg_decrypt = (nettle_crypt_func *)aes_decrypt; + ctx->alg_encrypt = (nettle_cipher_func *)aes_encrypt; + ctx->alg_decrypt = (nettle_cipher_func *)aes_decrypt; ctx->niv = AES_BLOCK_SIZE; break;
In nettle 3, cbc_encrypt() accepts 'nettle_cipher_func' instead of 'nettle_crypt_func' and these two differ in 'const' qualifier of the first argument. The build fails with: In file included from crypto/cipher.c:71:0: ./crypto/cipher-nettle.c: In function ‘qcrypto_cipher_encrypt’: ./crypto/cipher-nettle.c:154:38: error: passing argument 2 of ‘nettle_cbc_encrypt’ from incompatible pointer type cbc_encrypt(ctx->ctx_encrypt, ctx->alg_encrypt, ^ In file included from ./crypto/cipher-nettle.c:24:0, from crypto/cipher.c:71: /usr/include/nettle/cbc.h:48:1: note: expected ‘void (*)(const void *, size_t, uint8_t *, const uint8_t *) but argument is of type ‘void (*)( void *, size_t, uint8_t *, const uint8_t *) To allow both versions, we switch to the new definition and #if typedef it for old versions. Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- I don't know if we want to kill the #if compatibility after a while (so QEMU doesn't become glibc-like) -- I could split this patch into two, where the first one would just be reverted. configure | 4 +++- crypto/cipher-nettle.c | 16 ++++++++++------ 2 files changed, 13 insertions(+), 7 deletions(-)