On Saturday 01 March 2008 21:12, Jan-Oliver Wagner wrote:
> On Saturday 01 March 2008 17:23, [EMAIL PROTECTED] wrote:
> > --- trunk/openvas-libraries/libopenvas/proctitle.c 2008-02-29 22:43:58
> > UTC (rev 693) +++
> > trunk/openvas-libraries/libopenvas/proctitle.c 2008-03-01 16:23:49 UTC
> > (rev 694) @@ -31,7 +31,7 @@
> > environ = (char **) emalloc((sizeof (char *) * (i + 1))+envpsize+1);
> > s = ((char *)environ)+((sizeof (char *) * (i + 1)));
> > for (i = 0; envp[i] != NULL; i++){
> > - strcpy(s,envp[i]);
> > + strncpy(s,envp[i],strlen(envp[i]));
>
> shouldn't in general the length of the destination be the limiting factor?
Yes. Furthermore, the new code is actually wrong. Well, more precisely
it's probably OK in the particular case you quoted, but in general,
replacing strcpy(dest, src) with strncpy(dest, src, strlen(src)) is
wrong. There are at least two problems:
1. Different guarantees regarding the terminating null character.
strcpy also copies the terminating null character. The strncpy call
doesn't because it always copies exactly the number of characters
given in the third parameter and strlen doesn't count the terminating null.
In the code quoted above this is not actually a problem because
emalloc zeros the memory it allocates. However, in other places
where strcpy has been replaced with strncpy, erealloc was used to
make sure the destination memory block was big enaough. erealloc
simply calls realloc and does not zero any new memory if the memory
block has grown (it couldn't because it doesn't know how large the
block was before the realloc call).
2. The size parameter should indicate how much space is left in dest.
Obviously, simply using strlen(src) as the size defeats its purpose.
OTOH, simply using the remaining size of dest is also not
unproblematic (from the glibc manual):
Using `strncpy' as opposed to `strcpy' is a way to avoid bugs
relating to writing past the end of the allocated space for TO.
However, it can also make your program much slower in one common
case: copying a string which is probably small into a potentially
large buffer. In this case, SIZE may be large, and when it is,
`strncpy' will waste a considerable amount of time copying null
characters.
So, in all, I think the changes (rev. 694 and 687) actually lower the
quality of the code. The original strcpy calls were not wrong, so a
simple /* Flawfinder: ignore */ comment would have sufficed. It would also
have the advantage that we can later tell flawfinder to ignore those
comments to get it to recheck the code.
Many places where strcpy is used actually concatenate several strings.
It would be better to introduce a function along the lines of the second
concat function example in [1]. This would make the code more readable
by avoiding code duplication and it would reduce the number of places
where buffer lengths have to be determined.
Bernhard
[1] GNU C Library Manual
http://www.gnu.org/software/libc/manual/html_node/Copying-and-Concatenation.html
--
Bernhard Herzog Intevation GmbH, Osnabrück
Amtsgericht Osnabrück, HR B 18998 http://www.intevation.de/
Geschäftsführer: Frank Koormann, Bernhard Reiter, Dr. Jan-Oliver Wagner
pgpteZS7qDACe.pgp
Description: PGP signature
_______________________________________________ Openvas-devel mailing list [email protected] http://lists.wald.intevation.org/mailman/listinfo/openvas-devel
