From patchwork Thu Apr 19 13:01:31 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jim Meyering X-Patchwork-Id: 153748 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 490AAB7006 for ; Thu, 19 Apr 2012 23:02:17 +1000 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1335445338; h=Comment: DomainKey-Signature:Received:Received:Received:Received:From:To: Cc:Subject:In-Reply-To:References:Date:Message-ID:Lines: MIME-Version:Content-Type:Content-Transfer-Encoding:Mailing-List: Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:Sender:Delivered-To; bh=KbnAUbFQjPHIhiWx+JN7/l8mfjw=; b=PBebYjrq3MW8CsXHT25/JzRUM9PsMu8051ja35Mc1cUutUAOG+U5LoNqejAJMY R5wRQyokfTAIbeiHeCRJ2+8LmUHjUt1X5weZWCH+1VuulRDdcQ5akwvAZFHhKmvJ /jBy05CqN/lp/aN8cBdwcKcgqYF1RI1GHvtd7yLJ8YCQw= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID:Lines:MIME-Version:Content-Type:Content-Transfer-Encoding:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=FFqmTPWnx+k9WHmldaGHdOCEEQibSRLmVD0WBfksOewoW0x1IG3o9TCkgVuZrY PImZYGB9jjYZ5vfXiztAPk+2hyRI37xkkySEk/Os6eshJ8gCsZJjgmfOuAvn19AR 7CS/gpu1c9T2rfUCyRrXBSO7u9qOHDPpJWArYfVjBBA40=; Received: (qmail 16824 invoked by alias); 19 Apr 2012 13:02:09 -0000 Received: (qmail 16794 invoked by uid 22791); 19 Apr 2012 13:02:06 -0000 X-SWARE-Spam-Status: No, hits=-3.1 required=5.0 tests=AWL, BAYES_00, KHOP_THREADED, TW_CP, TW_TM, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx.meyering.net (HELO mx.meyering.net) (88.168.87.75) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 19 Apr 2012 13:01:33 +0000 Received: from rho.meyering.net (localhost.localdomain [127.0.0.1]) by rho.meyering.net (Acme Bit-Twister) with ESMTP id AB3D6600B8; Thu, 19 Apr 2012 15:01:31 +0200 (CEST) From: Jim Meyering To: Richard Guenther Cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] genmodes: remove misleading use of strncpy In-Reply-To: (Richard Guenther's message of "Thu, 19 Apr 2012 14:31:37 +0200") References: <87hawgq9yg.fsf@rho.meyering.net> <87fwbzq5qr.fsf@rho.meyering.net> Date: Thu, 19 Apr 2012 15:01:31 +0200 Message-ID: <871unjq3is.fsf@rho.meyering.net> Lines: 122 MIME-Version: 1.0 X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Richard Guenther wrote: > On Thu, Apr 19, 2012 at 2:13 PM, Jim Meyering wrote: >> Richard Guenther wrote: >>> On Thu, Apr 19, 2012 at 12:42 PM, Jim Meyering wrote: >>>> Found by inspection. >>>> Seeing this strncpy use without the usual following NUL-termination, >>>> my reflex was that it was a buffer overrun candidate, but then I >>>> realized this is gcc, so that's not very likely. >>>> Looking a little harder, I saw the preceding strlen >= sizeof buf test, >>>> which means there is no risk of overrun. >>>> >>>> That preceding test means the use of strncpy is misleading, since >>>> with that there is no risk of the name being too long for the buffer, >>>> and hence no reason to use strncpy. >>>> One way to make the code clearer is to use strcpy, as done below. >>>> An alternative is to use memcpy (buf, m->name, strlen (m->name) + 1). >> >> Thanks for the quick feedback. >> >>> Without a comment before the strcpy this isn't any more clear than >> >> Good point. >> How about with this folded in? >> >> diff --git a/gcc/genmodes.c b/gcc/genmodes.c >> index f786258..3833017 100644 >> --- a/gcc/genmodes.c >> +++ b/gcc/genmodes.c >> @@ -452,6 +452,7 @@ make_complex_modes (enum mode_class cl, >>       if (cl == MODE_FLOAT) >>        { >>          char *p, *q = 0; >> +         /* We verified above that m->name+NUL fits in buf.  */ >>          strcpy (buf, m->name); >>          p = strchr (buf, 'F'); >>          if (p == 0) > > That's better. Or even cache the strlen result and use memcpy here > as Jakub suggested. > >>> when using strncpy.  Also your issue with the terminating NUL is >>> still present for the snprintf call done later (in fact the code looks like >>> it can be optimized, avoiding the strcpy in the path to the snprintf). >> >> Isn't it different with snprintf? >> While strncpy does *NOT* necessarily NUL-terminate, >> snprintf explicitly does, always. > > Sure, my point was that the > > if (strlen (m->name) >= sizeof buf) > { > error ("%s:%d:mode name \"%s\" is too long", > m->file, m->line, m->name); > continue; > } > > check does not account for the (conditional) prepending of 'C' and the > snprintf would silently discard the last character of the name. Yes, you're right. It's good to avoid snprintf and its truncation, too. >> IMHO, if you'd like to avoid the duplication in the strcpy/snprintf >> path, that deserves to be a different change. > > Sure. > > The patch is ok with caching strlen and using memcpy. Like this, I presume: [alternatively, declare and compute m_len on a separate line, just before the comparison: size_t m_len = strlen (m->name); I'd actually prefer that, but don't know if decl-after-stmt is ok here. ] 2012-04-19 Jim Meyering genmodes: remove misleading use of strncpy * genmodes.c (make_complex_modes): Avoid unnecessary use of strncpy. We verified above that the string(including trailing NUL) fits in buf, so just use memcpy. --- 1.7.10.208.gb4267 diff --git a/gcc/genmodes.c b/gcc/genmodes.c index 8b6f5bc..484a6ac 100644 --- a/gcc/genmodes.c +++ b/gcc/genmodes.c @@ -1,5 +1,5 @@ /* Generate the machine mode enumeration and associated tables. - Copyright (C) 2003, 2004, 2005, 2006, 2007, 2010 + Copyright (C) 2003, 2004, 2005, 2006, 2007, 2010, 2012 Free Software Foundation, Inc. This file is part of GCC. @@ -435,11 +435,13 @@ make_complex_modes (enum mode_class cl, for (m = modes[cl]; m; m = m->next) { + size_t m_len; + /* Skip BImode. FIXME: BImode probably shouldn't be MODE_INT. */ if (m->precision == 1) continue; - if (strlen (m->name) >= sizeof buf) + if ((m_len = strlen (m->name)) >= sizeof buf) { error ("%s:%d:mode name \"%s\" is too long", m->file, m->line, m->name); @@ -452,7 +454,8 @@ make_complex_modes (enum mode_class cl, if (cl == MODE_FLOAT) { char *p, *q = 0; - strncpy (buf, m->name, sizeof buf); + /* We verified above that m->name+NUL fits in buf. */ + memcpy (buf, m->name, m_len + 1); p = strchr (buf, 'F'); if (p == 0) q = strchr (buf, 'D');