Re: Coverity scan of Fedora Guile-2.0.2 package

[Andy Wingo]

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 <address@hidden> 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.