[Qemu-devel] Re: [PATCH] security_20040618

["Charlie Gordon" <gmane@chqrlie.org>]

"Tim" <tim-qemu@sentinelchicken.org> wrote in message
news:20040619150514.GB1962@sentinelchicken.org...
> > > --- qemu-current/monitor.c 2004-06-16 20:49:59.000000000 -0700
> > > +++ qemu-dev/monitor.c 2004-06-17 22:12:49.000000000 -0700
> > >                  str = qemu_malloc(strlen(buf) + 1);
> > > -                strcpy(str, buf);
> > > +                pstrcpy(str, strlen(buf) + 1,  buf);
> >
> > In my opinion, it already absolutely unnecessary correction.
> > There is in this place no problem.
>
> 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.