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

[Tim]

> > Yeah, you are probably right.  I looked at that one on 3 seperate
> > occasions before making the change, since I recognized that there are
> > very few conditions where it could possibly be a problem, and come to
> > think of it, this fix doesn't mitigate those conditions.
> >
> > That chunk of code makes me uncomfortable for other reasons though (does
> > qemu_malloc() return NULL ever?  could buf possibly be missing a
> > trailing '\0' ever?) so I'll re-visit it again and see what makes the
> > most sense.  The pstrcpy isn't hurting anything though.  Slightly slower
> > copy, due to the length checking, but it isn't in a critical piece of
> > code (monitor.c is just for the user interface command prompt, right?),
> > so I also don't see a reason to remove it, esp if changes in the future
> > open up the possibility of an overflow.
> 
> No, he is ABSOLUTELY right !  This patch is useless and confusing.
> pstrcpy's second argument is the size of the buffer the first argument
> points to, computing it a second time is error prone as it duplicates
> qemu_malloc size argument calculation.  Many modifications down the road,
> these two lines may become sufficiently distant that a modification to one
> will not be reflected in the other.  Furthermore, it is completely innane to
> require protection from buffer overflow in pstrcpy by passing the exact size
> of the third argument !  It reminds me of an overtly cautious programmer I
> met a few years ago who made sure 'strings were properly null terminated by
> adding : str[strlen(str)] = '\0';  what a joke!
> 
> Thus I see no reason to ACCEPT such a patch, removing it a just a matter of
> cleanliness.
> 
> Still there was a more constructive remark to make on the above code as it
> would definitely be better written as
> 
> str = qemu_strdup(buf);
> 
> with obvious semantics for qemu_strdup.
> 
> Anyone to write such a patch and use qemu_strdup in other places as
> appropriate ?
> 
> Charlie Gordon.
> 
> PS: I fully agree with Fabrice about strncpy, disbelievers should read `man
> strncpy` carefully and learn.


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?

thanks,
tim