Am Friday 07 September 2012 schrieb Niels Möller:
> Tim Ruehsen <[email protected]> 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
_______________________________________________
nettle-bugs mailing list
[email protected]
http://lists.lysator.liu.se/mailman/listinfo/nettle-bugs
