From patchwork Sun Dec 4 23:55:04 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Sebor X-Patchwork-Id: 86469 Delivered-To: patch@linaro.org Received: by 10.140.20.101 with SMTP id 92csp1237798qgi; Sun, 4 Dec 2016 15:55:24 -0800 (PST) X-Received: by 10.84.209.227 with SMTP id y90mr119615880plh.111.1480895724871; Sun, 04 Dec 2016 15:55:24 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id f8si12558294plm.176.2016.12.04.15.55.24 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 04 Dec 2016 15:55:24 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-443441-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-443441-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-443441-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:from :subject:to:message-id:date:mime-version:content-type; q=dns; s= default; b=SwqP+PUcvwQ0xm2V/tQBmxbS5ohxHxLxxMJRA/0R1y3voQW7xMSCv bQaeVDXdvsOHuZ32x45B9MGX59n/TXfAWZAJxXGLPmpZtRGu5M8j2vq53vwWknzO +yOeLZscdGNZtRirigHh15lVJLyrKtCLz9UNInoV8fpkifMjFSMXs4= 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:from :subject:to:message-id:date:mime-version:content-type; s= default; bh=Kf0Oi07Op6VJigqpspoE1nnIpcY=; b=OKA8omDtXv0p9cllA0wg Mtb/B0XzkUTmQoH/4uzsHUZ0PSbQ23fOpDhJu3Kh22jb6oLlVnaci5BVe61I4RNA P86Bp0XjTl1PYwqhip4g9tambaHoektZ/A5gdqEi+Df2n4uFlv4UZIUISb3dVJNn 1FqUyl7VjFKSf8I9JIiOlcc= Received: (qmail 4379 invoked by alias); 4 Dec 2016 23:55:11 -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 4365 invoked by uid 89); 4 Dec 2016 23:55:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=no version=3.3.2 spammy=warned, 4337, UD:format, 1, 94 X-HELO: mail-qt0-f177.google.com Received: from mail-qt0-f177.google.com (HELO mail-qt0-f177.google.com) (209.85.216.177) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 04 Dec 2016 23:55:08 +0000 Received: by mail-qt0-f177.google.com with SMTP id w33so299385109qtc.3 for ; Sun, 04 Dec 2016 15:55:08 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:subject:to:message-id:date:user-agent :mime-version; bh=MQTkyRESL4jHXBu7UuowGoYOZCjezuSX5y26tiG8ekE=; b=DGJeuBbPerj+l4naHsUxZzMXXggOzWDoQBXrdgxjgqakY5rZe14DymkipK15deFoFT jJtGJjW8kQfAEPKusJKPOgVcsP9nqH51yfoox6liHRCs/ahkZgMdncYzbSt/s+VAcicV akWNkorKOpNN6GjfIE+YUNUsUr4faAsiDskW0CZPKphA5hkbFz98t9xU/uFszPZWTPm3 8t333aImDo98doAw6FGzliRtHO7bFubBJXuOqx1FhRqMcQz/7/g6FKmihaVEmm578kmV hk5otMk6vlqJD8JiixFxG/KyTp9+sA6Bj1n+bLG5CP5Ul4n81R18amV+Q4gpH4Q6nkGy gxnw== X-Gm-Message-State: AKaTC03l2ECsQOMB/ayNiMzsfTuCSSALHZ29DgPsfPK5Ql1r9BE/DQR40FR80bBbfmiuiQ== X-Received: by 10.200.56.253 with SMTP id g58mr48535244qtc.201.1480895706687; Sun, 04 Dec 2016 15:55:06 -0800 (PST) Received: from [192.168.0.26] (97-122-174-22.hlrn.qwest.net. [97.122.174.22]) by smtp.gmail.com with ESMTPSA id 21sm8150886qkf.17.2016.12.04.15.55.05 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 04 Dec 2016 15:55:06 -0800 (PST) From: Martin Sebor Subject: [PATCH] detect null sprintf pointers (PR 78519) To: Gcc Patch List Message-ID: <8279f735-1d61-8eec-ba9b-c477481c2a54@gmail.com> Date: Sun, 4 Dec 2016 16:55:04 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 X-IsSubscribed: yes Bug 78519 points out that while the -Wformat warning flags a small subset of sprintf calls with a null pointer argument to a %s directive (those where the pointer is a constant) it misses the much bigger set where the pointer is not a constant but instead is determined to be null as a result of optimization. This is because -Wformat runs too early, before any of the optimization passes that make it possible to detect that non-constant pointers are null. With the -Wformat-length warning running much later than -Wformat, it's trivial to detect and diagnose these types of bugs with it. The attached patch adds this warning, along with the ability to detect a null destination pointer when it's required to be non-null (this is in all of the {v,}sprintf functions and in {v,}snprintf when the size argument is not zero). Ultimately, the destination pointer argument (but not the format string) to the {v,}sprintf functions needs to be declared nonnull (pursuant to bug 78673) and the null-checking moved elsewhere. I'm testing a follow-on patch that does just that but I post this fix in the meantime since its main focus is the null %s argument. 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 e86c4dc..7004f09 100644 --- a/gcc/gimple-ssa-sprintf.c +++ b/gcc/gimple-ssa-sprintf.c @@ -433,7 +433,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; } @@ -461,6 +461,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. */ @@ -1624,6 +1627,20 @@ format_string (const conversion_spec &spec, tree arg) res.range.min = 0; } } + else if (arg && integer_zerop (arg)) + { + /* Handle null pointer argument. */ + + fmtresult res; + /* Set the range based on Glibc "(null)" output but leave + all other members at default to indicate that the range + isn't trustworthy. This allows the rest of the format + string to be checked for problems. */ + res.range.min = 0; + res.range.max = 6; + res.nullp = true; + return res; + } else { /* For a '%s' and '%ls' directive with a non-constant string, @@ -1765,6 +1782,18 @@ format_directive (const pass_sprintf_length::call_info &info, } } + bool warned = false; + + if (fmtres.nullp) + { + warned = fmtwarn (dirloc, pargrange, NULL, + OPT_Wformat_length_, + "%<%.*s%> directive argument is null", + (int)cvtlen, cvtbeg); + + /* Proceed below. */ + } + /* 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. */ @@ -1775,8 +1804,6 @@ format_directive (const pass_sprintf_length::call_info &info, /* The result is a range (i.e., it's inexact). */ if (!res->warned) { - bool warned = false; - if (navail < fmtres.range.min) { /* The minimum directive output is longer than there is @@ -2726,6 +2753,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) @@ -2733,7 +2763,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)) { @@ -2795,6 +2825,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; @@ -2813,7 +2857,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-7.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-7.c new file mode 100644 index 0000000..ee4b04e --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-7.c @@ -0,0 +1,94 @@ +/* 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 "-O1 -Wformat -Wformat-length -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" } */ + /* { dg-warning "output may be truncated writing between 0 and 6 bytes" "(null)" { target *-*-* } .-1 } */ + + 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)); +}