From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Andy Wingo Newsgroups: gmane.lisp.guile.devel Subject: Re: Coverity scan of Fedora Guile-2.0.2 package Date: Fri, 29 Jul 2011 09:32:01 +0200 Message-ID: <87y5zhtudq.fsf@pobox.com> References: <4E312E2A.50504@redhat.com> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: dough.gmane.org 1311924737 21042 80.91.229.12 (29 Jul 2011 07:32:17 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Fri, 29 Jul 2011 07:32:17 +0000 (UTC) Cc: guile-devel@gnu.org To: Michal Luscon Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Fri Jul 29 09:32:13 2011 Return-path: Envelope-to: guile-devel@m.gmane.org Original-Received: from lists.gnu.org ([140.186.70.17]) by lo.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1QmhYL-00057Z-02 for guile-devel@m.gmane.org; Fri, 29 Jul 2011 09:32:13 +0200 Original-Received: from localhost ([::1]:48042 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QmhYK-0002sJ-Ge for guile-devel@m.gmane.org; Fri, 29 Jul 2011 03:32:12 -0400 Original-Received: from eggs.gnu.org ([140.186.70.92]:53934) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QmhYH-0002sA-5n for guile-devel@gnu.org; Fri, 29 Jul 2011 03:32:10 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QmhYF-0001aK-Gi for guile-devel@gnu.org; Fri, 29 Jul 2011 03:32:08 -0400 Original-Received: from a-pb-sasl-sd.pobox.com ([74.115.168.62]:61278 helo=sasl.smtp.pobox.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QmhYF-0001YV-8T for guile-devel@gnu.org; Fri, 29 Jul 2011 03:32:07 -0400 Original-Received: from sasl.smtp.pobox.com (unknown [127.0.0.1]) by a-pb-sasl-sd.pobox.com (Postfix) with ESMTP id 6CBF6539D; Fri, 29 Jul 2011 03:32:06 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=pobox.com; h=from:to:cc :subject:references:date:in-reply-to:message-id:mime-version :content-type; s=sasl; bh=qgp21h0WnzCD9z2CbdEEZkuVg8A=; b=ZgGsCn GPvgywXOvWSP6gm/o92SKBgtQEYNXtZN48mSx8bu9Y8C3UgbAJkpXzB8mryX5XP2 vfwooiJNPSpfUB4J9CdrI2gvNfMmOy8PQGjCH9wxXB6cXo7n4iuKghBtHvlk4aOr X4OfPsE49o3BRcGhNuUlD6j7Nv6RfzwrZGBqs= DomainKey-Signature: a=rsa-sha1; c=nofws; d=pobox.com; h=from:to:cc :subject:references:date:in-reply-to:message-id:mime-version :content-type; q=dns; s=sasl; b=wZm6igoWXVbFEGrlSf7cVM91UzxYK57T Y/euoye6YUDL1o7VUosGMfdjBCu4xRUvM4irMYmcD+Uld36vH6+Blk7H9KwS+ve4 Y+1c6k8hYa5m6fpwv6A2BtmNqEmGisYEMfy7ubE+YNaSAV5o8+qvqJ+oxMt6yJ1O RLhHicOYBnM= Original-Received: from a-pb-sasl-sd.pobox.com (unknown [127.0.0.1]) by a-pb-sasl-sd.pobox.com (Postfix) with ESMTP id 6438D539C; Fri, 29 Jul 2011 03:32:06 -0400 (EDT) Original-Received: from badger (unknown [90.164.198.39]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by a-pb-sasl-sd.pobox.com (Postfix) with ESMTPSA id A68C3539B; Fri, 29 Jul 2011 03:32:05 -0400 (EDT) In-Reply-To: <4E312E2A.50504@redhat.com> (Michal Luscon's message of "Thu, 28 Jul 2011 11:38:50 +0200") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) X-Pobox-Relay-ID: DC0DA71E-B9B4-11E0-8819-B797DE995924-02397024!a-pb-sasl-sd.pobox.com X-detected-operating-system: by eggs.gnu.org: Solaris 10 (beta) X-Received-From: 74.115.168.62 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:12690 Archived-At: Hi Michal, Thank you for sending these results. I have used coverity in the past and know that it can find lots of interesting bugs. However, a few of Guile's constructs caused a fair number of false positives in this batch. So in the beginning there were not many actual positives. However towards the end of the list, we started finding real issues. The only one with a significant security impact was the buffer overflow when reading array tags. I fixed it properly in 2.0 and will back-port a more minimal fix to 1.8. The list of unique issues and their resolutions is below. Given the number of false positives though, I will decline your offer for the full report. Anyone other Guile developer is welcome to it, of course. Regards, Andy On Thu 28 Jul 2011 11:38, Michal Luscon writes: > Error: CHECKED_RETURN: > /builddir/build/BUILD/guile-2.0.2/libguile/r6rs-ports.c:1151: check_return: Calling function "scm_fill_input" without checking return value (as is done elsewhere 5 out of 6 times). This one is fine, the `count' check is equivalent and comes up later. > Error: CONSTANT_EXPRESSION_RESULT: > /builddir/build/BUILD/guile-2.0.2/libguile/conv-uinteger.i.c:103: result_independent_of_operands: val <= 18446744073709551615UL /* 9223372036854775807UL * 2UL + 1UL */ is always true regardless of the values of its operands. This occurs as the logical operand of if. This one and others are also fine -- they are multiply-included files which are parameterized on different type ranges, and we rely on the compiler to eliminate dead branches. > Error: CONSTANT_EXPRESSION_RESULT: > /builddir/build/BUILD/guile-2.0.2/libguile/numbers.c:4570: result_independent_of_operands: ((m_tmp->_mp_size < 0) ? -1 : (m_tmp->_mp_size > 0)) < 0 is always false regardless of the values of its operands. This occurs as the logical first operand of '&&'. This one and similar ones perplex me. They come from the expansion of mpz_sgn, which can (and do) indeed return -1, 0, or 1. I think it is a bug in coverity. > > Error: DEADCODE: > /builddir/build/BUILD/guile-2.0.2/libguile/gc.c:788: dead_error_condition: On this path, the switch value "tag" cannot be "1047UL". Fixed, I think. > /builddir/build/BUILD/guile-2.0.2/libguile/gc.c:748: const: After this line, the value of "tag" is equal to 23. > /builddir/build/BUILD/guile-2.0.2/libguile/gc.c:776: equality_cond: Jumping to case "23UL". > /builddir/build/BUILD/guile-2.0.2/libguile/gc.c:742: new_values: Noticing condition "tag >= 255UL". > /builddir/build/BUILD/guile-2.0.2/libguile/gc.c:788: dead_error_begin: Execution cannot reach this statement "case 1047UL:". I am unclear about these, but this function is internal and never called right now, and will change in 2.2, so I am going to punt. > Error: DEADCODE: > /builddir/build/BUILD/guile-2.0.2/libguile/numbers.c:1503: > dead_error_condition: On this path, the condition "yy == 0L" cannot be > true. Fixed. > Error: FORWARD_NULL: > /builddir/build/BUILD/guile-2.0.2/libguile/strings.c:1924: var_compare_op: Comparing "buf" to null implies that "buf" might be null. > /builddir/build/BUILD/guile-2.0.2/libguile/strings.c:1943: var_deref_model: Passing null variable "buf" to function "unistring_escapes_to_guile_escapes", which dereferences it. This is fine, as scm_encoding_error does a nonlocal exit. > Error: FORWARD_NULL: > /builddir/build/BUILD/guile-2.0.2/libguile/srfi-14.c:454: assign_zero: Assigning: "c->ranges" = 0. > /builddir/build/BUILD/guile-2.0.2/libguile/srfi-14.c:462: var_deref_model: Passing null variable "c->ranges" to function "scm_i_charset_set", which dereferences it. > /builddir/build/BUILD/guile-2.0.2/libguile/srfi-14.c:91: deref_parm: Directly dereferencing parameter "cs->ranges". When ranges is NULL, len is 0. No error. > Error: NEGATIVE_RETURNS: > /builddir/build/BUILD/guile-2.0.2/libguile/programs.c:58: negative_return_fn: Function "scm_c_vector_length(free_variables)" returns a negative number. > /builddir/build/BUILD/guile-2.0.2/libguile/vectors.c:135: negative_return: Calling "scm_to_uint64", which might return a negative value. > /builddir/build/BUILD/guile-2.0.2/libguile/conv-uinteger.i.c:32: var_tested_nsceg: Variable "(scm_t_uintmax)n" is negative. > /builddir/build/BUILD/guile-2.0.2/libguile/conv-uinteger.i.c:34: return_negative_variable: Explicitly returning negative variable "n". > /builddir/build/BUILD/guile-2.0.2/libguile/vectors.c:135: return_negative_fn: Returning the return value of "scm_to_uint64", which might be negative. > /builddir/build/BUILD/guile-2.0.2/libguile/programs.c:58: var_assign: Assigning: unsigned variable "len" = "scm_c_vector_length". > /builddir/build/BUILD/guile-2.0.2/libguile/programs.c:64: negative_returns: Using unsigned variable "len" in a loop exit condition. This (and other similar cases) is a bug in coverity, presumably: there is no way for scm_to_uint64 to return a negative number. Perhaps it doesn't understand the multiple-include thing correctly. > Error: NEGATIVE_RETURNS: > /builddir/build/BUILD/guile-2.0.2/libguile/control.c:253: negative_return_fn: Function "scm_ilength(args)" returns a negative number. > /builddir/build/BUILD/guile-2.0.2/libguile/list.c:190: return_negative_constant: Explicitly returning negative value "-1L". > /builddir/build/BUILD/guile-2.0.2/libguile/control.c:253: var_assign: Assigning: unsigned variable "n" = "scm_ilength". > /builddir/build/BUILD/guile-2.0.2/libguile/control.c:255: negative_returns: Using unsigned variable "n" in a loop exit condition. Good one! Fixed. > Error: NEGATIVE_RETURNS: > /builddir/build/BUILD/guile-2.0.2/libguile/eval.c:285: negative_return_fn: Function "scm_ilength((SCM *)(scm_t_cell *)(scm_t_bits)(0 ? *NULL = mx : mx)[0])" returns a negative number. False positive. The return value will be positive, as memoize.c constructed the mx properly. > Error: NEGATIVE_RETURNS: > /builddir/build/BUILD/guile-2.0.2/libguile/posix.c:1298: negative_returns: Passing negative constant "-1" to a parameter that cannot be negative. > /builddir/build/BUILD/guile-2.0.2/libguile/strings.c:2061: parm_assign_alias: Assigning: "i" = "argc". > /builddir/build/BUILD/guile-2.0.2/libguile/strings.c:2066: index: Indexing with parameter copy "i". False positive. See scm_makfromstrs for details. > Error: NO_EFFECT: > /builddir/build/BUILD/guile-2.0.2/lib/strftime.c:616: bad_memset: > Memset with fill value '0'. Did you want 0? "memset(p, 48, _delta)". False positive; I think that's what this code wants to do. > Error: NO_EFFECT: > /builddir/build/BUILD/guile-2.0.2/libguile/gen-scmconfig.c:218: array_null: Comparing an array to null is not useful: ""inline"". False positive; this is part of build configuration. > Error: OVERRUN_STATIC: > /builddir/build/BUILD/guile-2.0.2/libguile/arrays.c:912: overrun-local: Overrunning static array "tag", with 80 elements, at position 80 with index variable "tag_len". Nasty, a read-time buffer overflow. Fixed. > Error: OVERRUN_STATIC: > /builddir/build/BUILD/guile-2.0.2/libguile/hashtab.c:279: overrun-local: Overrunning static array "hashtable_size", with 25 elements, at position 25 with index variable "i". Fixed. > TError: RESOURCE_LEAK: > /builddir/build/BUILD/guile-2.0.2/libguile/script.c:320: leaked_storage: Variable "nargv" going out of scope leaks the storage it points to. Fixed, though it is a small thing. > Error: RESOURCE_LEAK: > /builddir/build/BUILD/guile-2.0.2/libguile/script.c:327: alloc_fn: Calling allocation function "script_read_arg". > /builddir/build/BUILD/guile-2.0.2/libguile/script.c:330: leaked_storage: Variable "narg" going out of scope leaks the storage it points to. I looked at it a little bit and couldn't find a nice way to fix this leak. Because it is small and only at startup, I am going to punt. > Error: RESOURCE_LEAK: > /builddir/build/BUILD/guile-2.0.2/libguile/posix.c:1342: alloc_fn: Calling allocation function "tmpfile". > /builddir/build/BUILD/guile-2.0.2/libguile/posix.c:1344: leaked_storage: Variable "rv" going out of scope leaks the storage it points to. Good one. I filed a bug. > Error: SIZEOF_MISMATCH: > /builddir/build/BUILD/guile-2.0.2/libguile/bytevectors.c:256: suspicious_sizeof: Passing argument "24UL /* 3UL * sizeof (SCM) /*8*/ */" to function "scm_gc_malloc" and then casting the return value to "SCM" is suspicious. Did you intend to use "sizeof(struct scm_unused_struct)" instead of "sizeof (SCM)" ? This was fine, but unclear; I changed it. > Error: UNUSED_VALUE: > /builddir/build/BUILD/guile-2.0.2/libguile/read.c:411: returned_pointer: Pointer "tmp" returned by "scm_read_expression(port)" is never used. Harmless, but fixed. > Error: UNUSED_VALUE: > /builddir/build/BUILD/guile-2.0.2/libguile/stacks.c:189: returned_pointer: Pointer "frame" returned by "scm_stack_ref(stack, scm_from_int64(len - 1UL))" is never used. Harmless, but fixed.