Re: [Qemu-devel] Re: Re: [PATCH] security_20040618

[Tim <tim-qemu@sentinelchicken.org>]

> > Based on comments received thus far, including yours, I am reviewing
> > that section of code (as I mentioned above), and will be releasing a new
> > revision of the patch in a day or two.  I admit, I am not a perfect
> > programmer.  I am merely trying to help out by fixing the tiny problems
> > that are often missed by programmers that have more important things to
> > worry about.  I appreciate it when people show me where I am wrong, but
> > could you please keep your criticism a bit more constructive?
> 
> Sorry if I sounded a bit harsh, I'm sure every contribution is appreciated,
> and your submitting patches is more helpful than my criticizing them...

That's ok, I was just taken aback a bit.

> I merely wanted to emphasize how broken strncpy is and how much more useful
> pstrcpy is.
>
> My suggestion on qemu_strdup is imho constructive :-)  and about the only
> reason this is not completely off topic ;-)
> 
> But as far as strnpy is concerned, I *want* to be destructive : this C
> library function is a mess, it doesn't do what most C programmers
> believe.  It causes bugs, or blatant inefficiencies due to the inept null
> padding on large buffers.
> It is so unlikely that the precise behaviour of that horrible thing be what
> is needed in any C program...
> There are quite a few problems around uses of this function even in gnu
> software or the linux kernel.

Yes, since Fabrice pointed out the differences between my version of
pstrcpy() and his, I have come to appreciate pstrcpy()'s correctness and
speed.  I see what you mean by the problems with strncpy(), and I'll
make an effort to eliminate it with the same prejudice I have tried to
eliminate most strcpy() and sprintf() calls.

> There are other candidates for libc functions every programmer should reject
> disgruntedly : sprintf, gets, strtok, mktemp, tmpnam,
> tempnam... or any of the C library functions duly tagged in the man pages as
> never to be used
> Similarly, extreme care is needed in do/while loops...

Agreed.  In this day and age, there's really no excuse to use unsafe
functions such as these.  That's why I attempt to eliminate them with my
patch even though there may be no immediately obvious overflow issue.

Thanks for the additional comments & information.

cheers,
tim