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