[Freeipa-devel] [PATCHES] Coverity fixes
Martin Basti
mbasti at redhat.com
Thu Aug 11 12:51:11 UTC 2016
On 05.08.2016 14:13, Lukas Slebodnik wrote:
> On (05/08/16 12:43), Petr Vobornik wrote:
>> On 07/28/2016 01:01 PM, Martin Basti wrote:
>>>
>>> On 25.07.2016 11:46, Simo Sorce wrote:
>>>> The attached patches fix some minor issues found by coverity, and pull
>>>> in other fixes released by the asn1c project.
>>>>
>>>> Simo.
>>>>
>>>>
>>>>
>>> I cannot build RPMS with this patch, is there any missing build dependency?
>>>
>>> /bin/sh ./libtool --tag=CC --mode=link gcc -Wall -Wshadow
>>> -Wstrict-prototypes -Wpointer-arith -Wcast-align
>>> -Werror-implicit-function-declaration -O2 -g -pipe -Wall
>>> -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
>>> -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches
>>> -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -g -O2 -Wall
>>> -Wextra -Wformat-security -Wno-unused-parameter -Wno-sign-compare
>>> -Wno-missing-field-initializers -Wl,-z,relro
>>> -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -o ipa-getkeytab ipa-getkeytab.o
>>> ipa-client-common.o ipa_krb5.o ../asn1/libipaasn1.la -lkrb5 -lk5crypto -lcom_err
>>> -llber -lldap -lsasl2 -lpopt -lini_config -lbasicobjects -lref_array
>>> -lcollection -lini_config -lini_config
>>> libtool: link: gcc -Wall -Wshadow -Wstrict-prototypes -Wpointer-arith
>>> -Wcast-align -Werror-implicit-function-declaration -O2 -g -pipe -Wall
>>> -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
>>> -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches
>>> -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -g -O2 -Wall
>>> -Wextra -Wformat-security -Wno-unused-parameter -Wno-sign-compare
>>> -Wno-missing-field-initializers -Wl,-z -Wl,relro
>>> -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -o ipa-getkeytab ipa-getkeytab.o
>>> ipa-client-common.o ipa_krb5.o ../asn1/.libs/libipaasn1.a -lkrb5 -lk5crypto
>>> -lcom_err -llber -lldap -lsasl2 -lpopt -lbasicobjects -lref_array -lcollection
>>> -lini_config
>>> ../asn1/.libs/libipaasn1.a(constr_CHOICE.o): In function `CHOICE_decode_uper':
>>> /root/freeipa/rpmbuild/BUILD/freeipa-4.4.0/asn1/asn1c/constr_CHOICE.c:897:
>>> undefined reference to `uper_open_type_get'
>>> ../asn1/.libs/libipaasn1.a(constr_CHOICE.o): In function `CHOICE_encode_uper':
>>> /root/freeipa/rpmbuild/BUILD/freeipa-4.4.0/asn1/asn1c/constr_CHOICE.c:982:
>>> undefined reference to `uper_open_type_put'
>>> ../asn1/.libs/libipaasn1.a(constr_SEQUENCE.o): In function
>>> `SEQUENCE_handle_extensions':
>>> /root/freeipa/rpmbuild/BUILD/freeipa-4.4.0/asn1/asn1c/constr_SEQUENCE.c:1285:
>>> undefined reference to `uper_open_type_put'
>>> ../asn1/.libs/libipaasn1.a(constr_SEQUENCE.o): In function `SEQUENCE_decode_uper':
>>> /root/freeipa/rpmbuild/BUILD/freeipa-4.4.0/asn1/asn1c/constr_SEQUENCE.c:1187:
>>> undefined reference to `uper_open_type_get'
>>> /root/freeipa/rpmbuild/BUILD/freeipa-4.4.0/asn1/asn1c/constr_SEQUENCE.c:1203:
>>> undefined reference to `uper_open_type_skip'
>>> collect2: error: ld returned 1 exit status
>>>
>>> Martin^2
>>>
>> Bumping. Was it temporary issue or issue in the patch?
>>
> I could not see such error.
> However, these patches would be good to test with coverity.
> We need to use fedora rawhide for testing due to BuildRequires
> in freeipa.spec. But C-part of freeIPA cannot be compiled on rawhide
> due to new samba (4.5). Patches are already on the list.
>
> LS
>
Lukas helped me to fix error I reported before (missing entries in
Makefile.am), so I run covscan and I get bunch of new errors.
Question is, do we want push autogenerated code?
Error: FORWARD_NULL (CWE-476): [#def1]
freeipa-4.4.0/asn1/asn1c/INTEGER.c:463: assign_zero: Assigning:
"dec_value_start" = "NULL".
freeipa-4.4.0/asn1/asn1c/INTEGER.c:488: var_deref_model: Passing null
pointer "dec_value_start" to "asn_strtol_lim", which dereferences it.
freeipa-4.4.0/asn1/asn1c/INTEGER.c:975:2: deref_parm: Directly
dereferencing parameter "str".
# 973| if(str >= *end) return ASN_STRTOL_ERROR_INVAL;
# 974|
# 975|-> switch(*str) {
# 976| case '-':
# 977| last_digit_max++;
Error: MISSING_BREAK (CWE-484): [#def2]
freeipa-4.4.0/asn1/asn1c/INTEGER.c:976: unterminated_case: The case for
value "'-'" is not terminated by a 'break' statement.
freeipa-4.4.0/asn1/asn1c/INTEGER.c:979: fallthrough: The above case
falls through to this one.
# 977| last_digit_max++;
# 978| sign = -1;
# 979|-> case '+':
# 980| str++;
# 981| if(str >= *end) {
Error: NEGATIVE_RETURNS (CWE-394): [#def3]
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:381: var_tested_neg: Variable
"tlv_len" tests negative.
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:391: var_assign: Assigning:
signed variable "sel->left" = "tlv_len".
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:247: var_assign: Assigning:
unsigned variable "Left" = "sel->left".
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:275: negative_returns: "Left" is
passed to a parameter that cannot be negative.
freeipa-4.4.0/asn1/asn1c/ber_tlv_tag.c:33:2: parm_loop_bound: Using
unsigned parameter "size" in a loop exit test.
# 273| }
# 274|
# 275|-> tl = ber_fetch_tag(buf_ptr, Left, &tlv_tag);
# 276| ASN_DEBUG("fetch tag(size=%ld,L=%ld), %sstack,
left=%ld, wn=%ld, tl=%ld",
# 277| (long)size, (long)Left, sel?"":"!",
Error: FORWARD_NULL (CWE-476): [#def4]
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:583: var_compare_op: Comparing
"st->buf" to null implies that "st->buf" might be null.
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:591: alias_transfer: Assigning:
"buf" = "st->buf".
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:601: var_deref_op: Dereferencing
null pointer "buf".
# 599| p = scratch;
# 600| }
# 601|-> *p++ = h2c[(*buf >> 4) & 0x0F];
# 602| *p++ = h2c[*buf & 0x0F];
# 603| }
Error: FORWARD_NULL (CWE-476): [#def5]
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:583: var_compare_op: Comparing
"st->buf" to null implies that "st->buf" might be null.
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:591: alias_transfer: Assigning:
"buf" = "st->buf".
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:615: var_deref_op: Dereferencing
null pointer "buf".
# 613| _i_ASN_TEXT_INDENT(1, ilevel);
# 614| }
# 615|-> *p++ = h2c[(*buf >> 4) & 0x0F];
# 616| *p++ = h2c[*buf & 0x0F];
# 617| *p++ = 0x20;
Error: FORWARD_NULL (CWE-476): [#def6]
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:739: var_compare_op: Comparing
"st->buf" to null implies that "st->buf" might be null.
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:742: alias_transfer: Assigning:
"buf" = "st->buf".
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:745: var_deref_op: Dereferencing
null pointer "buf".
# 743| end = buf + st->size;
# 744| for(ss = buf; buf < end; buf++) {
# 745|-> unsigned int ch = *buf;
# 746| int s_len; /* Special encoding sequence length */
# 747|
Error: FORWARD_NULL (CWE-476): [#def7]
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:1659: var_compare_op: Comparing
"st->buf" to null implies that "st->buf" might be null.
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:1665: alias_transfer: Assigning:
"buf" = "st->buf".
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:1674: var_deref_op:
Dereferencing null pointer "buf".
# 1672| p = scratch;
# 1673| }
# 1674|-> *p++ = h2c[(*buf >> 4) & 0x0F];
# 1675| *p++ = h2c[*buf & 0x0F];
# 1676| *p++ = 0x20;
Error: REVERSE_INULL (CWE-476): [#def8]
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:1706: deref_ptr: Directly
dereferencing pointer "td".
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:1713: check_after_deref:
Null-checking "td" suggests that it may be null, but it has already been
dereferenced on all paths leading to the check.
# 1711| struct _stack *stck;
# 1712|
# 1713|-> if(!td || !st)
# 1714| return;
# 1715|
Error: DEADCODE (CWE-561): [#def9]
freeipa-4.4.0/asn1/asn1c/ber_tlv_length.c:40: assignment: Assigning:
"len" = "0L".
freeipa-4.4.0/asn1/asn1c/ber_tlv_length.c:44: cond_at_least: Condition
"len < 0L", taking false branch. Now the value of "len" is at least 0.
freeipa-4.4.0/asn1/asn1c/ber_tlv_length.c:54: assignment: Assigning:
"lenplusepsilon" = "(size_t)len + 1024UL".
freeipa-4.4.0/asn1/asn1c/ber_tlv_length.c:63: at_least: At condition
"lenplusepsilon < 0L", the value of "lenplusepsilon" must be at least 1024.
freeipa-4.4.0/asn1/asn1c/ber_tlv_length.c:63: dead_error_condition: The
condition "lenplusepsilon < 0L" cannot be true.
freeipa-4.4.0/asn1/asn1c/ber_tlv_length.c:65: dead_error_line: Execution
cannot reach this statement: "return -1L;".
# 63| if(lenplusepsilon < 0) {
# 64| /* Too large length value */
# 65|-> return -1;
# 66| }
# 67|
Error: REVERSE_INULL (CWE-476): [#def10]
freeipa-4.4.0/asn1/asn1c/constr_CHOICE.c:1034: deref_ptr: Directly
dereferencing pointer "td".
freeipa-4.4.0/asn1/asn1c/constr_CHOICE.c:1037: check_after_deref:
Null-checking "td" suggests that it may be null, but it has already been
dereferenced on all paths leading to the check.
# 1035| int present;
# 1036|
# 1037|-> if(!td || !ptr)
# 1038| return;
# 1039|
Error: RESOURCE_LEAK (CWE-772): [#def11]
freeipa-4.4.0/asn1/asn1c/constr_SEQUENCE.c:1147: alloc_fn: Storage is
returned from allocation function "malloc".
freeipa-4.4.0/asn1/asn1c/constr_SEQUENCE.c:1147: var_assign: Assigning:
"epres" = storage returned from "malloc(bmlength + 15L >> 3)".
freeipa-4.4.0/asn1/asn1c/constr_SEQUENCE.c:1151: noescape: Resource
"epres" is not freed or pointed-to in "per_get_many_bits".
freeipa-4.4.0/asn1/asn1c/per_support.c:123:48: noescape:
"per_get_many_bits(asn_per_data_t *, uint8_t *, int, int)" does not free
or save its parameter "dst".
freeipa-4.4.0/asn1/asn1c/constr_SEQUENCE.c:1152: leaked_storage:
Variable "epres" going out of scope leaks the storage it points to.
# 1150| /* Get the extensions map */
# 1151| if(per_get_many_bits(pd, epres, 0, bmlength))
# 1152|-> _ASN_DECODE_STARVED;
# 1153|
# 1154| memset(&epmd, 0, sizeof(epmd));
Error: CHECKED_RETURN (CWE-252): [#def12]
freeipa-4.4.0/asn1/asn1c/constr_SEQUENCE.c:1320: check_return: Calling
"per_put_few_bits" without checking return value (as is done elsewhere
23 out of 28 times).
freeipa-4.4.0/asn1/asn1c/INTEGER.c:724: example_checked: Example 1:
"per_put_few_bits(po, inext, 1)" has its value checked in
"per_put_few_bits(po, inext, 1)".
freeipa-4.4.0/asn1/asn1c/NativeEnumerated.c:180: example_checked:
Example 2: "per_put_few_bits(po, inext, 1)" has its value checked in
"per_put_few_bits(po, inext, 1)".
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:1307: example_checked: Example
3: "per_put_few_bits(po, ch, unit_bits)" has its value checked in
"per_put_few_bits(po, ch, unit_bits)".
freeipa-4.4.0/asn1/asn1c/constr_CHOICE.c:949: example_checked: Example
4: "per_put_few_bits(po, 1U, 1)" has its value checked in
"per_put_few_bits(po, 1U, 1)".
freeipa-4.4.0/asn1/asn1c/constr_SEQUENCE.c:1282: example_checked:
Example 5: "per_put_few_bits(po1, present, 1)" has its value checked in
"per_put_few_bits(po1, present, 1)".
# 1318| if(specs->ext_before >= 0) {
# 1319| n_extensions = SEQUENCE_handle_extensions(td, sptr, 0, 0);
# 1320|-> per_put_few_bits(po, n_extensions ? 1 : 0, 1);
# 1321| } else {
# 1322| n_extensions = 0; /* There are no extensions to
encode */
Error: DEADCODE (CWE-561): [#def13]
freeipa-4.4.0/asn1/asn1c/per_encoder.c:126: cond_notnull: Condition
"td", taking true branch. Now the value of "td" is not "NULL".
freeipa-4.4.0/asn1/asn1c/per_encoder.c:146: notnull: At condition "td",
the value of "td" cannot be "NULL".
freeipa-4.4.0/asn1/asn1c/per_encoder.c:146: dead_error_condition: The
condition "td" must be true.
freeipa-4.4.0/asn1/asn1c/per_encoder.c:146: dead_error_line: Execution
cannot reach the expression """" inside this statement:
"ASN_DEBUG("Failed to encode...".
# 144|
# 145| if(_uper_encode_flush_outp(&po))
# 146|-> _ASN_ENCODE_FAILED;
# 147| }
# 148|
Error: DEADCODE (CWE-561): [#def14]
freeipa-4.4.0/asn1/asn1c/per_opentype.c:176: assignment: Assigning:
"padding" = "pd->moved % 8UL".
freeipa-4.4.0/asn1/asn1c/per_opentype.c:179: between: At condition
"padding > 7L", the value of "padding" must be between 1 and 7.
freeipa-4.4.0/asn1/asn1c/per_opentype.c:177: cond_cannot_single:
Condition "padding", taking true branch. Now the value of "padding"
cannot be equal to 0.
freeipa-4.4.0/asn1/asn1c/per_opentype.c:179: cannot_single: At condition
"padding > 7L", the value of "padding" cannot be equal to 0.
freeipa-4.4.0/asn1/asn1c/per_opentype.c:179: dead_error_condition: The
condition "padding > 7L" cannot be true.
freeipa-4.4.0/asn1/asn1c/per_opentype.c:180: dead_error_begin: Execution
cannot reach this statement: "ASN_DEBUG("Too large paddin...".
# 178| int32_t pvalue;
# 179| if(padding > 7) {
# 180|-> ASN_DEBUG("Too large padding %d in open type",
# 181| (int)padding);
# 182| rv.code = RC_FAIL;
Error: OVERRUN (CWE-119): [#def15]
freeipa-4.4.0/asn1/asn1c/per_support.c:14: assignment: Assigning: "n" =
"(n + 1) % 2". The value of "n" is now between 0 and 1 (inclusive).
freeipa-4.4.0/asn1/asn1c/per_support.c:15: overrun-buffer-arg: Calling
"snprintf" with "buf[n]" and "64UL" is suspicious because the function
call may access "buf" at byte "n * sizeof (char [32]) + 63UL". [Note:
The source code implementation of the function has been overridden by a
builtin model.]
# 13| static int n;
# 14| n = (n+1) % 2;
# 15|-> snprintf(buf[n], sizeof(buf),
# 16| "{m=%ld span %+ld[%d..%d] (%d)}",
# 17| (long)pd->moved,
Error: FORWARD_NULL (CWE-476): [#def16]
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:1513: var_compare_op: Comparing
"st->buf" to null implies that "st->buf" might be null.
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:1618: alias_transfer: Assigning:
"buf" = "st->buf".
freeipa-4.4.0/asn1/asn1c/OCTET_STRING.c:1631: var_deref_model: Passing
null pointer "buf" to "per_put_many_bits", which dereferences it.
freeipa-4.4.0/asn1/asn1c/per_support.c:419:4: deref_parm: Directly
dereferencing parameter "src".
# 417|
# 418| if(nbits >= 24) {
# 419|-> value = (src[0] << 16) | (src[1] << 8) | src[2];
# 420| src += 3;
# 421| nbits -= 24;
More information about the Freeipa-devel
mailing list