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.

--
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

Reply via email to