From patchwork Thu Dec 15 04:21:12 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Sebor X-Patchwork-Id: 88103 Delivered-To: patch@linaro.org Received: by 10.140.20.101 with SMTP id 92csp585844qgi; Wed, 14 Dec 2016 20:21:36 -0800 (PST) X-Received: by 10.99.143.9 with SMTP id n9mr576213pgd.133.1481775696584; Wed, 14 Dec 2016 20:21:36 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id i7si237893pli.9.2016.12.14.20.21.36 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 14 Dec 2016 20:21:36 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-444470-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org; spf=pass (google.com: domain of gcc-patches-return-444470-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-444470-patch=linaro.org@gcc.gnu.org; dmarc=fail (p=NONE dis=NONE) header.from=gmail.com DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=WVnIRcbUVUcovI5BO QOXKxdkH5/1pGuPQu9SEUFR0Pbe/8E6iFoq+VuY4akH5DVDGQ8kX6xVd3HMjEdCE tIThEfglp9ttWVwv6rd7oQpWKTSk6LymtZvRLecmt+bQdeiG0oyBexM6gt9RJ8T6 Imchm0Pj185Ni4EY5v6Xi7Mfy4= 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 :subject:to:references:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=wZ0oLnEJ/9RMX52cD7vtMy0 NZro=; b=Ol3ymiHmbdPPhWwKjHK8cfZCXgp8L2KWlPhNVgqEif36zE4HICoc4Dq cNAL8TfiQh1SD5ljeQXOV1JEu4CLFhGyZQWT0xI8P21jgNZbyhIkKaCLt8DpUti5 DtQfNbaZqEClzzDlCeSUWQLKCnuh7vA3EZPyW4wa/SgJQ3gsCWKs= Received: (qmail 66180 invoked by alias); 15 Dec 2016 04:21:19 -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 66165 invoked by uid 89); 15 Dec 2016 04:21:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=warned, 439, 7, sk:number_ X-HELO: mail-qt0-f193.google.com Received: from mail-qt0-f193.google.com (HELO mail-qt0-f193.google.com) (209.85.216.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 15 Dec 2016 04:21:16 +0000 Received: by mail-qt0-f193.google.com with SMTP id l20so5506090qta.1 for ; Wed, 14 Dec 2016 20:21:16 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to; bh=U6s85Kmor8s/tZ6DzGXQDkvJ9ZkWYS3P8vGIiy1SEWU=; b=nAPtAfH4WBxn84Dp1RMpOXvak5v9S/q5I0MZmNJJH/PBbqhGW6eKFQZOE/1zrI3vuw nWHgFo/q6O2ayyzjaIuN9l+W04SI0+hCZC80krIQmlDFw75VBp0CjDeejYgR3jQ3L9DD zMnK+lyTJCWRADKsaIem+wrQS17MR7ElSpUwU9Sh6WwVUALFrzaPRdkdz5RJ6thx4r2Y pSotClcjDNx+ucle3PtWyxSW3u5Gj2A5WisoE39pKMDrQ94PM538nsfflpo3JDTclM3e Yd2JJs5RaosKjsX/u+yLqVF96zWOVAhES9prYAvcpxd/yVcrfrr/JEOhixNMFDrgOmyC /H3A== X-Gm-Message-State: AIkVDXKrjtk509/mIf5YP7iX8wvvUlbmalArWUCRsxUM/sX07hSKDowUPzCWBqOLXSVKDw== X-Received: by 10.200.45.129 with SMTP id p1mr233411qta.96.1481775674924; Wed, 14 Dec 2016 20:21:14 -0800 (PST) Received: from [192.168.0.26] (97-124-188-210.hlrn.qwest.net. [97.124.188.210]) by smtp.gmail.com with ESMTPSA id 6sm82083qke.18.2016.12.14.20.21.13 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 14 Dec 2016 20:21:14 -0800 (PST) Subject: Re: [PATCH] detect null sprintf pointers (PR 78519) To: Jeff Law , Gcc Patch List References: <8279f735-1d61-8eec-ba9b-c477481c2a54@gmail.com> <494f810a-69df-2008-3200-ba1949717559@redhat.com> <1014a36c-b949-3339-3bad-94d37ba479e3@gmail.com> <50ff4863-1dba-9bda-da41-922fa1ce6776@redhat.com> From: Martin Sebor Message-ID: <38cc207d-a4bf-e645-0562-c13a23fee025@gmail.com> Date: Wed, 14 Dec 2016 21:21:12 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <50ff4863-1dba-9bda-da41-922fa1ce6776@redhat.com> X-IsSubscribed: yes >> I suppose setting a range seemed better than giving up. Then again, >> since with this patch GCC will warn on null %s pointers there may >> not be much point in trying to see if there's also some other >> problem after that, except perhaps in code that deliberately relies >> on the Glibc feature. I'd be fine with just stopping at this point >> if you prefer. > I think I'd rather just stop at this point. I don't think we're gaining > much, if anything and encoding the glibc-ism in here seems like a mistake. That's fine. Attached is an updated patch with this change. Thanks Martin PR middle-end/78519 - missing warning for sprintf %s with null pointer gcc/ChangeLog: PR middle-end/78519 * gimple-ssa-sprintf.c (format_string): Handle null pointers. (format_directive): Diagnose null pointer arguments. (pass_sprintf_length::handle_gimple_call): Diagnose null destination pointers. Correct location of null format string in diagnostics. gcc/testsuite/ChangeLog: PR middle-end/78519 * gcc.dg/tree-ssa/builtin-sprintf-warn-7.c: New test. diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c index 5909e05..435c162 100644 --- a/gcc/gimple-ssa-sprintf.c +++ b/gcc/gimple-ssa-sprintf.c @@ -439,7 +439,7 @@ struct result_range struct fmtresult { fmtresult () - : argmin (), argmax (), knownrange (), bounded (), constant () + : argmin (), argmax (), knownrange (), bounded (), constant (), nullp () { range.min = range.max = HOST_WIDE_INT_MAX; } @@ -467,6 +467,9 @@ struct fmtresult are also constant (such as determined by constant propagation, though not value range propagation). */ bool constant; + + /* True when the argument is a null pointer. */ + bool nullp; }; /* Description of a conversion specification. */ @@ -1765,6 +1768,16 @@ format_string (const conversion_spec &spec, tree arg) res.range.min = 0; } } + else if (arg && integer_zerop (arg)) + { + /* Handle null pointer argument. */ + + fmtresult res; + res.range.min = 0; + res.range.max = HOST_WIDE_INT_MAX; + res.nullp = true; + return res; + } else { /* For a '%s' and '%ls' directive with a non-constant string, @@ -1910,13 +1923,27 @@ format_directive (const pass_sprintf_length::call_info &info, } } + if (fmtres.nullp) + { + fmtwarn (dirloc, pargrange, NULL, + OPT_Wformat_length_, + "%<%.*s%> directive argument is null", + (int)cvtlen, cvtbeg); + + /* Don't bother processing the rest of the format string. */ + res->warned = true; + res->number_chars = HOST_WIDE_INT_M1U; + res->number_chars_min = res->number_chars_max = res->number_chars; + return; + } + + bool warned = res->warned; + /* Compute the number of available bytes in the destination. There must always be at least one byte of space for the terminating NUL that's appended after the format string has been processed. */ unsigned HOST_WIDE_INT navail = min_bytes_remaining (info.objsize, *res); - bool warned = res->warned; - if (fmtres.range.min < fmtres.range.max) { /* The result is a range (i.e., it's inexact). */ @@ -2871,6 +2898,9 @@ pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator *gsi) return; } + /* The first argument is a pointer to the destination. */ + tree dstptr = gimple_call_arg (info.callstmt, 0); + info.format = gimple_call_arg (info.callstmt, idx_format); if (idx_dstsize == HOST_WIDE_INT_M1U) @@ -2878,7 +2908,7 @@ pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator *gsi) /* For non-bounded functions like sprintf, determine the size of the destination from the object or pointer passed to it as the first argument. */ - dstsize = get_destination_size (gimple_call_arg (info.callstmt, 0)); + dstsize = get_destination_size (dstptr); } else if (tree size = gimple_call_arg (info.callstmt, idx_dstsize)) { @@ -2948,6 +2978,20 @@ pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator *gsi) } else { + /* For calls to non-bounded functions or to those of bounded + functions with a non-zero size, warn if the destination + pointer is null. */ + if (integer_zerop (dstptr)) + { + /* This is diagnosed with -Wformat only when the null is a constant + pointer. The warning here diagnoses instances where the pointer + is not constant. */ + location_t loc = gimple_location (info.callstmt); + warning_at (EXPR_LOC_OR_LOC (dstptr, loc), + OPT_Wformat_length_, "null destination pointer"); + return; + } + /* Set the object size to the smaller of the two arguments of both have been specified and they're not equal. */ info.objsize = dstsize < objsize ? dstsize : objsize; @@ -2971,7 +3015,8 @@ pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator *gsi) /* This is diagnosed with -Wformat only when the null is a constant pointer. The warning here diagnoses instances where the pointer is not constant. */ - warning_at (EXPR_LOC_OR_LOC (info.format, input_location), + location_t loc = gimple_location (info.callstmt); + warning_at (EXPR_LOC_OR_LOC (info.format, loc), OPT_Wformat_length_, "null format string"); return; } diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-8.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-8.c new file mode 100644 index 0000000..2550065 --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-8.c @@ -0,0 +1,93 @@ +/* PR middle-end/78519 - missing warning for sprintf %s with null pointer + Also exercises null destination pointer and null format string. + { dg-do compile } + { dg-options "-O2 -Wformat -Wformat-length -Wno-nonnull -ftrack-macro-expansion=0" } */ + +typedef __builtin_va_list va_list; + +#define sprintf __builtin_sprintf +#define snprintf __builtin_snprintf +#define vsprintf __builtin_vsprintf +#define vsnprintf __builtin_vsnprintf + + +static char* null (void) +{ + return 0; +} + + +void sink (int); +#define T sink + + +/* Verify that calls with a null destination pointer are diagnosed. */ + +void test_null_dest (va_list va) +{ + char *p = null (); + T (sprintf (null (), "%i", 0)); /* { dg-warning "null destination pointer" } */ + T (sprintf (p, "%i", 0)); /* { dg-warning "null destination pointer" } */ + T (sprintf (p, "%i abc", 0)); /* { dg-warning "null destination pointer" } */ + + T (snprintf (null (), 1, "%i", 0)); /* { dg-warning "null destination pointer" } */ + T (snprintf (p, 2, "%i", 0)); /* { dg-warning "null destination pointer" } */ + T (snprintf (p, 3, "%i abc", 0)); /* { dg-warning "null destination pointer" } */ + + /* Snprintf with a null pointer and a zero size is a special request + to determine the size of output without writing any. Such calls + are valid must not be diagnosed. */ + T (snprintf (p, 0, "%i", 0)); + + T (vsprintf (null (), "%i", va)); /* { dg-warning "null destination pointer" } */ + T (vsprintf (p, "%i", va)); /* { dg-warning "null destination pointer" } */ + + T (vsnprintf (null (), 1, "%i", va)); /* { dg-warning "null destination pointer" } */ + T (vsnprintf (p, 2, "%i", va)); /* { dg-warning "null destination pointer" } */ + T (vsnprintf (p, 0, "%i", va)); +} + +void test_null_format (char *d, va_list va) +{ + char *fmt = null (); + + T (sprintf (d, null ())); /* { dg-warning "null format string" } */ + T (sprintf (d, fmt)); /* { dg-warning "null format string" } */ + + T (snprintf (d, 0, null ())); /* { dg-warning "null format string" } */ + T (snprintf (d, 1, fmt)); /* { dg-warning "null format string" } */ + + T (vsprintf (d, null (), va)); /* { dg-warning "null format string" } */ + T (vsprintf (d, fmt, va)); /* { dg-warning "null format string" } */ + + T (vsnprintf (d, 0, null (), va)); /* { dg-warning "null format string" } */ + T (vsnprintf (d, 1, fmt, va)); /* { dg-warning "null format string" } */ +} + +void test_null_arg (char *d, const char *s) +{ + char *p = null (); + + T (sprintf (d, "%-s", null ())); /* { dg-warning "directive argument is null" } */ + T (sprintf (d, "%-s", p)); /* { dg-warning "directive argument is null" } */ + T (sprintf (d, "%s %s", p, s)); /* { dg-warning "directive argument is null" } */ + T (sprintf (d, "%s %s", s, p)); /* { dg-warning "directive argument is null" } */ + T (sprintf (d, "%s %i", p, 1)); /* { dg-warning "directive argument is null" } */ + T (sprintf (d, "%i %s", 1, p)); /* { dg-warning "directive argument is null" } */ + T (sprintf (d, "%.0s", p)); /* { dg-warning "directive argument is null" } */ + T (sprintf (d, "%1.0s", p)); /* { dg-warning "directive argument is null" } */ + + T (snprintf (d, 0, "%-s", null ())); /* { dg-warning "directive argument is null" } */ + T (snprintf (d, 1, "%-s", p)); /* { dg-warning "directive argument is null" } */ + + T (sprintf (d, "%i %s", 1, null ())); /* { dg-warning "directive argument is null" } */ + T (sprintf (d, "%i %s", 2, p)); /* { dg-warning "directive argument is null" } */ + + T (snprintf (d, 0, "%i %s", 1, null ())); /* { dg-warning "directive argument is null" } */ + T (snprintf (d, 9, "%i %s", 2, p)); /* { dg-warning "directive argument is null" } */ + + /* A sanity check that the %p directive doesn't emit a warning + with a null pointer. */ + T (sprintf (d, "%p", null ())); + T (sprintf (d, "%p", p)); +}