[Freeipa-devel] [PATCHES] Coverity fixes
Martin Basti
mbasti at redhat.com
Tue Aug 16 10:34:45 UTC 2016
On 14.08.2016 10:59, Simo Sorce wrote:
> On Thu, 2016-08-11 at 14:51 +0200, Martin Basti wrote:
>> 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?
> Yes we definitely want it pushed, so covscan can look at it and we can
> commit fixes before the upstream project (which is very slow) takes them
> in.
>
> Once these errors crop up in our covscan report I will take another pass
> at fixing the real ones and ignoring false positives.
>
> Simo.
>
>> 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;
>
I put missing files to makefile before push
ACK
master:
* 512aa90bec108a1d523ac5868342c68892f2c4cb Regenerate asn1 code
* cf0816f41503c9a556373b37b987dcb7a9be040b Additional coverity fixes.
More information about the Freeipa-devel
mailing list