From patchwork Fri Sep 13 18:01:36 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marek Polacek X-Patchwork-Id: 274833 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 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id F09A22C00AA for ; Sat, 14 Sep 2013 04:01:55 +1000 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:mime-version:content-type; q=dns; s=default; b=cMwrOgXeMaCj3WCjTxKaY61dJlDzYBuu/riBBpTTfGZjZGA125 ACvud3yNeD+tFmjSqcGPy8bNMN4ED7yOdMHDH9mBF3RXBk+jceBUg9gJ2jUc7VwK PJpYyHIGlQP6tnNp3lwpMp3Kl9UjimmzP76JCbCP5VisCLetTdkUbDln4= 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:date :from:to:cc:subject:message-id:mime-version:content-type; s= default; bh=MPFTHm1n/ov8lfmXFTwtcFIlBf8=; b=svPJ56PlAaAURAINgjAQ iedGDqL2kvnALHx41MlpwlPHXIwVj0ZdNwbgLw66eLD3RfhCzn24OCsIANyczn7F IcPna3xCTvNckoixl+0RNdMQQ9FymfPaagYLPCFTV1JJj3LiUTRqDxsuGGg1IPnd c17sQDp6iYdJt5Od29YBuUw= Received: (qmail 25811 invoked by alias); 13 Sep 2013 18:01:47 -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 25799 invoked by uid 89); 13 Sep 2013 18:01:46 -0000 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, 13 Sep 2013 18:01:46 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r8DI1eGE026801 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 13 Sep 2013 14:01:41 -0400 Received: from redhat.com (ovpn-116-91.ams2.redhat.com [10.36.116.91]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r8DI1aPj011416 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Fri, 13 Sep 2013 14:01:38 -0400 Date: Fri, 13 Sep 2013 20:01:36 +0200 From: Marek Polacek To: GCC Patches Cc: Jakub Jelinek , "Joseph S. Myers" Subject: [PATCH] Don't always instrument shifts (PR sanitizer/58413) Message-ID: <20130913180135.GU23899@redhat.com> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.5.20 (2009-06-14) This is kind of fugly, but don't have anything better at the moment. The thing is that we were always instrumenting shift expressions, even when both operands were INTEGER_CSTs and we could prove at compile time that the expression is well defined. This causes problems in the C FE, mainly at places where the integer constant expression are expected; one glaring example is e.g. the case label value. So with this patch we don't instrument expression that are safe. If we cannot prove that the expression does not cause undefined behavior, we generate COMPOUND_EXPR as before, the FE then errors on it -- but in this case doing that seems fine. I'm only concerned about the error message, perhaps I could do something similar as in the C++ FE, i.e. write a function that gets a COMPOUND_EXPR and says whether this is an ubsan COMPOUND_EXPR and if it is, we'd instead "case label does not reduce to an integer constant" issued something along the lines of "undefined behavior occured". Ran ubsan testsuite/bootstrap-ubsan on x86_64-linux, ok for trunk? 2013-09-13 Marek Polacek PR sanitizer/58413 c-family/ * c-ubsan.c (ubsan_instrument_shift): Don't instrument an expression if we can prove it is correct. testsuite/ * c-c++-common/ubsan/shift-4.c: New test. * c-c++-common/ubsan/shift-5.c: New test. * gcc.dg/ubsan/c-shift-1.c: New test. Marek --- gcc/c-family/c-ubsan.c.mp3 2013-09-13 19:19:55.410925155 +0200 +++ gcc/c-family/c-ubsan.c 2013-09-13 19:20:16.766006242 +0200 @@ -104,6 +104,40 @@ ubsan_instrument_shift (location_t loc, tree uprecm1 = build_int_cst (op1_utype, op0_prec - 1); tree precm1 = build_int_cst (type1, op0_prec - 1); + /* If OP0 is of an unsigned type, we may prove that OP1 is not + greater than UPRECM1 (and not negative); in that case we can + bail out early. */ + if (TYPE_UNSIGNED (type0) + && TREE_CODE (op1) == INTEGER_CST + && tree_int_cst_sgn (op1) != -1 + && !tree_int_cst_lt (uprecm1, op1)) + return NULL_TREE; + + /* Even when OP0 is of a signed type, we may prove that there's + nothing wrong with the shift if both operands are INTEGER_CSTs + and wouldn't trigger UB. We do this only for C. + XXX Should we treat ANSI C specially wrt 1 << 31? */ + if (TREE_CODE (op0) == INTEGER_CST + && TREE_CODE (op1) == INTEGER_CST + && !TYPE_UNSIGNED (type0) + && tree_int_cst_sgn (op1) != -1 + && !tree_int_cst_lt (uprecm1, op1) + && !c_dialect_cxx ()) + { + /* If this is a right shift, we can return now. */ + if (code == RSHIFT_EXPR) + return NULL_TREE; + + /* Otherwise see comment below. */ + tree x = fold_convert_loc (loc, unsigned_type_for (type0), op0); + x = fold_build2 (RSHIFT_EXPR, TREE_TYPE (x), x, + fold_build2 (MINUS_EXPR, TREE_TYPE (precm1), precm1, + op1)); + if (integer_zerop (x)) + return NULL_TREE; + } + + /* Ok, we have to do the instrumentation. */ t = fold_convert_loc (loc, op1_utype, op1); t = fold_build2 (GT_EXPR, boolean_type_node, t, uprecm1); @@ -125,7 +159,7 @@ ubsan_instrument_shift (location_t loc, x < 0 || ((unsigned) x >> (precm1 - y)) if > 1, is undefined. */ if (code == LSHIFT_EXPR - && !TYPE_UNSIGNED (TREE_TYPE (op0)) + && !TYPE_UNSIGNED (type0) && (cxx_dialect == cxx11 || cxx_dialect == cxx1y)) { tree x = fold_build2 (MINUS_EXPR, integer_type_node, precm1, op1); --- gcc/testsuite/c-c++-common/ubsan/shift-4.c.mp3 2013-09-13 18:25:02.294833062 +0200 +++ gcc/testsuite/c-c++-common/ubsan/shift-4.c 2013-09-13 18:43:56.171046915 +0200 @@ -0,0 +1,30 @@ +/* PR sanitizer/58413 */ +/* { dg-do run } */ +/* { dg-options "-fsanitize=shift -w" } */ + +int x = 7; +int +main (void) +{ + /* All of the following should pass. */ + int A[128 >> 5] = {}; + int B[128 << 5] = {}; + + static int e = + ((int) + (0x00000000 | ((31 & ((1 << (4)) - 1)) << (((15) + 6) + 4)) | + ((0) << ((15) + 6)) | ((0) << (15)))); + + if (e != 503316480) + __builtin_abort (); + + switch (x) + { + case 1 >> 4: + case 1 << 4: + case 128 << (4 + 1): + case 128 >> (4 + 1): + return 1; + } + return 0; +} --- gcc/testsuite/c-c++-common/ubsan/shift-5.c.mp3 2013-09-13 18:25:06.195847144 +0200 +++ gcc/testsuite/c-c++-common/ubsan/shift-5.c 2013-09-13 19:16:38.990211229 +0200 @@ -0,0 +1,21 @@ +/* { dg-do compile} */ +/* { dg-options "-fsanitize=shift -w" } */ +/* { dg-shouldfail "ubsan" } */ + +int x; +int +main (void) +{ + /* None of the following should pass. */ + switch (x) + { + case 1 >> -1: /* { dg-error "" } */ + case -1 >> -1: /* { dg-error "" } */ + case 1 << -1: /* { dg-error "" } */ + case -1 << -1: /* { dg-error "" } */ + case -1 >> 200: /* { dg-error "" } */ + case 1 << 200: /* { dg-error "" } */ + return 1; + } + return 0; +} --- gcc/testsuite/gcc.dg/ubsan/c-shift-1.c.mp3 2013-09-13 19:01:21.334825808 +0200 +++ gcc/testsuite/gcc.dg/ubsan/c-shift-1.c 2013-09-13 19:01:21.334825808 +0200 @@ -0,0 +1,18 @@ +/* { dg-do compile} */ +/* { dg-options "-fsanitize=shift -w" } */ +/* { dg-shouldfail "ubsan" } */ + +int x; +int +main (void) +{ + /* None of the following should pass. */ + int A[1 >> -1] = {}; /* { dg-error "variable-sized object may not be initialized" } */ + int B[-1 >> -1] = {}; /* { dg-error "variable-sized object may not be initialized" } */ + int D[1 << -1] = {}; /* { dg-error "variable-sized object may not be initialized" } */ + int E[-1 << -1] = {}; /* { dg-error "variable-sized object may not be initialized" } */ + int F[-1 >> 200] = {}; /* { dg-error "variable-sized object may not be initialized" } */ + int G[1 << 200] = {}; /* { dg-error "variable-sized object may not be initialized" } */ + + return 0; +}