It seems that in the GCM macros nettle uses casts to functions. That is pretty dangerous with the changes of parameters in functions in nettle 3. The issue is the compiler will not warn for serious errors such as different function type. An example macro is GCM_ENCRYPT.
#define GCM_ENCRYPT(ctx, encrypt, length, dst, src) \ (0 ? (encrypt)(&(ctx)->cipher, 0, (void *)0, (void *)0) \ : gcm_encrypt(&(ctx)->gcm, &(ctx)->key, &(ctx)->cipher, \ (nettle_cipher_func *) (encrypt), \ (length), (dst), (src)))
I don't think that nettle should be casting functions. The issue seems to be present in ctr, eax and cbc modes as well.
regards, Nikos
Nikos Mavrogiannopoulos nmav@gnutls.org writes:
That is pretty dangerous with the changes of parameters in functions in nettle 3. The issue is the compiler will not warn for serious errors such as different function type. An example macro is GCM_ENCRYPT.
#define GCM_ENCRYPT(ctx, encrypt, length, dst, src) \ (0 ? (encrypt)(&(ctx)->cipher, 0, (void *)0, (void *)0) \ : gcm_encrypt(&(ctx)->gcm, &(ctx)->key, &(ctx)->cipher, \ (nettle_cipher_func *) (encrypt), \ (length), (dst), (src)))
The idea of this macro is that
1. It should be possible to pass a context with the cipher element typed as a struct aes128_ctx, and encrypt as the function aes128_encrypt, without errors or warnings. To have the cast in one place, instead of sprinkled throughout application code.
2. The expression after the 0 ? should give some additional typechecking, so that, e.g, having a ctx->cipher of type aes128_ctx and encrypt as the function camellia128_crypt, you will get a warning from the compiler about bad first argument to camellia128_crypt.
If you pass an encrypt function of type nettle_cipher_func (with a const void *) first argument, you get less type checking, but at least you should get a check on number of arguments, and integers vs pointers.
Can you give an example usage where you'd want to get a compiler warning, but you don't get one?
BTW, a minor improvement to type checking would be to change the (void*) 0 expressions above to (uint8_t *) 0 and (const uint8_t *) 0.
Regards, /Niels
On Sat, 2014-12-06 at 09:59 +0100, Niels Möller wrote:
The idea of this macro is that
It should be possible to pass a context with the cipher element typed as a struct aes128_ctx, and encrypt as the function aes128_encrypt, without errors or warnings. To have the cast in one place, instead of sprinkled throughout application code.
The expression after the 0 ? should give some additional typechecking, so that, e.g, having a ctx->cipher of type aes128_ctx and encrypt as the function camellia128_crypt, you will get a warning from the compiler about bad first argument to camellia128_crypt.
If you pass an encrypt function of type nettle_cipher_func (with a const void *) first argument, you get less type checking, but at least you should get a check on number of arguments, and integers vs pointers.
In modern compilers you cannot rely on that. The constructions if(0){} and other impossible situations are optimized out early.
Can you give an example usage where you'd want to get a compiler warning, but you don't get one?
The change of the cipher_func to accept a size_t instead of unsigned. https://gitorious.org/gnutls/gnutls/commit/488cfe9a57840faeb2e35250757d8d33d...
regards, Nikos
Nikos Mavrogiannopoulos nmav@gnutls.org writes:
In modern compilers you cannot rely on that. The constructions if(0){} and other impossible situations are optimized out early.
Examples? I would expect any serious C compiler to do basic type checking of dead code (even if I don't expect to get all warnings generated by optimization passes, i.e., the warnings you normally get only if you compile with -O). If gcc stopped doing type checking on dead code, I would likely file a bug report.
Can you give an example usage where you'd want to get a compiler warning, but you don't get one?
The change of the cipher_func to accept a size_t instead of unsigned. https://gitorious.org/gnutls/gnutls/commit/488cfe9a57840faeb2e35250757d8d33d...
Hmm. In that case, I'd recommend declaring the function as
static nettle_cipher_func padock_aes_encrypt;
(or with nettle_crypt_func, before the introduction of nettle_cipher_func). Then you would have gotten a warning for mismatch between declaration and definition, when that type was changed. But that's of little help now, I guess.
What do you suggest to do with the nettle macros? They are intended for use with the various _CTX() macros, and for that purpose, I think the current definitions with casts and all are adequate, so I'd prefer to not simply delete the cast.
We could add additional macros, say
#define GCM_ENCRYPT_NO_CAST(ctx, encrypt, length, dst, src) \ gcm_encrypt(&(ctx)->gcm, &(ctx)->key, &(ctx)->cipher, \ (encrypt), (length), (dst), (src))
Question is, does that solve a real problem? My initial feeling is that when there's nothing "magic" left inside the macro, using it provides little benefit; code would likely be clearer by calling gcm_encrypt directly. What do you think?
Regards, /Niels
On Sat, 2014-12-06 at 22:01 +0100, Niels Möller wrote:
Nikos Mavrogiannopoulos nmav@gnutls.org writes:
In modern compilers you cannot rely on that. The constructions if(0){} and other impossible situations are optimized out early.
Examples? I would expect any serious C compiler to do basic type checking of dead code (even if I don't expect to get all warnings generated by optimization passes, i.e., the warnings you normally get only if you compile with -O). If gcc stopped doing type checking on dead code, I would likely file a bug report.
An example that would compile and run perfectly on modern gcc is: #include <stdio.h> int main() { if (0) { call_this_unknown_function(); } else { printf("hello\n"); } }
but indeed, while it skips undefined functions, it still performs type checking on the known ones.
What do you suggest to do with the nettle macros? They are intended for use with the various _CTX() macros, and for that purpose, I think the current definitions with casts and all are adequate, so I'd prefer to not simply delete the cast.
What is the purpose of the cast? Isn't the macro supposed to have a function pointer input? If yes, I don't see the cast purpose, while I see the issues that it can cause. Yes I could have used the function, but I did use the macro which was part of the API, and should be treated as such.
Question is, does that solve a real problem?
What do you mean by that? I sent a real issue on the previous mail. A problem that would have been very hard to notice and debug. When the API and ABI are changed it is best to let the compiler warn of these changes and not hide them under casts.
regards, Nikos
Nikos Mavrogiannopoulos nmav@gnutls.org writes:
What is the purpose of the cast? Isn't the macro supposed to have a function pointer input?
It's supposed to get a function pointer, but not necessarily of exactly the type nettle_cipher_func. The idea is that applications should be able to do
struct GCM_CTX(struct aes128_ctx) ctx; ... GCM_ENCRYPT(&ctx, aes128_encrypt, ...)
without compiler warnings and without manually casting aes128_encrypt to (nettle_cipher_func *). The main purpose of these convenience macros is to do the needed cast to (nettle_cipher_func *), and bundle that cast together with a little extra typechecking.
Question is, does that solve a real problem?
What do you mean by that?
If one prefers to not have that cast, one can call always call gcm_encrypt directly; GCM_ENCRYPT is an optional feature.
So my question is, do you think it would make your code easier to read and debug, if nettle provided a simple non-magic wrapper macro around gcm_encrypt (in addition to the current GCM_ENCRYPT macro which you find questionable), or would it be clearer to call gcm_encrypt directly?
Regards, /Niels
On Sun, 2014-12-07 at 13:50 +0100, Niels Möller wrote:
Question is, does that solve a real problem?
What do you mean by that?
If one prefers to not have that cast, one can call always call gcm_encrypt directly; GCM_ENCRYPT is an optional feature.
So my question is, do you think it would make your code easier to read and debug, if nettle provided a simple non-magic wrapper macro around gcm_encrypt (in addition to the current GCM_ENCRYPT macro which you find questionable), or would it be clearer to call gcm_encrypt directly?
No. Putting an other safer macro will not solve that issue. My concern is on the easiness to have a bug-free transition to 3.0 from 2.7.1.
regards, Nikos
Nikos Mavrogiannopoulos nmav@gnutls.org writes:
My concern is on the easiness to have a bug-free transition to 3.0 from 2.7.1.
I see. I don't think I'm going to remove the cast for nettle-3.1. I view it as a feature, not a bug. The intent is to reduce the amount of casts in application code, and provide something slightly safer.
I wonder if there are any other simple changes (besides documenting potential problems) which would reduce upgrade problems?
Hmm, maybe, in the
(0 ? (encrypt)(&(ctx)->cipher, 0, (void *)0, (void *)0)
part of macro, intended for typechecking, one could change the 0 constant for the size argument to some larger constant which doesn't fit in a plain int. E.g., consider the following test program:
#include <stdlib.h>
extern foo (int x);
int main (int argc, char **argv) { if (0) foo (~(size_t)0); return 0; }
When I compile it with gcc, version 4.7.2, no optimization or special warning flags, on x86_64, it produces a warning,
size_t-type-checking.c: In function `main': size_t-type-checking.c:8:5: warning: overflow in implicit constant conversion [-Woverflow]
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
Hmm, maybe, in the
(0 ? (encrypt)(&(ctx)->cipher, 0, (void *)0, (void *)0)
part of macro, intended for typechecking, one could change the 0 constant for the size argument to some larger constant which doesn't fit in a plain int.
I've now done this, and I've also changed the type of the pointer arguments to be more specific.
Nikos, if you have the time, it would be nice if you could build gnutls without the https://gitorious.org/gnutls/gnutls/commit/488cfe9a57840faeb2e35250757d8d33d... fix, and with the nettle header files from master, and see if you get any warnings (on a 64-bit arch, where size_t is larger than unsigned int).
Regards, /Niels
On Fri, 2014-12-12 at 20:04 +0100, Niels Möller wrote:
Nikos, if you have the time, it would be nice if you could build gnutls without the https://gitorious.org/gnutls/gnutls/commit/488cfe9a57840faeb2e35250757d8d33d... fix, and with the nettle header files from master, and see if you get any warnings (on a 64-bit arch, where size_t is larger than unsigned int).
There is a truncation warning indeed.
regards, Nikos
Nikos Mavrogiannopoulos nmav@gnutls.org writes:
There is a truncation warning indeed.
Good. Thanks for testing.
Regards, /Niels
nettle-bugs@lists.lysator.liu.se
-
Nikos Mavrogiannopoulos -
nisse＠lysator.liu.se