On Tue, 2016-08-16 at 12:34 +0200, Martin Basti wrote: > > 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.
Thanks a lot! Simo. -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code