From patchwork Fri Sep 16 18:24:11 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 671023 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]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3sbNwd6WB4z9s2Q for ; Sat, 17 Sep 2016 04:24:33 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=fk/8rG/T; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:subject:from:to:cc:date:in-reply-to:references :content-type:mime-version:content-transfer-encoding; q=dns; s= default; b=UdyNK1yXbWhvN3oiyLG+aionuUxj0MqNpKiBl2F3BbCwm3wWwNLzp ZNSp8xnpVkNewgh9xLVchqkRRGQ1cND763/WO2KLg+lkiKJcexSrZNUpsaQtnnoD 73wPWgrDGcKs4gHGiOyQ93BgFZBS9eragkP6jxJJ9UDml4tB+720BM= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:subject:from:to:cc:date:in-reply-to:references :content-type:mime-version:content-transfer-encoding; s=default; bh=oXGYbR/t4MJuhdohzlvPd7Bu3BY=; b=fk/8rG/TcZs3lCi5opok9cfK63/h AZE2zH9Fc45ZqIJAsOeMNAndUKY+QKY/FDvwRxaCczIxaUhSFV5+6WgyX7K0/0Wu pHC9RDIfeq4L2pVawRnhR4oZEZ7y2xbbpffnBV9SLf7ZBbR5U7QwKUUKeEX90pPn 3SUDtvmORV6RnAY= Received: (qmail 19772 invoked by alias); 16 Sep 2016 18:24:26 -0000 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 Received: (qmail 19762 invoked by uid 89); 16 Sep 2016 18:24:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.4 required=5.0 tests=BAYES_00, KAM_ASCII_DIVIDERS, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=combining, consisting, love X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 16 Sep 2016 18:24:15 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C7B3F13A60; Fri, 16 Sep 2016 18:24:13 +0000 (UTC) Received: from vpn-233-121.phx2.redhat.com (vpn-233-121.phx2.redhat.com [10.3.233.121]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u8GIOBeC024736; Fri, 16 Sep 2016 14:24:11 -0400 Message-ID: <1474050251.6782.70.camel@redhat.com> Subject: Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905) From: David Malcolm To: Martin Sebor , Gcc Patch List Cc: Joseph Myers , Jeff Law , Richard Biener , Jakub Jelinek , Bernd Schmidt , Manuel =?ISO-8859-1?Q?L=F3pez-Ib=E1=F1ez?= , Florian Weimer Date: Fri, 16 Sep 2016 14:24:11 -0400 In-Reply-To: <57D60D03.7080601@gmail.com> References: <5776B33E.2080504@gmail.com> <577A8D6A.3070902@gmail.com> <578D512F.9050909@gmail.com> <9bb5ad66-4985-8c42-f800-4d84e0e18659@redhat.com> <57A3AFFF.7090109@gmail.com> <57AD30E5.3000801@gmail.com> <22a47656-c23c-4840-2e49-a59f4af513b1@redhat.com> <57B725F6.8000405@gmail.com> <110cfc6b-7856-9b51-885f-05402b14fc3e@redhat.com> <57D1B5F0.1030504@gmail.com> <57D60D03.7080601@gmail.com> Mime-Version: 1.0 X-IsSubscribed: yes On Sun, 2016-09-11 at 20:03 -0600, Martin Sebor wrote: > On 09/08/2016 04:10 PM, Joseph Myers wrote: > > On Thu, 8 Sep 2016, Martin Sebor wrote: > > > > > diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in > > > index da133a4..4607495 100644 > > > --- a/gcc/doc/tm.texi.in > > > +++ b/gcc/doc/tm.texi.in > > > @@ -4081,6 +4081,13 @@ In either case, it remains possible to > > > select code-generation for the alternate > > > scheme, by means of compiler command line switches. > > > @end defmac > > > > > > +@deftypefn {Target Hook} {const char *} > > > TARGET_LIBC_PRINTF_POINTER_FORMAT (tree, const char **@var{flags} > > > ) > > > +A hook to determine the target @code{printf} implementation > > > format string > > > +that the most closely corresponds to the @code{%p} format > > > directive. > > > +The object pointed to by the @var{flags} is set to a string > > > consisting > > > +of recognized format flags such as the @code{'#'} character. > > > +@end deftypefn > > > > No, the substance of hook documentation should go in target.def > > with just > > an @hook line in tm.texi.in leading to the documentation going in > > tm.texi > > automatically. > > > > You appear to be defining a target macro masquerading as a hook. > > Please > > don't (new target macros should be avoided where possible); use a > > proper > > hook. (Maybe the settings depending on OS rather than architecture > > means > > it needs to be one of those whose default is a manual setting in > > target-def.h rather than automatically generated, but that should > > be the > > limit of deviation from the normal workings of hooks.) > > > > > + const char *pfmt = TARGET_LIBC_PRINTF_POINTER_FORMAT (arg, > > > &flags); > > > > With a proper hook them you'd call > > targetm.libc_printf_pointer_format. > > > > > + inform (callloc, > > > + (nbytes + exact == 1 > > > + ? "format output %wu byte into a destination of > > > size %wu" > > > + : "format output %wu bytes into a destination > > > of size %wu"), > > > + nbytes + exact, info.objsize); > > > > You need to use G_() around both format strings in such a case; > > xgettext > > doesn't know how to extract them both. > > Attached is revision 8 of the patch (hopefully) adjusted as > requested above, and with a simple test with of the multiline > diagnostic directives suggested by David. This revision also > enables the -fprintf-return-value option by default. The libgomp > test failures I was seeing in my earlier testing must have been > caused by an older version of GMP or MPFR that I had inadvertently > use (normally I use in-tree versions downloaded and expanded there > by the download_prerequisites script but that time I forgot that > step). > > David, in the way of feedback, I spent hours debugging the simple > multiline test I added. It seems that DejaGnu is exquisitely > sensitive to whitespaces in the multiline output. I appreciate > that spacing is essential to lining up the caret and the tildes > with the text of the program but tests fail even when all the > expected output is lined up right in the multiline directive but > off by a space (or tab) with respect to the actual output. That > DejaGnu output doesn't make this very clear at all makes debugging > these types of issues a pain. I wonder if there's a better to do > this. Gah; I'm sorry this was such a pain to do. I tend to just copy&paste the stuff I want to quote directly from Emacs's compilation buffer. However a nit, which I think is why you had problems... You have two pairs of dg-begin/end-multiline-output, but I think it's better to have four; use a single begin/end pair for each call to diagnostic_show_locus. Hopefully doing that ought to make things a bit less sensitive to whitespace, and easier to debug: each begin/end can be handled by itself, whereas by combining them it's harder for multiline.exp to detect them. If combined, they can only be detected if all of the dg-warning/dg-messages work (and are thus pruned by prune.exp), if any aren't pruned, the multiline outputs will also fail. Maybe this exacerbated the problem? So something like this, grouping the 4 diagnostics together with their diagnostic_show_locus output by opening and closing comments (line numbers will need adjusting): +void test (void) +{ + sprintf (dst + 7, "%-s", "1"); + /* { dg-warning "writing a terminating nul past the end of the destination" "" { target *-*-*-* } 10 } + { dg-begin-multiline-output "" } + sprintf (dst + 7, "%-s", "1"); + ^~~~~ + { dg-end-multiline -output "" } */ + /* { dg-message "format output 2 bytes into a destination of size 1" "" { target *-*-*-* } 10 } + { dg-begin -multiline-output "" } + sprintf (dst + 7, "%-s", "1"); + ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + { dg-end-multiline-output "" } */ + + sprintf (dst + 7, "%-s", "abcd"); + /* { dg-warning ".%-s. directive writing 4 bytes into a region of size 1" "" { target *-*-*-* } 20 } + { dg-begin-multiline-output "" } + sprintf (dst + 7, "%-s", "abcd"); + ^~~ ~~~~~~ + { dg-end-multiline-output "" } */ + /* { dg-message "format output 5 bytes into a destination of size 1" "" { target *-*-*-* } 20 } + { dg-begin-multiline-output "" } + sprintf (dst + 7, "%-s", "abcd"); + ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + { dg-end-multiline-output "" } */ +} Another nit, if I may: FWIW I'm not in love with the wording of the messages. Sorry to bikeshed, but how about: warning: buffer overflow will occur when writing terminating NUL and: note: formatted output of 2 bytes into a destination of size 1 or somesuch. diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c new file mode 100644 index 0000000..a601ba4 --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c @@ -0,0 +1,29 @@ +/* { dg-do compile } */ +/* { dg-options "-Wformat -Wformat-length=1 -fdiagnostics-show-caret -ftrack-macro-expansion=0" } */ + +extern int sprintf (char*, const char*, ...); + +char dst [8]; + +void test (void) +{ + sprintf (dst + 7, "%-s", "1"); + /* { dg-warning "writing a terminating nul past the end of the destination" "" { target *-*-*-* } 10 } + { dg-message "format output 2 bytes into a destination of size 1" "" { target *-*-*-* } 10 } + { dg-begin-multiline-output "" } + sprintf (dst + 7, "%-s", "1"); + ^~~~~ + sprintf (dst + 7, "%-s", "1"); + ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + { dg-end-multiline-output "" } */ + + sprintf (dst + 7, "%-s", "abcd"); + /* { dg-warning ".%-s. directive writing 4 bytes into a region of size 1" "" { target *-*-*-* } 20 } + { dg-message "format output 5 bytes into a destination of size 1" "" { target *-*-*-* } 20 } + { dg-begin-multiline-output "" } + sprintf (dst + 7, "%-s", "abcd"); + ^~~ ~~~~~~ + sprintf (dst + 7, "%-s", "abcd"); + ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + { dg-end-multiline-output "" } */ +}