CONC-786: static analysis cleanup #293
Open
Conversation
This file contains hidden or 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
The Infer static analyser regards DEAD_STORES are writing to a variable that isn't used. This remove those writes.
When allocation fails this returns. The handling in the calling
function isn't implemented yet.
libmariadb/ma_dtoa.c:646: error: Null Dereference(NULLPTR_DEREFERENCE)
`rv` could be null (null value originating from line 644) and is dereferenced.
644. rv= (Bigint*) malloc(len);
645.
646. rv->k= k;
^
647. rv->maxwds= x;
648. }
libmariadb/mariadb_stmt.c:2414: error: Null Dereference(NULLPTR_DEREFERENCE)
`cmd_buff` could be null (null value originating from line 2413) and is dereferenced.
2412. size_t packet_len= STMT_ID_LENGTH + 2 + length;
2413. uchar *cmd_buff= (uchar *)calloc(1, packet_len);
2414. int4store(cmd_buff, stmt->stmt_id);
^
2415. int2store(cmd_buff + STMT_ID_LENGTH, param_number);
2416. memcpy(cmd_buff + STMT_ID_LENGTH + 2, data, length);
plugins/auth/my_auth.c:308: error: Null Dereference(NULLPTR_DEREFERENCE)
`buff` could be null (null value originating from line 218) and is dereferenced.
306. if (!(mysql->server_capabilities & CLIENT_MYSQL))
307. mysql->client_flag&= ~CLIENT_MYSQL;
308. int4store(buff,mysql->client_flag);
^
This came up as deadstores in infer, but we can test these return values.
This could fail OOM, so just fail the test, unlikely as it is.
This resolves Infer errors;
unittest/libmariadb/async.c:151: error: Memory Leak(MEMORY_LEAK_C)
Memory dynamically allocated by `malloc`, indirectly via call to `mysql_init()` on line 149 is not freed after the last access at line 151, column 5.
149. mysql_init(&mysql);
150. rc= mysql_options(&mysql, MYSQL_OPT_NONBLOCK, 0);
151. check_mysql_rc(rc, (MYSQL *)&mysql);
^
and:
unittest/libmariadb/bulk1.c:231: error: Memory Leak(MEMORY_LEAK_C)
Memory dynamically allocated by `malloc`, indirectly via call to `mysql_stmt_bind_param()` on line 230 is not freed after the last access at line 231, column 3.
229.
230. rc= mysql_stmt_bind_param(stmt, bind);
231. check_stmt_rc(rc, stmt);
…struct Infer error is: libmariadb/ma_io.c:126: error: Uninitialized Value(PULSE_UNINITIALIZED_VALUE) `mysql.options.extension` is read without initialization during the call to `mysql_client_find_plugin()`. 124. MYSQL mysql; 125. if (rio_plugin ||(rio_plugin= (struct st_mysql_client_plugin_REMOTEIO *) 126. mysql_client_find_plugin(&mysql, NULL, MARIADB_CLIENT_REMOTEIO_PLUGIN))) The mysql structure here is obsecuring the 'mysql' pointer passed to the function. The mysql_client_find_plugin uses the mysql.options.extension in mysql_load_plugin_v to determine if a path or environment variable is used to load the plugin. As the mysql pointer passed in is assumed to have been initialized. This wasn't the case with mariadb_rpl_open where the implementation adds a mysql_init, and closes it before returning.
Infer error is:
ibmariadb/mariadb_async.c:86: error: Uninitialized Value(PULSE_UNINITIALIZED_VALUE)
`sock` is read without initialization.
84. application context. The application will then resume us when the socket
85. polls ready for write, indicating that the connection attempt completed.
86. */
^
Resolving this by returning an error for my_connect_async on a true value
returned by ma_pvio_get_handle (indicating an error).
The init_read_hdr static function is called from 3
locations within this file for a hdr on the stack.
This was the logical location to initialize offset.
Infer error:
libmariadb/mariadb_dyncol.c:4145: error: Uninitialized Value(PULSE_UNINITIALIZED_VALUE)
`hdr.offset` is read without initialization during the call to `hdr_interval_length()`.
4143. goto err;
4144. header.length=
4145. hdr_interval_length(&header, header.entry + header.entry_size);
^
4146. header.data= header.dtpool + header.offset;
4147. /*
…dling
From Infer:
libmariadb/mariadb_lib.c:784: error: Null Dereference(NULLPTR_DEREFERENCE)
`options->init_command` could be null (null value originating from line 783) and is dereferenced in the call to `ma_init_dynamic_array()`.
782. {
783. options->init_command= (DYNAMIC_ARRAY*)malloc(sizeof(DYNAMIC_ARRAY));
784. ma_init_dynamic_array(options->init_command, sizeof(char*), 5, 5);
^
To resolve the static function we return true on this error,
and let the mysql_optionsv return an CR_OUT_OF_MEMORY if
either allocation failed.
Avoid nullptr defererence:
libmariadb/mariadb_lib.c:1042: error: Null Dereference(NULLPTR_DEREFERENCE)
null (null value originating from line 1042) is dereferenced.
1040. ***************************************************************************/
1041.
1042. static size_t rset_field_offsets[]= {
^
1043. OFFSET(MYSQL_FIELD, catalog),
1044. OFFSET(MYSQL_FIELD, catalog_length),
libmariadb/mariadb_lib.c:1078: error: Null Dereference(NULLPTR_DEREFERENCE) `last_length` could be null (null value originating from line 1063) and is dereferenced. 1076. /* NULL_LENGTH (see also CONC-709) */ 1077. rc= 1; 1078. *last_length= 0; To be sure we check that last_length isn't null.
Where the feof failed, possibly in remote_io, this leaked memory and there was no client error.
libmariadb/mariadb_rpl.c:1247: error: Dead Store(DEAD_STORE)
The value written to `&ev` is never used.
1245. RPL_CHECK_POS(ev, ev_end, len);
1246. rpl_set_string_and_len(&rpl_event->event.execute_load_query.statement, ev, len);
1247. ev+= len;
^
remove increment as result isn't used.
Handle malloc failure.
plugins/io/remote_io.c:317: error: Memory Leak(MEMORY_LEAK_C) Memory dynamically allocated by `malloc` on line 286 is not freed after the last access at line 317, column 5. 315. curl_easy_cleanup(rf->curl); 316. 317. free(file);
And check DROP USER result after making it an IF EXISTS variant.
grooverdan
force-pushed
the
3.3-infer
branch
2 times, most recently
from
626494e to
88849b7
Compare
The net.extension is always dynamicly allocated. Its freeing shouldn't depend on free_me. Its important to set this pointer to null so that anything that calls mysql_close on this failed initialization, like mariadb_reconnect does, will not double-free the pointer. mariadb_reconnect doesn't need to set free_me to 0, it already was by virtue of a pointer passed to mysql_init.
With the previous commit, a mysql_init failure leaves no memory
allocated, so there's no need to close.
This avoids a Infer error (but I didn't see the cause):
libmariadb/mariadb_lib.c:2108: error: Uninitialized Value(PULSE_UNINITIALIZED_VALUE)
`mysql.extension` is read without initialization during the call to `mysql_close()`.
MYSQL tmp_mysql;
if (!mysql_init(&tmp_mysql))
{
/* extensions may have failed to allocate */
SET_CLIENT_ERROR(mysql, CR_OUT_OF_MEMORY, SQLSTATE_UNKNOWN, 0);
2106. tmp_mysql.free_me= 0;
2107. mysql_close(&tmp_mysql);
2108. return(1);
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.
lots of changes in error handling.
few in removing uncecessary
few false positives per CONC entry
test case handing of deadstore and allocation leaks incomplete.
Using github actions for testing instead of local testing.