MDBF-534: Coverity scan: fix client folder #2496
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
cvicentiu
reviewed
Feb 14, 2023
client/mysqltest.cc
Outdated
| size_t len; | ||
| if (!sandbox) | ||
| return false; | ||
| else |
There was a problem hiding this comment.
No need for this else statement. Prefer to reduce indentation. See coding guidelines.md :)
an3l
force-pushed
the
bb-10.11-anel-coverity-client-v2
branch
from
779e2bd
to
4dd6c3a
Compare
LinuxJedi
requested changes
Feb 14, 2023
There was a problem hiding this comment.
Test is currently failing due to a crash in mariadb-dump:
#0 mysql_free_result (result=0xd8) at /home/buildbot/amd64-centos-7/build/libmariadb/libmariadb/mariadb_lib.c:598
598 if (result->handle && result->handle->status == MYSQL_STATUS_USE_RESULT)
#0 mysql_free_result (result=0xd8) at /home/buildbot/amd64-centos-7/build/libmariadb/libmariadb/mariadb_lib.c:598
#1 0x000055cbaaa47c24 in dump_table (table=<optimized out>, db=db@entry=0x7ffc7b460d04 "test", hash_key=hash_key@entry=0x0, len=len@entry=0) at /home/buildbot/amd64-centos-7/build/client/mysqldump.c:4656
#2 0x000055cbaaa3e5af in dump_selected_tables (tables=<optimized out>, table_names=<optimized out>, db=0x7ffc7b460d04 "test") at /home/buildbot/amd64-centos-7/build/client/mysqldump.c:6140
#3 main (argc=2, argv=0x55cbabc35988) at /home/buildbot/amd64-centos-7/build/client/mysqldump.c:7265
This is likely the new free of res in dump_table crashing due to not being initialised to NULL at the top of the function. Could also be a double-free, but I couldn't spot that in a quick scan. GDB should spot this with a debug build to be sure.
LinuxJedi
reviewed
Feb 14, 2023
an3l
force-pushed
the
bb-10.11-anel-coverity-client-v2
branch
from
4dd6c3a
to
95d47f0
Compare
an3l
force-pushed
the
bb-10.11-anel-coverity-client-v2
branch
from
95d47f0
to
f2b36c3
Compare
LinuxJedi
approved these changes
Feb 16, 2023
There was a problem hiding this comment.
I think this is all good now, although I think it should maybe be against 10.4 instead of 10.11.
@cvicentiu agreed?
--------------------------------- File: `mysql` --------------------------------- - Coverity (RESOURCE_LEAK): https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728394&defectInstanceId=53073025&mergedDefectId=1520090&eventId=53073025-15 `mysql`: memory allocated by `mysql_fetch_row` is not freed. - FALSE POSITIVES: - Coverity (TAINTED_SCALAR): - https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728394&defectInstanceId=53074559&mergedDefectId=1520403 - Coverity (COPY_PASTE_ERROR): https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728394&defectInstanceId=53074521&mergedDefectId=1520300 - Coverity (STRING_NULL): https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728394&defectInstanceId=53072524&mergedDefectId=1519374 - Coverity (CHECKED_RETURN): https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728394&defectInstanceId=53074932&mergedDefectId=971708 - INTENTIONAL: - Coverity (UNINIT): https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728394&defectInstanceId=53074758&mergedDefectId=1519932 https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728394&defectInstanceId=53073939&mergedDefectId=1519738 - Coverity(BAD_FREE): https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728394&defectInstanceId=53073938&mergedDefectId=1519491 https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728394&defectInstanceId=53074819&mergedDefectId=1519462 --------------------------------- File: `mysql_plugin` --------------------------------- - Coverity (FORWARD_NULL): https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728420&defectInstanceId=53074485&mergedDefectId=971915 Dereference after null check when using `fclose`. - FALSE POSITIVES: - Coverity (STRING_OVERFLOW): https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728420&defectInstanceId=53075014&mergedDefectId=972410 - Additionally fix typo
--------------------------------- File: `mysqladmin` --------------------------------- - Coverity (PRINTF_ARGS): https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728412&defectInstanceId=53073308&mergedDefectId=1520228&eventId=53073308-0 `mysql_upgrade` - extra argument to printf format specifiera - Coverity (TAINTED_SCALAR) - FAlSE POSITIVE: https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728412&defectInstanceId=53072897&mergedDefectId=1519349
--------------------------------- File: `mysqlbinlog` --------------------------------- - Coverity (FORWARD_NULL): https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728438&defectInstanceId=53074517&mergedDefectId=1519690&eventId=53074517-46 `mysqlbinlog` - for `opt_raw_mode` file is set to 0, make sure it opened before.
--------------------------------- File: `mysqlcheck` --------------------------------- - Coverity (FORWARD_NULL): https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728409&defectInstanceId=53075052&mergedDefectId=1520314&eventId=53075052-7 `mysqlcheck` - make sure `op` is non-null - Coverity (TAINTED_SCALAR) - FALSE POSITIVES: https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728409&defectInstanceId=53074482&mergedDefectId=1519904
--------------------------------
File: `mysqldump`:
--------------------------------
-Coverity (`BAD_SHIFT`):
https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728415&defectInstanceId=53073433&mergedDefectId=1211186&eventId=53073433-25
`mysqldump` - Error obtained by coverity is implication of type
conversion.
It may happen that function `find_type` returns -1 which
is assigned to `uint` that gets converted by compiler to max
(UINT_32/64). In that situation left bit shift may lead to UB.
Converting from `uint` to `int` will solve the problem.
- Coverity (`RESOURCE_LEAK`):
- https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728415&defectInstanceId=53072912&mergedDefectId=1519239
- https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728415&defectInstanceId=53073706&mergedDefectId=1519368
- https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728415&defectInstanceId=53073560&mergedDefectId=1519655
- https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728415&defectInstanceId=53074494&mergedDefectId=1519822&fileStart=4001&fileEnd=4250
- https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728415&defectInstanceId=53074999&mergedDefectId=1519915&eventId=53074999-53
- https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728415&defectInstanceId=53075060&mergedDefectId=1519964
- https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728415&defectInstanceId=53073268&mergedDefectId=1519967
- https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728415&defectInstanceId=53073015&mergedDefectId=1520164
`mysqldump` - in case of error memory should be freeed.
- Coverity (`UNINT`) - FALSE POSITIVES:
- https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728415&defectInstanceId=53074364&mergedDefectId=1519587&eventId=53074364-10
- https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728415&defectInstanceId=53072619&mergedDefectId=1519684&eventId=53072619-1
- https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728415&defectInstanceId=53073256&mergedDefectId=1519722
- https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728415&defectInstanceId=53074251&mergedDefectId=1519979
- https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728415&defectInstanceId=53074996&mergedDefectId=1520021
- https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728415&defectInstanceId=53073425&mergedDefectId=1520166&eventId=53073425-9
---------------------------------
File: `mysqladmin`
---------------------------------
- Coverity (PRECEDANCE_ERROR) a.k.a MDEV-15736:
https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728425&defectInstanceId=53074187&mergedDefectId=1519944
- Coverity (BAD_FREE) - FALSE POSITIVE:
https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728425&defectInstanceId=53074614&mergedDefectId=1520042
---------------------------------
File: `mysqlimport`
---------------------------------
- FALSE POSITIVES
- Coverity (TAINTED_SCALAR):
https://scan5.scan.coverity.com/reports.htm#v58936/p10357/ fileInstanceId=231728411&defectInstanceId=53074012&mergedDefectId=1519158&eventId=53074012-6
- Coverity (UNINT):
https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728411&defectInstanceId=53072860&mergedDefectId=1520020
---------------------------------
File: `mysqlshow`
---------------------------------
- FALSE POSITIVES
- Coverity (TAINTED_SCALAR):
https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728418&defectInstanceId=53074361&mergedDefectId=1519232&eventId=53074361-4
- Coverity (UNINT):
https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728411&defectInstanceId=53072860&mergedDefectId=1520020
- Coverity (BAD_FREE):
https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728418&defectInstanceId=53073408&mergedDefectId=1519972
--------------------------------- File: `mysqlslap` --------------------------------- - Coverity (CHECKED_RETURN): https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728428&defectInstanceId=53073524&mergedDefectId=1520114 - FALSE POSITIVES - Coverity (DC.WEAK_CRYPTO) (`random()`): https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728428&defectInstanceId=53073112&mergedDefectId=1225806 https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728428&defectInstanceId=53074491&mergedDefectId=1409160 https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728428&defectInstanceId=53074151&mergedDefectId=1409180 https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728428&defectInstanceId=53073799&mergedDefectId=1409183 - Coverity (TAINTED_SCALAR): https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728428&defectInstanceId=53074667&mergedDefectId=1519586 - Coverity (UNINT): - Coverity (BAD_FREE): https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728428&defectInstanceId=53074415&mergedDefectId=1520371
--------------------------------- File: `mysqltest` --------------------------------- - Coverity (SIZEOF_MISMATCH): - https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728385&defectInstanceId=53074863&mergedDefectId=972322 Function `qsort` have to use size of element that is `uchar *` - Coverity (REVERSE_INULL): - https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728385&defectInstanceId=53074524&mergedDefectId=1519693&fileStart=3376&fileEnd=3625 First check if null and then use `strlen`, not reversed. - FALSE POSITIVES - Coverity (TAINTED_SCALAR): https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728385&defectInstanceId=53074760&mergedDefectId=1519321 - Coverity (CHECKED_RETURN): - https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728385&defectInstanceId=53074692&mergedDefectId=971714 - https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728385&defectInstanceId=53072839&mergedDefectId=971715 - Coverity (FORWARD_NULL): There is already issued DBUG_ASSERT(query_end) few lines before https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728385&defectInstanceId=53074002&mergedDefectId=971916&eventId=53074002-5 - Coverity (OVERRUN): - https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728385&defectInstanceId=53074470&mergedDefectId=1519697 - https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728385&defectInstanceId=53074862&mergedDefectId=1520391 `uint64_max` and `SIZE_MAX` (max for `size_t`) are same as `count` argument for `memcmp`. - Coverity (RESOURCE_LEAK): - https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728385&defectInstanceId=53074163&mergedDefectId=1519889&eventId=53074163-446 - INTENTION: - Coverity (SIZEOF_MISMATCH): - https://scan5.scan.coverity.com/reports.htm#v58936/p10357/fileInstanceId=231728385&defectInstanceId=53074650&mergedDefectId=1520109 `len` argument is used only in printing so it is not making impact (may be removed as an alternative). In this example size of pointer (8B) is used, that is not the size of value that pointer points to.
LinuxJedi
force-pushed
the
bb-10.11-anel-coverity-client-v2
branch
from
f2b36c3
to
e78f433
Compare
cvicentiu
reviewed
Feb 20, 2023
| @@ -3169,7 +3169,7 @@ int main(int argc, char** argv) | |||
| /* Set delimiter back to semicolon */ | |||
| if (retval != ERROR_STOP) | |||
| { | |||
| if (!stop_event_string.is_empty()) | |||
| if (!stop_event_string.is_empty() && result_file) | |||
There was a problem hiding this comment.
@an3l @LinuxJedi I am confused about this check only happening here.
If this check is necessary here, why is the fprintf at line 3161 legal? That one doesn't check for opt_raw_mode either. Is opt_raw_mode mutually exclusive with opt_flashback?
There was a problem hiding this comment.
@cvicentiu coverity report said that in case whenopt_raw_mode is used and opt_flasback is 0, result_file is assigned to 0 (line 3071) and when comes to this line, fprintf is used on 0.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The purpose of this patch is to address scan by coverity on MariaDB client folder.
There are couple of commits for one or more files in client folder.
Each commit is addressing specific coverity status on found defect in specific file or files.
How can this PR be tested?
Check commit message and check coverity scan.
Basing the PR against the correct MariaDB version