Some things I fixed, when having a look at the sources.
Just two points for code reviewing:
1. #define NONNULL(...) __attribute__ ((nonnull(__VA_ARGS__))) is contains a C99 feature (...), but there are also C99 long long constants somewhere in the code (if you mind C89 compliancy).
2. In cast128.c I removed the wiping of t, l and r. Instead I set t=0 at the beginning of the loops (It seemed to be used uninitialized in F1 macro). Please just have a short look into it - maybe the "wiping" has some undocumented deeper meaning !?
Regards, Tim
Tim Ruehsen tim.ruehsen@gmx.de writes:
Some things I fixed, when having a look at the sources.
Thanks.
I'm going to a reply to one or a few issues at a time. Feel free to remind me if some things are still left unaddressed in a few weeks.
#define NONNULL(...) __attribute__ ((nonnull(__VA_ARGS__))) is contains a C99 feature (...), but there are also C99 long long constants somewhere in the code (if you mind C89 compliancy).
I intend to stick to C89, except that I'll happily use features which have been widely available for a long time even though not standardized. Maybe the use of long long constants (where?) are of that type?
So I don't want to use __VA_ARGS__. In this particular case, shouldn't it work fine with
#define NONNULL(args) __attribute__ ((nonnull args))
to be used as, e.g., NONNULL((1)) ?
About __attribute__ nonnull, I have some questions.
1. Does it give any significant benefit, in terms of significant optimizations (I don't think I understand what optimizations it should make possible) or real bugs catched?
2. Since when (release and year) is it supported by gcc?
3. To really make use of it, we should add it to a lot of prototypes in the installed headers. That makes things a bit more complicated, since those headers can't use config.h.
Regards, /Niels
Am Friday 07 September 2012 schrieb Niels Möller:
Tim Ruehsen tim.ruehsen@gmx.de writes:
Some things I fixed, when having a look at the sources.
Thanks.
I'm going to a reply to one or a few issues at a time. Feel free to remind me if some things are still left unaddressed in a few weeks.
#define NONNULL(...) __attribute__ ((nonnull(__VA_ARGS__))) is contains a C99 feature (...), but there are also C99 long long constants somewhere in the code (if you mind C89 compliancy).
I intend to stick to C89, except that I'll happily use features which have been widely available for a long time even though not standardized. Maybe the use of long long constants (where?) are of that type?
So I don't want to use __VA_ARGS__. In this particular case, shouldn't it work fine with
#define NONNULL(args) __attribute__ ((nonnull args))
Yes, functions with several non-null args than need several NONNULL(x) occurrences. That isn't too ugly. Just change it as you like.
to be used as, e.g., NONNULL((1)) ?
About __attribute__ nonnull, I have some questions.
- Does it give any significant benefit, in terms of significant optimizations (I don't think I understand what optimizations it should make possible) or real bugs catched?
The idea of nonnull is to enable the compiler detecting possible NULL arguments where they must not be. E.g. char *s=NULL; strlen(s); would let the compiler print a warning. Especially static analyzers can make use of nonnull attributes
Of course there is a BUT: Gcc (IMHO, upto the 4.8) has a problem in detecting indirectly used NULL values. At the same time, when optimizing a function with a nonnull argument, gcc optimizes away checks against NULL for the appropriate variables. see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308
Example: void f(char *arg) NONNULL(1) { if (arg) *arg=0; }
would be optimized to void f(char *arg) NONNULL(1) { *arg=0; }
Since gcc can't detect indirect NULL values, the function may crash the process.
I am not shure, but maybe something like #if defined(__clang__) # define NONNULL(args) __attribute__ ((nonnull args)) #endif
to make use of NONNULL at least for clang analyzing.
Different projects handle nonnull in different ways. I, personally, wouldn't miss that feature...
- Since when (release and year) is it supported by gcc?
I found it to be for gcc >= 3.3, see http://ohse.de/uwe/articles/gcc-attributes.html#func-nonnull
- To really make use of it, we should add it to a lot of prototypes in the installed headers. That makes things a bit more complicated, since those headers can't use config.h.
The more functions we add nonnull to, the more segfaults the compiler should catch (see 1.). The reason I used nonnull were several compiler warnings. The current libc6 header files seem to use nonnull by default (gcc 4.7.1, libc6 2.13). Maybe there should be -std=c89 in den CFLAGS to prevent that ?
Regards, /Niels
Regards, Tim
Tim Ruehsen tim.ruehsen@gmx.de writes:
#define NONNULL(args) __attribute__ ((nonnull args))
Yes, functions with several non-null args than need several NONNULL(x) occurrences.
I don't think so. NONNULL((1,3,5)) should work, with "(1,3,5)" treated as a single argument by the preprocessor. I would actually prefer to have NONNULL declaration on each argument in question, just like UNUSED, but if I understand it correctly, that won't work.
The idea of nonnull is to enable the compiler detecting possible NULL arguments where they must not be. E.g. char *s=NULL; strlen(s); would let the compiler print a warning.
I'm not entirely convinced about the usefulness of that.
At the same time, when optimizing a function with a nonnull argument, gcc optimizes away checks against NULL for the appropriate variables.
I'm not entirely convinced that makes for any real improvements of performance on real code. And I think I'd rather get a warning from the compiler about a useless comparison (like, like gcc warns for for unsigned x = ...; if (x >= 0)...) than having the optimizer silently remove the code.
In particular, if I have a funtion where a pointer must not be NULL, and I have an assert (p != NULL); to that effect, then I will *not* consider it an improvement if the compiler decides to remove that assert.
Is there anything more useful and interesting that the compiler can do, knowing that a pointer can't be NULL?
Sorry if I sound extremely negative. I think nonnull declarations in header files could serve some purpose for documentation, but I doubt it will improve performance or make it easier to find real bugs.
Since gcc can't detect indirect NULL values, the function may crash the process.
I'm not sure what you mean here, but I guess it won't work well to have the nonnull attribute propagate (say, in the same way as const). With the following code,
int foo (const char *a, const char *b) __attribute__ ((nonnull) { return strcmp (a, b); }
int bar (const char *a) { return foo (a, "foo"); }
should gcc issue a warning (since as far as we have told the compiler, bar (NULL) is allowed, but that is not allowed for it's call to foo)? Does it, or clang, issue warnings for such code?
- Since when (release and year) is it supported by gcc?
I found it to be for gcc >= 3.3, see http://ohse.de/uwe/articles/gcc-attributes.html#func-nonnull
Released back in 2004 (http://gcc.gnu.org/releases.html). Then I guess it shouldn't be too bad to use it unconditionally when compiling with gcc. Or check the gcc version predefines, that's not a very big hassle.
The reason I used nonnull were several compiler warnings. The current libc6 header files seem to use nonnull by default (gcc 4.7.1, libc6 2.13).
Can you quote the warnings in question? I'd like to understand why it complains.
Maybe there should be -std=c89 in den CFLAGS to prevent that ?
That would make sense. Or at least *some* flag to issue warnings for c99 additions like C++-style comments and declarations not at the start of a block.
Regards, /Niels
Am Freitag, 7. September 2012 schrieb Niels Möller:
Tim Ruehsen tim.ruehsen@gmx.de writes:
#define NONNULL(args) __attribute__ ((nonnull args))
Yes, functions with several non-null args than need several NONNULL(x) occurrences.
I don't think so. NONNULL((1,3,5)) should work, with "(1,3,5)" treated as a single argument by the preprocessor.
Yes, I didn't think of that.
I would actually prefer to have NONNULL declaration on each argument in question, just like UNUSED, but if I understand it correctly, that won't work.
No, that won't work.
The idea of nonnull is to enable the compiler detecting possible NULL arguments where they must not be. E.g. char *s=NULL; strlen(s); would let
the
compiler print a warning.
I'm not entirely convinced about the usefulness of that.
At the same time, when optimizing a function with a nonnull argument, gcc optimizes away checks against NULL for the appropriate variables.
I'm not entirely convinced that makes for any real improvements of performance on real code.
Don't get me wrong. It is not about performance. It is about bugs detected by the compiler before they happen. assert() only detects these NULL args if your testing reaches this special point. The compiler is theoretically able to find these at compile time.
And I think I'd rather get a warning from the compiler about a useless comparison (like, like gcc warns for for unsigned x = ...; if (x >= 0)...) than having the optimizer silently remove the code.
In particular, if I have a funtion where a pointer must not be NULL, and I have an assert (p != NULL); to that effect, then I will *not* consider it an improvement if the compiler decides to remove that assert.
Is there anything more useful and interesting that the compiler can do, knowing that a pointer can't be NULL?
Sorry if I sound extremely negative. I think nonnull declarations in header files could serve some purpose for documentation, but I doubt it will improve performance or make it easier to find real bugs.
Again, not performance. Bug detection while compilation.
Since gcc can't detect indirect NULL values, the function may crash the process.
I'm not sure what you mean here, but I guess it won't work well to have the nonnull attribute propagate (say, in the same way as const). With the following code,
int foo (const char *a, const char *b) __attribute__ ((nonnull) { return strcmp (a, b); }
int bar (const char *a) { return foo (a, "foo"); }
should gcc issue a warning (since as far as we have told the compiler, bar (NULL) is allowed, but that is not allowed for it's call to foo)? Does it, or clang, issue warnings for such code?
I just put the code into a C file. As expected (and documented, if you read the link): gcc does not detect the implicit call. scan-build (clang analyzer) does !!
- Since when (release and year) is it supported by gcc?
I found it to be for gcc >= 3.3, see http://ohse.de/uwe/articles/gcc-attributes.html#func-nonnull
Released back in 2004 (http://gcc.gnu.org/releases.html). Then I guess it shouldn't be too bad to use it unconditionally when compiling with gcc. Or check the gcc version predefines, that's not a very big hassle.
The reason I used nonnull were several compiler warnings. The current
libc6
header files seem to use nonnull by default (gcc 4.7.1, libc6 2.13).
Can you quote the warnings in question? I'd like to understand why it complains.
Just use scan-build/scan-view and you have in the report.
Maybe there should be -std=c89 in den CFLAGS to prevent that ?
That would make sense. Or at least *some* flag to issue warnings for c99 additions like C++-style comments and declarations not at the start of a block.
Regards, /Niels
Have a nice weekend.
Tim
Tim Ruehsen tim.ruehsen@gmx.de writes:
In cast128.c I removed the wiping of t, l and r. Instead I set t=0 at the beginning of the loops (It seemed to be used uninitialized in F1 macro).
I don't think the wiping has any meaning. Perhaps it was intended to somehow avoid leaking intermediate values. (I'm not the original author of this file).
I don't understand the comment on t being used before initialized. As far as I see, all of F1, F2 and F3 first assign t, then apply some sbox substitutions on it. Am I missing something?
If you are addressing some compiler warnings, please, *pretty please*, quote them accurately.
I do get several warnings (false positives) on a different function in this file when I compile with gcc:
cast128.c: In function `nettle_cast128_set_key': cast128.c:276:1: warning: `t[3]' may be used uninitialized in this function [-Wmaybe-uninitialized] cast128.c:210:26: warning: `z[3]' may be used uninitialized in this function [-Wmaybe-uninitialized]
and a bunch more similar ones. I think I'd prefer to reorganize this function completely, I find the current organization hard to understand.
diff --git a/examples/eratosthenes.c b/examples/eratosthenes.c index 2b9d04f..0eea941 100644 --- a/examples/eratosthenes.c +++ b/examples/eratosthenes.c @@ -399,5 +399,10 @@ main (int argc, char **argv) printf("%lu\n", n); } }
- /* silence static analyzers */
- free(block);
- free(sieve);
- return EXIT_SUCCESS;
}
Why this? It ought to be the job of the C library to free all malloced storage after main returns. Do you get a warning for this? That sounds overly noisy to me.
diff --git a/examples/io.c b/examples/io.c index 7b2289c..cda9ed3 100644 --- a/examples/io.c +++ b/examples/io.c @@ -125,7 +125,10 @@ read_file(const char *name, unsigned max_size, char **contents) fclose(f);
/* NUL-terminate the data. */
- buffer[done] = '\0';
- if (buffer)
- buffer[done] = '\0';
- else
- done = 0; *contents = buffer;
This ought not to happen. Hmm. I think the only way the execution can omit the realloc call (and the check for NULL return value) in the loop is if feof returns true immediately after fopen. Does you analyzer agree with that?
Then the correct fix is to move the feof call to after fread, as suggested in the preceding FIXME comment.
diff --git a/examples/nettle-benchmark.c b/examples/nettle-benchmark.c index b76a91c..48e53d0 100644 --- a/examples/nettle-benchmark.c +++ b/examples/nettle-benchmark.c @@ -96,7 +96,7 @@ static double frequency = 0.0; #define BENCH_ITERATIONS 10 #endif
-static void +static void NORETURN die(const char *format, ...) { va_list args;
Applied.
diff --git a/tools/misc.h b/tools/misc.h index 70c9eeb..af55998 100644 --- a/tools/misc.h +++ b/tools/misc.h @@ -28,19 +28,10 @@ #endif
void -die(const char *format, ...) -#if __GNUC___
__attribute__((__format__ (__printf__,1, 2)))__attribute__((__noreturn__))-#endif
;+die(const char *format, ...) PRINTF_STYLE(1,2) NORETURN;
void -werror(const char *format, ...) -#if __GNUC___
__attribute__((__format__ (__printf__,1, 2)))-#endif
;+werror(const char *format, ...) PRINTF_STYLE(1,2);
void * xalloc(size_t size);
Applied. I found one more werror prototype, in examples/io.h, which I improved in the same way (maybe that was in another patch of yours? I haven't reviewed all yet).
Regards, /Niels
Am Friday 07 September 2012 schrieb Niels Möller:
Tim Ruehsen tim.ruehsen@gmx.de writes:
In cast128.c I removed the wiping of t, l and r. Instead I set t=0 at the beginning of the loops (It seemed to be used uninitialized in F1 macro).
I don't think the wiping has any meaning. Perhaps it was intended to somehow avoid leaking intermediate values. (I'm not the original author of this file).
I don't understand the comment on t being used before initialized. As far as I see, all of F1, F2 and F3 first assign t, then apply some sbox substitutions on it. Am I missing something?
You are right (my editor 'macro-expanded' F1 to a pretty ugly term - I just oversaw that t is being initialized.). The line t = 0 can be removed in cast128_encrypt() and cast128_decrypt(), sorry.
If you are addressing some compiler warnings, please, *pretty please*, quote them accurately.
Some maintainers really want that, others not. I try to keep that in mind for the nettle project ...
I do get several warnings (false positives) on a different function in this file when I compile with gcc:
cast128.c: In function `nettle_cast128_set_key': cast128.c:276:1: warning: `t[3]' may be used uninitialized in this function [-Wmaybe-uninitialized] cast128.c:210:26: warning: `z[3]' may be used uninitialized in this function [-Wmaybe-uninitialized]
and a bunch more similar ones. I think I'd prefer to reorganize this function completely, I find the current organization hard to understand.
I took a look into these functions and decided not to change them. It would be pretty easy to silence these warnings (just clear the t[] and z[] arrays. The performance penalty will be hardly measurable. But this is a decision I did not want to make.
diff --git a/examples/eratosthenes.c b/examples/eratosthenes.c index 2b9d04f..0eea941 100644 --- a/examples/eratosthenes.c +++ b/examples/eratosthenes.c @@ -399,5 +399,10 @@ main (int argc, char **argv)
printf("%lu\n", n);}
}
/* silence static analyzers */
free(block);
free(sieve);
return EXIT_SUCCESS;
}
Why this? It ought to be the job of the C library to free all malloced storage after main returns. Do you get a warning for this? That sounds overly noisy to me.
Here are some reasons why silencing 'false positives' makes sense: 1. Each person new to the project will see these warnings and will waste time in viewing and understanding the source, just to say "oh yeah, it is a false positive - i'll keep that in mind...". It is a waste of precious human resources.
2. People might copy&paste the code (for whatever reasons) to a function which name is not "main".
3. How do you want to use valgrind for automatic memory leak testing ?
4. "main" is just a function name and only special if you link with the standard C library. A static analyzer can't be shure that the programmer will use some other mechanisms to start the program, so it complains about memory leaks even in the main() function. As a programmer, I really want this behaviour.
5. It is just a matter of good coding style. Are you going to say "ahhh, it is the main function - I don't need to call free() here." ?
There might be more reasons.
diff --git a/examples/io.c b/examples/io.c index 7b2289c..cda9ed3 100644 --- a/examples/io.c +++ b/examples/io.c @@ -125,7 +125,10 @@ read_file(const char *name, unsigned max_size, char **contents)
fclose(f);
/* NUL-terminate the data. */
- buffer[done] = '\0';
if (buffer)
buffer[done] = '\0';
else
done = 0;
*contents = buffer;
This ought not to happen. Hmm. I think the only way the execution can omit the realloc call (and the check for NULL return value) in the loop is if feof returns true immediately after fopen. Does you analyzer agree with that?
Then the correct fix is to move the feof call to after fread, as suggested in the preceding FIXME comment.
Well, I don't mind how you fix that warning. But the bare fact, that you have to think about it, tells me that there is something wrong in that function. Maybe not practically, but for someone who wants to understand the function. If you are not shure, if a variable will be initialized or not: just initialize it to make it 100% clear.
Regards, Tim
Tim Ruehsen tim.ruehsen@gmx.de writes:
- It is just a matter of good coding style.
I'll consider adding those free calls (motivated by the "good coding style" argument, with no comment about static analyzers).
Are you going to say "ahhh, it is the main function - I don't need to call free() here." ?
Yes, I'd definitely say that. And I would expect a static analyzer to do the same (either by default, or with some reasonable command line flag). I have a hard time to view this as a memory leak or a problem at all.
I'm a bit curious on how you and the analyzer reason about the following three examples.
1.
int main (int argv, char **argv) { char *p = malloc (4711); return 0; }
2.
int main (int argv, char **argv) { char *p = malloc (4711); exit (0); }
3.
static void foo (void) { fprintf (stderr, "Failed!\n"); exit (1); }
int main (int argv, char **argv) { char *p = malloc (4711); foo (); free (p); /* Let's keep the main function itself "clean" */ return 0; }
Is there any good reason to say that that in one of these cases, it's a memory leak, while in some of the other cases, it is not?
To arrange for proper freeing before program exit in case (3) (or more complex real programs with that pattern), one would have to add an atexit call in main and potentially also in lots of other functions doing allocation. That would really clutter the code, for very little benefit.
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
To arrange for proper freeing before program exit in case (3) (or more complex real programs with that pattern), one would have to add an atexit call in main and potentially also in lots of other functions doing allocation. That would really clutter the code, for very little benefit.
One benefit is that you can run the program under valgrind and it won't report any memory leaks. I find this quite useful for self-tests, when you want to catch memory leaks in a library.
The case is less strong for examples though, however people tend to cut'n'paste example code and if the original is missing proper calls to free, so will the copied code, and it may end up in a library or further down the call path where the memory leak might actually hurt.
/Simon
Am Monday 10 September 2012 schrieb Niels Möller:
Are you going to say "ahhh, it is the main function - I don't need to call free() here." ?
Yes, I'd definitely say that. And I would expect a static analyzer to do the same (either by default, or with some reasonable command line flag). I have a hard time to view this as a memory leak or a problem at all.
Then you don't close files, sockets, locks etc. either just because your are in main ? IMHO, you should change your habits... ;-)
As Simon Josefsson made clear, the main reason are automated tests for memory leaks using valgrind. Something, that a library test suite should not miss.
In nettle/run-tests just replace the line "$1" $testflags by valgrind --error-exitcode=1 --leak-check=full --show-reachable=yes "$1" $testflags
and 'make check': ... ===================== 36 of 51 tests failed =====================
But without freeing resources in main(), you now can't easily say, if you have a problem in the library or in the testsuite.
BTW, as you see, the static analyzers won't find all of these problems.
I'm a bit curious on how you and the analyzer reason about the following three examples.
...
Is there any good reason to say that that in one of these cases, it's a memory leak, while in some of the other cases, it is not?
To arrange for proper freeing before program exit in case (3) (or more complex real programs with that pattern), one would have to add an atexit call in main and potentially also in lots of other functions doing allocation. That would really clutter the code, for very little benefit.
Yes and No. It really depends on what YOU - as the programmer - want.
For libraries functions I would say, they should never call exit() etc. You normally have a matching pair of open/close, init/deinit etc. to be able to control resource freeing.
For applications, programmers normally spend some afford to at least be able to free all resources right before (non-error) exit(). To have at least some test-cases to check for resource leaks. Especially long-running apps need something like that.
If you decide, that 'emergency exits' on fatal errors doesn't have to free all resources, that is perfectly fine. And I know of none project that doesn't handle it in that practical way. Except libraries.
BTW, using the clang static analyzer is easy and straight forward. - cd into nettle - make clean - scan-build ./configure - scan-build make scan-build: 3 bugs found. scan-build: Run 'scan-view /tmp/scan-build-2012-09-11-1' to examine bug reports. - scan-view /tmp/scan-build-2012-09-11-1
Of course, there might be some false positives, but clang 3.1.1 does a pretty good job.
There is also cppcheck, another static analyzer. It may find other things than clang. - cd into nettle - cppcheck --enable=all .
e.g.: [testsuite/yarrow-test.c:141]: (warning) %d in format string (no. 1) requires a signed integer given in the argument list. [testsuite/yarrow-test.c:163]: (warning) %d in format string (no. 1) requires a signed integer given in the argument list. [testsuite/yarrow-test.c:168]: (warning) %d in format string (no. 1) requires a signed integer given in the argument list. [testsuite/yarrow-test.c:202]: (warning) %d in format string (no. 1) requires a signed integer given in the argument list. [testsuite/yarrow-test.c:220]: (error) Resource leak: input
Happy bug hunting ;-)
Regards, Tim
Tim Ruehsen tim.ruehsen@gmx.de writes:
Then you don't close files, sockets, locks etc. either just because your are in main ? IMHO, you should change your habits... ;-)
At some point, I choose to trust that the operating system and C library does its job. E.g, if I set the close-on-exec flag on some file descriptors, I don't try to close them explicitly between fork and exec. And "close-at-exit" seems like a more basic feature of the system.
Then I'm aware that for output files, one ought to close them and check the return value. In particular if using FILE, since it's bad if closing fails silently due to full disks at the time buffered data is written out. But I guess one can't close the C library stdout; there, fflush is the best one can do.
As Simon Josefsson made clear, the main reason are automated tests for memory leaks using valgrind. Something, that a library test suite should not miss.
In nettle/run-tests just replace the line "$1" $testflags by valgrind --error-exitcode=1 --leak-check=full --show-reachable=yes "$1" $testflags
and 'make check': ... ===================== 36 of 51 tests failed =====================
I see. So from this perspective, it's desirable that the code path leading to exit(EXIT_SUCCESS) frees all allocated storage. While it doesn't matter for failure exits.
And one ought to run the tests both with and without valgrind, to get proper testing also for testscases where programs are expected to fail (and I guess one could also make careful use of the value of the exit code, not jsut chek thhat it's non-zero).
Regards, /Niels
Tim Ruehsen tim.ruehsen@gmx.de writes:
As Simon Josefsson made clear, the main reason are automated tests for memory leaks using valgrind. Something, that a library test suite should not miss.
...
But without freeing resources in main(), you now can't easily say, if you have a problem in the library or in the testsuite.
However, as far as I recall, the Nettle library does not contain any calls to malloc or other memory allocating functions. So I don't think there could be any memory leaks from the library?
If it is a huge effort to release memory from the testsuite, it may not be worth it. But if it is a small effort, and it leads to being able to run all the testsuite under valgrind without any warnings, I would support doing it.
/Simon
Simon Josefsson simon@josefsson.org writes:
However, as far as I recall, the Nettle library does not contain any calls to malloc or other memory allocating functions. So I don't think there could be any memory leaks from the library?
The publickey functions which use gmp allocate memory for the bignums (unfortunately). There may even be some leaks there in the form of missing mpz_clear, so testing for memory leaks would be useful. The functions using nettle_buffer (mainly sexp_format) may allocate, depending on how the buffer is initialized.
But if it is a small effort, and it leads to being able to run all the testsuite under valgrind without any warnings, I would support doing it.
I don't think it's a big deal to get the testsuite to deallocate storage. Not a very high priority for me, though.
I think we should also modify the use of $EMULATOR to make it easier to run with valgrind (mainly, remove quoting when it's expanded for the shell), and perhaps rename it to TESTS_ENVIRONMENT for consistency with projects using automake.
Regards, /Niels
If it is a huge effort to release memory from the testsuite, it may not be worth it. But if it is a small effort, and it leads to being able to run all the testsuite under valgrind without any warnings, I would support doing it.
Yes, it was some hours of work, but here is a patch that releases all memory and now runs fine with valgrind.
There is just changed file outside testsuite/ and examples/ : buffer.c. nettle_buffer_clear() used to call realloc() with a length of 0 to wipe the allocated memory. But at least here it comes back with a valid pointer to one allocated byte (as valgrind reports). I use free() after the realloc if the returned pointer is not NULL (maybe realloc() should not be called at all !?). Niels, please have a look at it.
I did not remove or add any tests, just changed / added / removed some stuff from testutils.c and of course within the *-test.c(xx).
Niels, please remove the quotes from "$EMULATOR" in run-tests (my run-tests is somehow already in the local repo with some other changes I just can't revert). Than we could use EMULATOR="valgrind -q --error-exitcode=1 --leak-check=full --show- reachable=yes" to do an automated memory check.
A short overview of the changed files: # modified: buffer.c # modified: examples/rsa-encrypt.c # modified: examples/rsa-keygen.c # modified: testsuite/aes-test.c # modified: testsuite/arcfour-test.c # modified: testsuite/arctwo-test.c # modified: testsuite/bignum-test.c # modified: testsuite/blowfish-test.c # modified: testsuite/camellia-test.c # modified: testsuite/cast128-test.c # modified: testsuite/cbc-test.c # modified: testsuite/ctr-test.c # modified: testsuite/cxx-test.cxx # modified: testsuite/des-test.c # modified: testsuite/des3-test.c # modified: testsuite/gcm-test.c # modified: testsuite/hmac-test.c # modified: testsuite/md2-test.c # modified: testsuite/md4-test.c # modified: testsuite/md5-compat-test.c # modified: testsuite/md5-test.c # modified: testsuite/memxor-test.c # modified: testsuite/random-prime-test.c # modified: testsuite/ripemd160-test.c # modified: testsuite/rsa2sexp-test.c # modified: testsuite/salsa20-test.c # modified: testsuite/serpent-test.c # modified: testsuite/sexp-format-test.c # modified: testsuite/sexp-test.c # modified: testsuite/sexp2rsa-test.c # modified: testsuite/sha1-test.c # modified: testsuite/sha224-test.c # modified: testsuite/sha256-test.c # modified: testsuite/sha384-test.c # modified: testsuite/sha512-test.c # modified: testsuite/testutils.c # modified: testsuite/testutils.h # modified: testsuite/twofish-test.c # modified: testsuite/yarrow-test.c # modified: tools/pkcs1-conv.c
Regards, Tim
Tim Ruehsen tim.ruehsen@gmx.de writes:
There is just changed file outside testsuite/ and examples/ : buffer.c. nettle_buffer_clear() used to call realloc() with a length of 0 to wipe the allocated memory. But at least here it comes back with a valid pointer to one allocated byte (as valgrind reports). I use free() after the realloc if the returned pointer is not NULL (maybe realloc() should not be called at all !?).
This is a bit tricky. The nettle_buffer interface lets the user configure memory allocation using a single function pointer to the user's realloc function. It's not required to be compatible in any way with libc free.
My man page for realloc says
realloc() changes the size of the memory block pointed to by ptr to size bytes. [...] if size is equal to zero, and ptr is not NULL, then the call is equivalent to free(ptr).
The spec at
http://pubs.opengroup.org/onlinepubs/9699919799/functions/realloc.html
seems to agree. But when describing the return value, it allows a valid pointer to be returned,
If size is 0, either a null pointer or a unique pointer that can be successfully passed to free() shall be returned.
Does that mean that the spec allows the program
int main (int argc, char **argv) { void *p; while ( (p = malloc(1)) ) realloc (p, 0); return 1; }
to leak one byte of storage per iteration? If we need a workaround, the right place is in the realloc wrapppers in realloc.c.
I did not remove or add any tests, just changed / added / removed some stuff from testutils.c and of course within the *-test.c(xx).
I think it would have been good to discuss the aproach on list, before doing that work.
Niels, please remove the quotes from "$EMULATOR" in run-tests (my run-tests is somehow already in the local repo with some other changes I just can't revert).
I'm aware that needs fixing.
int test_main(void) { /* 128 bit keys */
- test_cipher(&nettle_aes128,
HL("0001020305060708 0A0B0C0D0F101112"),HL("506812A45F08C889 B97F5980038B8359"),H("D8F532538289EF7D 06B506A4FD5BE9C9"));
- test_cipher(&nettle_aes128,
"0001020305060708 0A0B0C0D0F101112","506812A45F08C889 B97F5980038B8359","D8F532538289EF7D 06B506A4FD5BE9C9");
The reason I introduced the HL and LDATA macros was that I find it useful that the testcases can chose either hex data or raw binary data. A change requiring hex everywhare is not so attractive. Maybe the macros can be fixed to arrange for deallocation (e.g, put all allocated strings on a list and free it at the end of main). Or, of we pass only a const char * to the various functions, include some prefix to say if it's hex or raw data (with length; I'd prefer to not rely on strlen here).
- unsigned key_length = decode_hex_length(key_hex);
- const uint8_t *key = decode_hex_dup(key_hex);
- unsigned length = decode_hex_length(cleartext_hex);
- const uint8_t *cleartext = decode_hex_dup(cleartext_hex);
- const uint8_t *ciphertext = decode_hex_dup(ciphertext_hex);
- struct arctwo_ctx ctx; uint8_t *data = xalloc(length);
@@ -46,6 +50,9 @@ test_arctwo(unsigned ekb, FAIL();
free(data);
- free((void *)ciphertext);
- free((void *)cleartext);
- free((void *)key);
}
It's unfortunate that the free prototype doesn't take a const void *, but we have to live with that. In this case, I think it's preferable to drop const from the declaration of those pointers, so there's no need for those explicit casts.
The less intrusive changes I'll try to get to within a few days.
Regards, /Niels
Am Friday 14 September 2012 schrieb Niels Möller:
Tim Ruehsen tim.ruehsen@gmx.de writes:
There is just changed file outside testsuite/ and examples/ : buffer.c. nettle_buffer_clear() used to call realloc() with a length of 0 to wipe the allocated memory. But at least here it comes back with a valid pointer to one allocated byte (as valgrind reports). I use free() after the realloc if the returned pointer is not NULL (maybe realloc() should not be called at all !?).
This is a bit tricky. The nettle_buffer interface lets the user configure memory allocation using a single function pointer to the user's realloc function. It's not required to be compatible in any way with libc free.
My man page for realloc says
realloc() changes the size of the memory block pointed to by ptr to size bytes. [...] if size is equal to zero, and ptr is not NULL, then the call is equivalent to free(ptr).
My man page says (Embedded GNU C Library 2.13) If size was equal to 0, either NULL or a pointer suitable to be passed to free() is returned. CONFORMING TO C89, C99.
My change handles both cases.
If we need a workaround, the right place is in the realloc wrapppers in realloc.c.
We need a workaround since different libraries handle it differently. But simplified it is just a if (p) free(p);
int test_main(void) {
/* 128 bit keys */
- test_cipher(&nettle_aes128,
HL("0001020305060708 0A0B0C0D0F101112"),HL("506812A45F08C889 B97F5980038B8359"),H("D8F532538289EF7D 06B506A4FD5BE9C9"));
- test_cipher(&nettle_aes128,
"0001020305060708 0A0B0C0D0F101112","506812A45F08C889 B97F5980038B8359","D8F532538289EF7D 06B506A4FD5BE9C9");The reason I introduced the HL and LDATA macros was that I find it useful that the testcases can chose either hex data or raw binary data. A change requiring hex everywhare is not so attractive.
I handled that case in hmac-test.c by introducing a 'flags' param that gives a hint which of the data params is hex and which is not.
e.g. hmac_test(alg_md5, MD5_DIGEST_SIZE, HEX_NONE, "Jefe", "what do ya want for nothing?", "750c783e6ab0b503 eaa86e310a5db738");
hmac_test(alg_md5, MD5_DIGEST_SIZE, (HEX_KEY|HEX_MSG), "aaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaa", "dddddddddddddddd dddddddddddddddd" "dddddddddddddddd dddddddddddddddd" "dddddddddddddddd dddddddddddddddd" "dddd", "56be34521d144c88 dbb8c733f0e8b3f6");
- unsigned key_length = decode_hex_length(key_hex);
- const uint8_t *key = decode_hex_dup(key_hex);
- unsigned length = decode_hex_length(cleartext_hex);
- const uint8_t *cleartext = decode_hex_dup(cleartext_hex);
- const uint8_t *ciphertext = decode_hex_dup(ciphertext_hex);
...
- free((void *)ciphertext);
- free((void *)cleartext);
- free((void *)key);
It's unfortunate that the free prototype doesn't take a const void *, but we have to live with that. In this case, I think it's preferable to drop const from the declaration of those pointers, so there's no need for those explicit casts.
I personally prefer a macro, something like #define xfree(a) do { if (a) { free((void *)(a)); a=NULL; } } while (0)
The less intrusive changes I'll try to get to within a few days.
You could tell which points you would like to have changed, and i'll change them here. After we are done, the patch could be applied in whole. I'll do the coding and you the review...
Regards, Tim
Tim Ruehsen tim.ruehsen@gmx.de writes:
Am Friday 14 September 2012 schrieb Niels Möller:
If we need a workaround, the right place is in the realloc wrapppers in realloc.c.
We need a workaround since different libraries handle it differently. But simplified it is just a if (p) free(p);
I think something like this (untested) should solve the problem. We should also document somewhere that buffer->realloc(p, 0) is expected to free the storage completely.
--- a/realloc.c +++ b/realloc.c @@ -34,13 +34,25 @@ void * nettle_realloc(void *ctx UNUSED, void *p, unsigned length) { + if (length == 0) + { + free (p); + return NULL; + } return realloc(p, length); }
void * nettle_xrealloc(void *ctx UNUSED, void *p, unsigned length) { - void *n = realloc(p, length); - if (length && !n) + void *n; + if (length == 0) + { + free (p); + return NULL; + } + + n = realloc(p, length); + if (!n) { fprintf(stderr, "Virtual memory exhausted.\n");
You could tell which points you would like to have changed, and i'll change them here. After we are done, the patch could be applied in whole. I'll do the coding and you the review...
Thanks. Then I'd like you to try to keep the interface of the HL, H, LDATA, LDUP macros intact, so you don't need to modify each and every test case, but add whatever additional machinery is needed to avoid memory leaks (linked list of allocated strings, GNU obstack, allocation from a static area, or whatever seems suitable). For the test files, I don't think it's a problem if we keep the allocations a bit longer than necessary. If that approach gets too painful, we'll have to consider some other way.
I prefer to do things in small pieces, so if I get to it before the rest of the fixes are ready, I'll check in missing mpz_clear and similar.
Regards, /Niels
You could tell which points you would like to have changed, and i'll change them here. After we are done, the patch could be applied in whole. I'll do the coding and you the review...
Thanks. Then I'd like you to try to keep the interface of the HL, H, LDATA, LDUP macros intact, so you don't need to modify each and every test case, but add whatever additional machinery is needed to avoid memory leaks (linked list of allocated strings, GNU obstack, allocation from a static area, or whatever seems suitable).
Here is just my opinion - it is not meant to offend you:
The HL and LDUP macros were really ugly and in very most cases not even needed. I already replaced them by extending the basic testroutines. So there is no need for linked list or whatever to keep track of allocated memory. Keeping it simple should be the preferred way.
If the day comes, that we have to further extend basic testroutines, because we *need* some mixture or HEX/BASE64/String/memory or whatever, we will extend them. Not now, because it is just not needed right now.
You are the maintainer and you decide what you like and what not. But if you insist in keeping the H, HL, LDUP macros, I have to cancel my offer, sorry. I can't work against my strong conviction.
Regards, Tim
Tim Ruehsen tim.ruehsen@gmx.de writes:
The HL and LDUP macros were really ugly and in very most cases not even needed.
They're intended as a kind of "mini-language" for the tests. I have earlier (in the lsh project, years ago) tried using m4 for the same purpose, but in the end I concluded that it was overal less trouble to just use the C preprocessor.
What in particular do you find ugly? That they expand to two arguments? Short cryptic names? Something else?
Wold you like them better if they expanded to some constructor calls for proper string objects olding both length and data, which were then passed to the various test helper functions? I don't care very much about the implementation of the mini-language.
But if you insist in keeping the H, HL, LDUP macros, I have to cancel my offer, sorry.
I'll do that myself then.
Regards, /Niels
Am Friday 14 September 2012 schrieb Niels Möller:
Tim Ruehsen tim.ruehsen@gmx.de writes:
The HL and LDUP macros were really ugly and in very most cases not even needed.
They're intended as a kind of "mini-language" for the tests. I have earlier (in the lsh project, years ago) tried using m4 for the same purpose, but in the end I concluded that it was overal less trouble to just use the C preprocessor.
But after all, you still won't need them. Just have a look in the patched hmac-test.c, where you can find are mixtures of hex data and strings.
What in particular do you find ugly? That they expand to two arguments? Short cryptic names? Something else?
Yes. Yes. Yes. But the main uglyness is that they don't track memory allocations. You could of course (as you proposed) include some memory tracking structures/functions. But that would even be less elegant (brrr..., it shakes me a bit).
Wold you like them better if they expanded to some constructor calls for proper string objects olding both length and data, which were then passed to the various test helper functions? I don't care very much about the implementation of the mini-language.
Yes, that is an alternative, though it requires to throw away most of the already working patches (sniff). H() would merge into HL().
Proposal (not tested): typedef struct { unsigned length; uint8_t *data; } testdata_t;
/* example HL() implementation with just one malloc() */ testdata_t *HL(const uint8_t *hex) { unsigned length = decode_hex_length(hex); testdata_t *data = xalloc(sizeof(testdata_t) + length);
data.length = length; data.data = &data.data + sizeof(data.data); decode_hex(data.data, hex);
return data; }
/* example test function with my first patch */ void test_cipher(const struct nettle_cipher *cipher, const uint8_t *key_hex, const uint8_t *cleartext_hex, const uint8_t *ciphertext_hex) { unsigned key_length = decode_hex_length(key_hex); const uint8_t *key = decode_hex_dup(key_hex); unsigned length = decode_hex_length(cleartext_hex); const uint8_t *cleartext = decode_hex_dup(cleartext_hex); const uint8_t *ciphertext = decode_hex_dup(ciphertext_hex);
...
free((void *)ciphertext); free((void *)cleartext); free((void *)key); }
/* example test function with testdata_t */ void test_cipher(const struct nettle_cipher *cipher, testdata_t *key, testdata_t *cleartext, testdata_t *ciphertext) { // use key.length instead of key_length, etc. ...
free(ciphertext); free(cleartext); free(key); }
/* example call to test function */ test_cipher(&nettle_aes128, HL("0001020305060708 0A0B0C0D0F101112"), HL("506812A45F08C889 B97F5980038B8359"), HL("D8F532538289EF7D 06B506A4FD5BE9C9"));
BTW xmalloc should be slightly changed to not allow size 0 (either fail or allocated a 1 byte size). From my man pages: If size is 0, then malloc() returns either NULL, or a unique pointer value that can later be successfully passed to free().
Regards, Tim
Tim Ruehsen tim.ruehsen@gmx.de writes:
But after all, you still won't need them. Just have a look in the patched hmac-test.c, where you can find are mixtures of hex data and strings.
I haven't read the complete patch very carefully. In hmac-test.c, you have introduced a flag argument, which is not as clumsy as I had expected (but I prefer my preprocessor hack before your algorithm enum and corresponding switch statement; I think it's clear that we have somewhat different taste).
In other places, you have introduced additional functions, like test_hash and test_hash_hex. I prefer a style which looks more like functional programming, with a single test_hash using a canonical representation of the input, and then the caller invokes any helper macro or function needed to convert data from whatever form is most convenient in the test file.
Then I happily admit that the current implementation with macros is not very beautiful.
/* example test function with testdata_t */ void test_cipher(const struct nettle_cipher *cipher, testdata_t *key, testdata_t *cleartext, testdata_t *ciphertext) { // use key.length instead of key_length, etc. ...
free(ciphertext); free(cleartext); free(key); }
In this model, we have a producer/consumer convention. The passed in data is *always* malloced, to be freed by the test function. That's a good idea, if we now consider deallocation important, that convention will simplify things (at the cost of an unnecessary allocation in the few cases where we now pass a string literal as the pointer).
xmalloc should be slightly changed to not allow size 0 (either fail or allocated a 1 byte size). From my man pages: If size is 0, then malloc() returns either NULL, or a unique pointer value that can later be successfully passed to free().
I don't see the problem.
First, it is important to note that callers of xalloc are not expected to check the return value of xalloc, it will never return NULL except maybe when size == 0, and in that case it does *not* indicate that allocation failed. By definition, if xalloc returned, then it did not fail.
Noow, xalloc is sometimes called with size 0, for example if you do decode_hex_dup(""). That's why its check for malloc failing also has to check size. It will succeed (i.e., not abort) no matter which convention for size == 0 which malloc follows, and simply return whatever malloc returns.
As far as I see, it doesn't matter if malloc (and hence xalloc) returns NULL or a valid pointer, in either case, the caller should not dereference the pointer, and eventually pass it to on to free.
In which way do you think it fails?
Reggards, /Niels
Am Friday 14 September 2012 schrieb Niels Möller:
Tim Ruehsen tim.ruehsen@gmx.de writes:
But after all, you still won't need them. Just have a look in the patched hmac-test.c, where you can find are mixtures of hex data and strings.
I haven't read the complete patch very carefully. In hmac-test.c, you have introduced a flag argument, which is not as clumsy as I had expected (but I prefer my preprocessor hack before your algorithm enum and corresponding switch statement; I think it's clear that we have somewhat different taste).
I just saw how the _NETTLE_HASH macro is working. Using that, the enum could be removed. E.g. by including md5-meta.h, we have a nettle_md5 variable declared, which holds all we need (context, init, update, maybe not a set_key function). At least something like that.
In other places, you have introduced additional functions, like test_hash and test_hash_hex. I prefer a style which looks more like functional programming, with a single test_hash using a canonical representation of the input, and then the caller invokes any helper macro or function needed to convert data from whatever form is most convenient in the test file.
Ok, the proposed HL() function would obsolete test_hash_hex().
Then I happily admit that the current implementation with macros is not very beautiful.
:-)
/* example test function with testdata_t */ void test_cipher(const struct nettle_cipher *cipher,
testdata_t *key, testdata_t *cleartext, testdata_t *ciphertext){
// use key.length instead of key_length, etc. ...
free(ciphertext); free(cleartext); free(key);
}
In this model, we have a producer/consumer convention. The passed in data is *always* malloced, to be freed by the test function. That's a good idea, if we now consider deallocation important, that convention will simplify things (at the cost of an unnecessary allocation in the few cases where we now pass a string literal as the pointer).
Not the allocation/strdup in unnecessary. It is just the call to decode_hex_length().
xmalloc should be slightly changed to not allow size 0 (either fail or allocated a 1 byte size). From my man pages: If size is 0, then malloc() returns either NULL, or a unique pointer value that can later be successfully passed to free().
I don't see the problem.
First, it is important to note that callers of xalloc are not expected to check the return value of xalloc, it will never return NULL except maybe when size == 0, and in that case it does *not* indicate that allocation failed. By definition, if xalloc returned, then it did not fail.
Noow, xalloc is sometimes called with size 0, for example if you do decode_hex_dup(""). That's why its check for malloc failing also has to check size. It will succeed (i.e., not abort) no matter which convention for size == 0 which malloc follows, and simply return whatever malloc returns.
As far as I see, it doesn't matter if malloc (and hence xalloc) returns NULL or a valid pointer, in either case, the caller should not dereference the pointer, and eventually pass it to on to free.
My proposed HL() routine expects xalloc never to return NULL. But that is easy to check.
The only argument for a change is, that malloc and thus xalloc behaves different on different plattforms. A defined behaviour of xalloc for all plattforms would make it more predictable. It is just a practical consideration for testing. Does not really matter to me.
Regards, Tim
Tim Ruehsen tim.ruehsen@gmx.de writes:
Yes, it was some hours of work, but here is a patch that releases all memory and now runs fine with valgrind.
I've now done the same, in a slightly different manner. I first tried to keep the convention that strings are passed as two arguments (length, pointer), and tried to make callers always allocate the data, and callees always free it. That turned out to be a bit inconvenient in a few places. So I rewrote it with a simple string class, and then I could just as well put all allocated strings on a linked list to be freed at the end.
I'll try to clean it up and commit it soon.
I just pushed two simpler fixes, not quoting $EMULATOR in run-tests, and memory leaks in the pkcs1-conv command line tool.
Regards, /Niels
Am Sunday 16 September 2012 schrieb Niels Möller:
Tim Ruehsen tim.ruehsen@gmx.de writes:
Yes, it was some hours of work, but here is a patch that releases all memory and now runs fine with valgrind.
I've now done the same, in a slightly different manner. I first tried to keep the convention that strings are passed as two arguments (length, pointer), and tried to make callers always allocate the data, and callees always free it. That turned out to be a bit inconvenient in a few places. So I rewrote it with a simple string class, and then I could just as well put all allocated strings on a linked list to be freed at the end.
I'll try to clean it up and commit it soon.
Looks like you meanwhile comitted it.
There are just two little things left open: * added mpz_clear in rsa-encrypt.c * added nettle_buffer_clear, rsa_private_key_clear and free in rsa-keygen.c
Regards, Tim
diff --git a/examples/rsa-encrypt.c b/examples/rsa-encrypt.c index 70d1503..ca1a8cd 100644 --- a/examples/rsa-encrypt.c +++ b/examples/rsa-encrypt.c @@ -253,7 +253,9 @@ main(int argc, char **argv) }
write_bignum(stdout, x); - + + mpz_clear(x); + if (!process_file(&ctx, stdin, stdout)) return EXIT_FAILURE; diff --git a/examples/rsa-keygen.c b/examples/rsa-keygen.c index 0ca39b4..165e6f4 100644 --- a/examples/rsa-keygen.c +++ b/examples/rsa-keygen.c @@ -160,5 +160,12 @@ main(int argc, char **argv) return EXIT_FAILURE; }
+ nettle_buffer_clear(&pub_buffer); + nettle_buffer_clear(&priv_buffer); + rsa_private_key_clear(&priv); + rsa_public_key_clear(&pub); + free(pub_name); + + return EXIT_SUCCESS; }
Tim Ruehsen tim.ruehsen@gmx.de writes:
There are just two little things left open:
- added mpz_clear in rsa-encrypt.c
- added nettle_buffer_clear, rsa_private_key_clear and free in rsa-keygen.c
Thanks. Applied.
/Niels
If it is a huge effort to release memory from the testsuite, it may not be worth it. But if it is a small effort, and it leads to being able to run all the testsuite under valgrind without any warnings, I would support doing it.
Yesterday, I send a 170Kb patch to the list... but it did not appear. What can/should I do ?
Regards, Tim
Tim Ruehsen tim.ruehsen@gmx.de writes:
Yesterday, I send a 170Kb patch to the list... but it did not appear. What can/should I do ?
It was held for moderation due to its size (current limit seems to be 40 KB). I just approved it. Did you get any useful reply from the list manager?
Regards, /Niels
Am Friday 14 September 2012 schrieb Niels Möller:
Tim Ruehsen tim.ruehsen@gmx.de writes:
Yesterday, I send a 170Kb patch to the list... but it did not appear. What can/should I do ?
It was held for moderation due to its size (current limit seems to be 40 KB). I just approved it. Did you get any useful reply from the list manager?
No, I didn't.
Regards, Tim
Tim Ruehsen tim.ruehsen@gmx.de writes:
diff --git a/examples/base16enc.c b/examples/base16enc.c index c3cb58f..3fc410e 100644 --- a/examples/base16enc.c +++ b/examples/base16enc.c @@ -47,21 +47,6 @@ int main(int argc UNUSED, char **argv UNUSED) {
- /* "buffer" will hold the bytes from disk: */
- uint8_t * buffer = (uint8_t *) malloc (CHUNK_SIZE * sizeof(uint8_t));
- if (buffer == NULL) {
- fprintf (stderr, "Cannot allocate read buffer.\n");
- return EXIT_FAILURE;
- }
- /* "result" will hold bytes before output: */
- uint8_t * result = (uint8_t *) malloc (ENCODED_SIZE * sizeof(uint8_t));
- if (result == NULL) {
- fprintf (stderr, "Cannot allocate write buffer.\n");
- return EXIT_FAILURE;
- }
#ifdef WIN32 _setmode(0, O_BINARY); #endif
Applied.
Thanks, /Niels
Tim Ruehsen tim.ruehsen@gmx.de writes:
diff --git a/pgp-encode.c b/pgp-encode.c index 9a69922..f84373c 100644 --- a/pgp-encode.c +++ b/pgp-encode.c @@ -246,7 +246,6 @@ pgp_put_rsa_sha1_signature(struct nettle_buffer *buffer, unsigned hash_end; unsigned sub_packet_start; uint8_t trailer[6];
uint8_t digest16[2]; mpz_t s;
/* Signature packet. The packet could reasonably be both smaller and
diff --git a/rsa2openpgp.c b/rsa2openpgp.c index c4666f3..4c62f49 100644 --- a/rsa2openpgp.c +++ b/rsa2openpgp.c @@ -64,7 +64,6 @@ rsa_keypair_to_openpgp(struct nettle_buffer *buffer, time_t now = time(NULL);
unsigned key_start;
unsigned key_length; unsigned userid_start;
struct sha1_ctx key_hash;
Applied.
diff --git a/desdata.c b/desdata.c index fc89c2d..6671b5d 100644 --- a/desdata.c +++ b/desdata.c @@ -62,7 +62,7 @@ int sorder[] = { 7, 5, 3, 1, 6, 4, 2, 0, };
-int printf(const char *, ...); +int printf(const char *, ...) PRINTF_STYLE(1,2);
int main(int argc UNUSED, char **argv UNUSED)
Changed it to include stdio.h instead.
diff --git a/examples/io.h b/examples/io.h index ff4a18d..e83b7eb 100644 --- a/examples/io.h +++ b/examples/io.h @@ -37,11 +37,7 @@ void * xalloc(size_t size);
void -werror(const char *format, ...) -#if __GNUC___
__attribute__((__format__ (__printf__,1, 2)))-#endif
;+werror(const char *format, ...) PRINTF_STYLE(1,2);
/* If size is > 0, read at most that many bytes. If size == 0,
- read until EOF. Allocates the buffer dynamically. */
Did this a few days ago.
index 48e53d0..3d07868 100644 --- a/examples/nettle-benchmark.c +++ b/examples/nettle-benchmark.c @@ -96,7 +96,7 @@ static double frequency = 0.0; #define BENCH_ITERATIONS 10 #endif
-static void NORETURN +static void NORETURN PRINTF_STYLE(1,2) die(const char *format, ...) { va_list args;
Applied.
Thanks, /Niels
nettle-bugs@lists.lysator.liu.se
-
nisse＠lysator.liu.se -
Simon Josefsson -
Tim Ruehsen -
Tim Rühsen