[Freeipa-devel] [PATCHES] Coverity fixes
Simo Sorce
simo at redhat.com
Sun Aug 14 08:59:56 UTC 2016
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;
--
Simo Sorce * Red Hat, Inc * New York
More information about the Freeipa-devel
mailing list