Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Covscan fix #126

Closed
wants to merge 32 commits into from
Closed

Covscan fix #126

wants to merge 32 commits into from

Conversation

ljavorsk
Copy link

@ljavorsk ljavorsk commented Feb 6, 2020

I've managed to fix few thing in this project.

There may be some errors from my side too (hopefully not) so please review it.

Still fixing a lot of resource leaks, but most important things are here.

Covscan log:

Error: FORWARD_NULL (CWE-476):
mariadb-connector-c-3.1.6-src/libmariadb/ma_dtoa.c:1608: assign_zero: Assigning: "S" = "NULL".
mariadb-connector-c-3.1.6-src/libmariadb/ma_dtoa.c:1910: var_deref_model: Passing null pointer "S" to "Bfree", which dereferences it.
Covscan log:

Error: REVERSE_INULL (CWE-476):
mariadb-connector-c-3.1.6-src/libmariadb/mariadb_lib.c:2166: deref_ptr: Directly dereferencing pointer "mysql".
mariadb-connector-c-3.1.6-src/libmariadb/mariadb_lib.c:2171: check_after_deref: Null-checking "mysql" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
Covscan log:

Error: NEGATIVE_RETURNS (CWE-394):
mariadb-connector-c-3.1.6-src/plugins/auth/caching_sha2_pw.c:215: negative_return_fn: Function "ftell(fp)" returns a negative number.
mariadb-connector-c-3.1.6-src/plugins/auth/caching_sha2_pw.c:215: assign: Assigning: "*pub_key_size" = "ftell(fp)".
mariadb-connector-c-3.1.6-src/plugins/auth/caching_sha2_pw.c:221: negative_returns: "*pub_key_size" is passed to a parameter that cannot be negative. [Note: The source code implementation of the function has been overridden by a builtin model.]
curl_easy_setopt return 0 if succeeded and non-zero if fails
@ljavorsk ljavorsk requested a review from 9EOR9 February 11, 2020 09:44
@ljavorsk
Copy link
Author

Added an foundation for clearing possible resource leaks, I will continue with fixing them

plugins/pvio/pvio_socket.c Outdated Show resolved Hide resolved
plugins/pvio/pvio_socket.c Outdated Show resolved Hide resolved
@@ -50,7 +50,7 @@
/* qhasm: z2 = z1^2^1 */
/* asm 1: fe_sq(>z2=fe#1,<z1=fe#11); for (i = 1;i < 1;++i) fe_sq(>z2=fe#1,>z2=fe#1); */
/* asm 2: fe_sq(>z2=t0,<z1=z); for (i = 1;i < 1;++i) fe_sq(>z2=t0,>z2=t0); */
fe_sq(t0,z); for (i = 1;i < 1;++i) fe_sq(t0,t0);
/* fe_sq(t0,z); for (i = 1;i < 1;++i) fe_sq(t0,t0); */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only for loop should be removed/commented out

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, should I change it in the PR? Because I see that you've already made an commit with correct one.

@@ -70,7 +70,7 @@ fe_mul(t0,t0,t1);
/* qhasm: z22 = z11^2^1 */
/* asm 1: fe_sq(>z22=fe#1,<z11=fe#1); for (i = 1;i < 1;++i) fe_sq(>z22=fe#1,>z22=fe#1); */
/* asm 2: fe_sq(>z22=t0,<z11=t0); for (i = 1;i < 1;++i) fe_sq(>z22=t0,>z22=t0); */
fe_sq(t0,t0); for (i = 1;i < 1;++i) fe_sq(t0,t0);
/* fe_sq(t0,t0); for (i = 1;i < 1;++i) fe_sq(t0,t0); */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

9EOR9 added a commit that referenced this pull request Feb 14, 2020
Various coverity scan fixes, including CONC-452 and CONC-453.
Special thanks to Lukas Javorsky for fixing numerous covscan
issues (This patch includes part of his pull request #126).

Coverity scan build was using the following cmake parameters:
-WITH_EXTERNAL_ZLIB=ON -DWITH_UNIT_TESTS=OFF.

CWE-416 (use after free) in dtoa.c (from netlib) is still open.
@ljavorsk
Copy link
Author

@9EOR9 I've just added fix for all resource leaks I could handle, also a lot of missing checks are added. Some use after frees, string overflows. You can cherry-pick the ones that are suitable, because I was doing this for quite a while and something could already be changed

@ljavorsk
Copy link
Author

Statistics of this whole PR (how many fixed):
BAD_FREE 1
BAD_SHIFT 1
BUFFER_SIZE_WARNING 2
CHECKED_RETURN 37
CPPCHECK_WARNING 8
DEADCODE 2
FORWARD_NULL 2
NEGATIVE_RETURNS 1
NO_EFFECT 3
RESOURCE_LEAK 150
REVERSE_INULL 1
STRING_OVERFLOW 3
STRING_SIZE 1
UNUSED_VALUE 5
USE_AFTER_FREE 13
VARARGS 3

@9EOR9
Copy link
Collaborator

9EOR9 commented Jun 18, 2020

Closing the pull request, beside 1 bug in dtoa.c (frequently updated from netlib) all issues were eliminated.

@9EOR9 9EOR9 closed this Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants