[Freeipa-devel] [PATCHES] Coverity fixes
Simo Sorce
simo at redhat.com
Tue Aug 16 10:38:43 UTC 2016
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.
More information about the Freeipa-devel
mailing list