From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.33) id 1Bc76U-0002Hz-PN for qemu-devel@nongnu.org; Sun, 20 Jun 2004 14:31:42 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.33) id 1Bc76U-0002Hn-8M for qemu-devel@nongnu.org; Sun, 20 Jun 2004 14:31:42 -0400 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.33) id 1Bc76U-0002Hk-5n for qemu-devel@nongnu.org; Sun, 20 Jun 2004 14:31:42 -0400 Received: from [80.91.224.249] (helo=main.gmane.org) by monty-python.gnu.org with esmtp (Exim 4.34) id 1Bc75E-0002NX-7t for qemu-devel@nongnu.org; Sun, 20 Jun 2004 14:30:24 -0400 Received: from root by main.gmane.org with local (Exim 3.35 #1 (Debian)) id 1Bc75D-0007kP-00 for ; Sun, 20 Jun 2004 20:30:23 +0200 Received: from dyn-83-157-70-117.ppp.tiscali.fr ([83.157.70.117]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Sun, 20 Jun 2004 20:30:23 +0200 Received: from gmane by dyn-83-157-70-117.ppp.tiscali.fr with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Sun, 20 Jun 2004 20:30:23 +0200 From: "Charlie Gordon" Date: Sun, 20 Jun 2004 20:22:44 +0200 Message-ID: References: <200406181841.i5IIfZQa019337@treas.simtreas.ru><40D3ED57.2040506@simtreas.ru> <20040619150514.GB1962@sentinelchicken.org> Sender: news Subject: [Qemu-devel] Re: [PATCH] security_20040618 Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org "Tim" 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.