From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.33) id 1Bc7zf-00058M-NW for qemu-devel@nongnu.org; Sun, 20 Jun 2004 15:28:43 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.33) id 1Bc7zd-00056X-Bl for qemu-devel@nongnu.org; Sun, 20 Jun 2004 15:28:42 -0400 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.33) id 1Bc7zd-00056U-7m for qemu-devel@nongnu.org; Sun, 20 Jun 2004 15:28:41 -0400 Received: from [206.72.67.39] (helo=claudius.sentinelchicken.org) by monty-python.gnu.org with smtp (Exim 4.34) id 1Bc7xv-0005s1-7t for qemu-devel@nongnu.org; Sun, 20 Jun 2004 15:26:55 -0400 Date: Sun, 20 Jun 2004 12:26:52 -0700 From: Tim Subject: Re: [Qemu-devel] Re: [PATCH] security_20040618 Message-ID: <20040620192652.GA1927@sentinelchicken.org> References: <20040619150514.GB1962@sentinelchicken.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 > > 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