From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Mark H Weaver Newsgroups: gmane.lisp.guile.devel Subject: Re: Undefined behavior in conv-integer.i.c? Date: Wed, 24 Feb 2016 03:11:56 -0500 Message-ID: <874mcysf1v.fsf@netris.org> References: <20160217161639.GE6131@localhost> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Trace: ger.gmane.org 1456301546 23394 80.91.229.3 (24 Feb 2016 08:12:26 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Wed, 24 Feb 2016 08:12:26 +0000 (UTC) Cc: guile-devel@gnu.org To: Miroslav Lichvar Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Wed Feb 24 09:12:22 2016 Return-path: Envelope-to: guile-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1aYUYV-0003Eq-Qi for guile-devel@m.gmane.org; Wed, 24 Feb 2016 09:12:20 +0100 Original-Received: from localhost ([::1]:34221 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aYUYU-0005Bq-MT for guile-devel@m.gmane.org; Wed, 24 Feb 2016 03:12:18 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:44764) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aYUYN-0005BT-KA for guile-devel@gnu.org; Wed, 24 Feb 2016 03:12:12 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aYUYJ-0003Kk-Io for guile-devel@gnu.org; Wed, 24 Feb 2016 03:12:11 -0500 Original-Received: from world.peace.net ([50.252.239.5]:40972) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aYUYJ-0003Ka-EE for guile-devel@gnu.org; Wed, 24 Feb 2016 03:12:07 -0500 Original-Received: from [10.1.10.78] (helo=jojen) by world.peace.net with esmtpsa (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.72) (envelope-from ) id 1aYUYA-0003HC-VR; Wed, 24 Feb 2016 03:11:59 -0500 In-Reply-To: <20160217161639.GE6131@localhost> (Miroslav Lichvar's message of "Wed, 17 Feb 2016 17:16:39 +0100") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.0.91 (gnu/linux) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6.x X-Received-From: 50.252.239.5 X-BeenThere: guile-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Developers list for Guile, the GNU extensibility library" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Original-Sender: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.lisp.guile.devel:18195 Archived-At: --=-=-= Content-Type: text/plain Hi, Miroslav Lichvar writes: > I was looking at a problem with guile-1.8.8 when compiled with > gcc-6.0. Two of the tests from the test suite were failing with > strange "out of range" errors [1]. After some investigation I think > the bug is that the code in libguile/conv-integer.i.c relies on > overflow of signed integers in the following code (starting on line > 77), specifically -TYPE_MIN being less than zero. Adding -fwrapv to > CFLAGS worked as a workaround for me. > > if (mpz_sgn (SCM_I_BIG_MPZ (val)) >= 0) > { > if (n < 0) > goto out_of_range; > } > else > { > n = -n; > if (n >= 0) > goto out_of_range; > } Thanks for bringing this to our attention. I've attached a preliminary patch to address these issues on the 'stable-2.0' branch. > Looking at the current guile code, conv-integer.i.c is identical to > what it was in 1.8.8, but interestingly the tests didn't fail for me. > Maybe something else is preventing gcc from using the optimization? The build system of recent Guile 2.0.x automatically adds -fwrapv to CFLAGS where supported. However, I hope to remove -fwrapv in the future, when we gain confidence that no code in Guile depends on it. > I'm not sure what would be the best way to fix it. Maybe n should > really be unsigned and compared to the maximum values, but what would > be the absolute value of TYPE_MIN if it should work also with other > integer representations than two's complement? My approach was to compare (abs_n - 1) to -(TYPE_MIN + 1) in the case where n is negative. Thanks, Mark --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=0001-Avoid-signed-integer-overflows-in-numeric-conversion.patch Content-Description: [PATCH] Avoid signed integer overflow in numeric conversions >From be75e1713caa7b66aedc837a226091f78dfcb8fe Mon Sep 17 00:00:00 2001 From: Mark H Weaver Date: Wed, 24 Feb 2016 02:17:43 -0500 Subject: [PATCH] Avoid signed integer overflows in numeric conversions. * libguile/conv-integer.i.c: Avoid signed overflow. * libguile/numbers.c (scm_is_signed_integer): Avoid signed overflow. --- libguile/conv-integer.i.c | 15 ++++++++++----- libguile/numbers.c | 15 ++++++++++----- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/libguile/conv-integer.i.c b/libguile/conv-integer.i.c index 4cf887c..fe2d363 100644 --- a/libguile/conv-integer.i.c +++ b/libguile/conv-integer.i.c @@ -64,25 +64,30 @@ SCM_TO_TYPE_PROTO (SCM val) } else { - scm_t_intmax n; + scm_t_uintmax abs_n; + TYPE n; size_t count; if (mpz_sizeinbase (SCM_I_BIG_MPZ (val), 2) > CHAR_BIT*sizeof (scm_t_uintmax)) goto out_of_range; - mpz_export (&n, &count, 1, sizeof (scm_t_uintmax), 0, 0, + mpz_export (&abs_n, &count, 1, sizeof (scm_t_uintmax), 0, 0, SCM_I_BIG_MPZ (val)); if (mpz_sgn (SCM_I_BIG_MPZ (val)) >= 0) { - if (n < 0) + if (abs_n <= TYPE_MAX) + n = abs_n; + else goto out_of_range; } else { - n = -n; - if (n >= 0) + /* Carefully avoid signed integer overflow. */ + if (TYPE_MIN < 0 && abs_n - 1 <= -(TYPE_MIN + 1)) + n = -1 - (TYPE)(abs_n - 1); + else goto out_of_range; } diff --git a/libguile/numbers.c b/libguile/numbers.c index 1e3fc30..5aaac64 100644 --- a/libguile/numbers.c +++ b/libguile/numbers.c @@ -1,4 +1,4 @@ -/* Copyright (C) 1995-2015 Free Software Foundation, Inc. +/* Copyright (C) 1995-2016 Free Software Foundation, Inc. * * Portions Copyright 1990, 1991, 1992, 1993 by AT&T Bell Laboratories * and Bellcore. See scm_divide. @@ -9622,6 +9622,7 @@ scm_is_signed_integer (SCM val, scm_t_intmax min, scm_t_intmax max) } else { + scm_t_uintmax abs_n; scm_t_intmax n; size_t count; @@ -9629,18 +9630,22 @@ scm_is_signed_integer (SCM val, scm_t_intmax min, scm_t_intmax max) > CHAR_BIT*sizeof (scm_t_uintmax)) return 0; - mpz_export (&n, &count, 1, sizeof (scm_t_uintmax), 0, 0, + mpz_export (&abs_n, &count, 1, sizeof (scm_t_uintmax), 0, 0, SCM_I_BIG_MPZ (val)); if (mpz_sgn (SCM_I_BIG_MPZ (val)) >= 0) { - if (n < 0) + if (abs_n <= max) + n = abs_n; + else return 0; } else { - n = -n; - if (n >= 0) + /* Carefully avoid signed integer overflow. */ + if (min < 0 && abs_n - 1 <= -(min + 1)) + n = -1 - (scm_t_intmax)(abs_n - 1); + else return 0; } -- 2.6.3 --=-=-=--