[Commits] dadf5f78411: Fix few compiler warnings.
revision-id: dadf5f78411b8f46ac53b8ea12c08ec317d9aab4 (mariadb-10.1.38-7-gdadf5f78411) parent(s): 346e46089621e6951e076c82ed5690aa23dcb5fe author: Jan Lindström committer: Jan Lindström timestamp: 2019-02-20 08:10:26 +0200 message: Fix few compiler warnings. dict_table_rename_in_cache dict_create_add_foreign_id Use of strncpy caused string truncation warnings and is replaced with memcpy. fts_fetch_index_words dfield_get_len() can return UNIV_SQL_NULL (=ULINT32_UNDEFINED) that is too big for used short datatype. Replaced with correct ulint. row_rename_table_for_mysql Target string was exactly same length as maximum length of copied string. --- storage/innobase/dict/dict0dict.cc | 12 ++++++------ storage/innobase/fts/fts0opt.cc | 10 +++++----- storage/innobase/include/dict0crea.ic | 4 ++-- storage/innobase/row/row0mysql.cc | 4 ++-- storage/xtradb/dict/dict0dict.cc | 12 ++++++------ storage/xtradb/fts/fts0opt.cc | 10 +++++----- storage/xtradb/include/dict0crea.ic | 4 ++-- storage/xtradb/row/row0mysql.cc | 4 ++-- 8 files changed, 30 insertions(+), 30 deletions(-) diff --git a/storage/innobase/dict/dict0dict.cc b/storage/innobase/dict/dict0dict.cc index 06c6c3effab..04b14e3ae06 100644 --- a/storage/innobase/dict/dict0dict.cc +++ b/storage/innobase/dict/dict0dict.cc @@ -1845,7 +1845,7 @@ dict_table_rename_in_cache( ulint db_len; char* old_id; - char old_name_cs_filename[MAX_TABLE_NAME_LEN+20]; + char old_name_cs_filename[MAX_TABLE_NAME_LEN+20]=""; uint errors = 0; /* All table names are internally stored in charset @@ -1855,13 +1855,13 @@ dict_table_rename_in_cache( in UTF-8 charset. The variable fkid here is used to store foreign key constraint name in charset my_charset_filename for comparison further below. */ - char fkid[MAX_TABLE_NAME_LEN+20]; + char fkid[MAX_TABLE_NAME_LEN+20]=""; ibool on_tmp = FALSE; /* The old table name in my_charset_filename is stored in old_name_cs_filename */ - strncpy(old_name_cs_filename, old_name, + memcpy(old_name_cs_filename, old_name, MAX_TABLE_NAME_LEN); if (strstr(old_name, TEMP_TABLE_PATH_PREFIX) == NULL) { @@ -1883,7 +1883,7 @@ dict_table_rename_in_cache( } else { /* Old name already in my_charset_filename */ - strncpy(old_name_cs_filename, old_name, + memcpy(old_name_cs_filename, old_name, MAX_TABLE_NAME_LEN); } } @@ -1922,7 +1922,7 @@ dict_table_rename_in_cache( } /* Convert the table name to UTF-8 */ - strncpy(table_name, table->name, + memcpy(table_name, table->name, MAX_TABLE_NAME_LEN); innobase_convert_to_system_charset( strchr(table_name, '/') + 1, @@ -1934,7 +1934,7 @@ dict_table_rename_in_cache( from charset my_charset_filename to UTF-8. This means that the table name is already in UTF-8 (#mysql#50). */ - strncpy(table_name, table->name, + memcpy(table_name, table->name, MAX_TABLE_NAME_LEN); } diff --git a/storage/innobase/fts/fts0opt.cc b/storage/innobase/fts/fts0opt.cc index 77293bc867a..2a97f0fa527 100644 --- a/storage/innobase/fts/fts0opt.cc +++ b/storage/innobase/fts/fts0opt.cc @@ -673,18 +673,18 @@ fts_fetch_index_words( fts_zip_t* zip = static_cast<fts_zip_t*>(user_arg); que_node_t* exp = sel_node->select_list; dfield_t* dfield = que_node_get_val(exp); - short len = static_cast<short>(dfield_get_len(dfield)); + ulint len = dfield_get_len(dfield); void* data = dfield_get_data(dfield); + ut_a(len <= FTS_MAX_WORD_LEN); + /* Skip the duplicate words. */ - if (zip->word.f_len == static_cast<ulint>(len) + if (zip->word.f_len == len && !memcmp(zip->word.f_str, data, len)) { return(TRUE); } - ut_a(len <= FTS_MAX_WORD_LEN); - memcpy(zip->word.f_str, data, len); zip->word.f_len = len; @@ -693,7 +693,7 @@ fts_fetch_index_words( /* The string is prefixed by len. */ zip->zp->next_in = reinterpret_cast<byte*>(&len); - zip->zp->avail_in = sizeof(len); + zip->zp->avail_in = sizeof(static_cast<short>(len)); /* Compress the word, create output blocks as necessary. */ while (zip->zp->avail_in > 0) { diff --git a/storage/innobase/include/dict0crea.ic b/storage/innobase/include/dict0crea.ic index 1cbaa47032b..cebbcf73864 100644 --- a/storage/innobase/include/dict0crea.ic +++ b/storage/innobase/include/dict0crea.ic @@ -68,7 +68,7 @@ dict_create_add_foreign_id( char table_name[MAX_TABLE_NAME_LEN + 20] = ""; uint errors = 0; - strncpy(table_name, name, + memcpy(table_name, name, MAX_TABLE_NAME_LEN + 20); innobase_convert_to_system_charset( @@ -77,7 +77,7 @@ dict_create_add_foreign_id( MAX_TABLE_NAME_LEN, &errors); if (errors) { - strncpy(table_name, name, + memcpy(table_name, name, MAX_TABLE_NAME_LEN + 20); } diff --git a/storage/innobase/row/row0mysql.cc b/storage/innobase/row/row0mysql.cc index 2a9ade1da2c..84f30e226dd 100644 --- a/storage/innobase/row/row0mysql.cc +++ b/storage/innobase/row/row0mysql.cc @@ -5173,8 +5173,8 @@ row_rename_table_for_mysql( if (!new_is_tmp) { /* Rename all constraints. */ - char new_table_name[MAX_TABLE_NAME_LEN] = ""; - char old_table_utf8[MAX_TABLE_NAME_LEN] = ""; + char new_table_name[MAX_TABLE_NAME_LEN+1] = ""; + char old_table_utf8[MAX_TABLE_NAME_LEN+1] = ""; uint errors = 0; strncpy(old_table_utf8, old_name, MAX_TABLE_NAME_LEN); diff --git a/storage/xtradb/dict/dict0dict.cc b/storage/xtradb/dict/dict0dict.cc index 1c489d13f1a..d23be874c2f 100644 --- a/storage/xtradb/dict/dict0dict.cc +++ b/storage/xtradb/dict/dict0dict.cc @@ -1851,7 +1851,7 @@ dict_table_rename_in_cache( ulint db_len; char* old_id; - char old_name_cs_filename[MAX_TABLE_NAME_LEN+20]; + char old_name_cs_filename[MAX_TABLE_NAME_LEN+20]=""; uint errors = 0; /* All table names are internally stored in charset @@ -1861,13 +1861,13 @@ dict_table_rename_in_cache( in UTF-8 charset. The variable fkid here is used to store foreign key constraint name in charset my_charset_filename for comparison further below. */ - char fkid[MAX_TABLE_NAME_LEN+20]; + char fkid[MAX_TABLE_NAME_LEN+20]=""; ibool on_tmp = FALSE; /* The old table name in my_charset_filename is stored in old_name_cs_filename */ - strncpy(old_name_cs_filename, old_name, + memcpy(old_name_cs_filename, old_name, MAX_TABLE_NAME_LEN); if (strstr(old_name, TEMP_TABLE_PATH_PREFIX) == NULL) { @@ -1889,7 +1889,7 @@ dict_table_rename_in_cache( } else { /* Old name already in my_charset_filename */ - strncpy(old_name_cs_filename, old_name, + memcpy(old_name_cs_filename, old_name, MAX_TABLE_NAME_LEN); } } @@ -1928,7 +1928,7 @@ dict_table_rename_in_cache( } /* Convert the table name to UTF-8 */ - strncpy(table_name, table->name, + memcpy(table_name, table->name, MAX_TABLE_NAME_LEN); innobase_convert_to_system_charset( strchr(table_name, '/') + 1, @@ -1940,7 +1940,7 @@ dict_table_rename_in_cache( from charset my_charset_filename to UTF-8. This means that the table name is already in UTF-8 (#mysql#50). */ - strncpy(table_name, table->name, + memcpy(table_name, table->name, MAX_TABLE_NAME_LEN); } diff --git a/storage/xtradb/fts/fts0opt.cc b/storage/xtradb/fts/fts0opt.cc index 77293bc867a..2a97f0fa527 100644 --- a/storage/xtradb/fts/fts0opt.cc +++ b/storage/xtradb/fts/fts0opt.cc @@ -673,18 +673,18 @@ fts_fetch_index_words( fts_zip_t* zip = static_cast<fts_zip_t*>(user_arg); que_node_t* exp = sel_node->select_list; dfield_t* dfield = que_node_get_val(exp); - short len = static_cast<short>(dfield_get_len(dfield)); + ulint len = dfield_get_len(dfield); void* data = dfield_get_data(dfield); + ut_a(len <= FTS_MAX_WORD_LEN); + /* Skip the duplicate words. */ - if (zip->word.f_len == static_cast<ulint>(len) + if (zip->word.f_len == len && !memcmp(zip->word.f_str, data, len)) { return(TRUE); } - ut_a(len <= FTS_MAX_WORD_LEN); - memcpy(zip->word.f_str, data, len); zip->word.f_len = len; @@ -693,7 +693,7 @@ fts_fetch_index_words( /* The string is prefixed by len. */ zip->zp->next_in = reinterpret_cast<byte*>(&len); - zip->zp->avail_in = sizeof(len); + zip->zp->avail_in = sizeof(static_cast<short>(len)); /* Compress the word, create output blocks as necessary. */ while (zip->zp->avail_in > 0) { diff --git a/storage/xtradb/include/dict0crea.ic b/storage/xtradb/include/dict0crea.ic index 1cbaa47032b..cebbcf73864 100644 --- a/storage/xtradb/include/dict0crea.ic +++ b/storage/xtradb/include/dict0crea.ic @@ -68,7 +68,7 @@ dict_create_add_foreign_id( char table_name[MAX_TABLE_NAME_LEN + 20] = ""; uint errors = 0; - strncpy(table_name, name, + memcpy(table_name, name, MAX_TABLE_NAME_LEN + 20); innobase_convert_to_system_charset( @@ -77,7 +77,7 @@ dict_create_add_foreign_id( MAX_TABLE_NAME_LEN, &errors); if (errors) { - strncpy(table_name, name, + memcpy(table_name, name, MAX_TABLE_NAME_LEN + 20); } diff --git a/storage/xtradb/row/row0mysql.cc b/storage/xtradb/row/row0mysql.cc index 93a4db98e7b..0ea2ae1b6d4 100644 --- a/storage/xtradb/row/row0mysql.cc +++ b/storage/xtradb/row/row0mysql.cc @@ -5183,8 +5183,8 @@ row_rename_table_for_mysql( if (!new_is_tmp) { /* Rename all constraints. */ - char new_table_name[MAX_TABLE_NAME_LEN] = ""; - char old_table_utf8[MAX_TABLE_NAME_LEN] = ""; + char new_table_name[MAX_TABLE_NAME_LEN+1] = ""; + char old_table_utf8[MAX_TABLE_NAME_LEN+1] = ""; uint errors = 0; strncpy(old_table_utf8, old_name, MAX_TABLE_NAME_LEN);
On February 20, 2019, Jan Lindström wrote:
revision-id: dadf5f78411b8f46ac53b8ea12c08ec317d9aab4 (mariadb-10.1.38-7-gdadf5f78411) parent(s): 346e46089621e6951e076c82ed5690aa23dcb5fe author: Jan Lindström committer: Jan Lindström timestamp: 2019-02-20 08:10:26 +0200 message:
Fix few compiler warnings.
The commit message should include the ticket number. I would also suggest fixing the grammar: MDEV-18659: Fix a few compiler warnings Could you make the title more specific?
dict_table_rename_in_cache dict_create_add_foreign_id Use of strncpy caused string truncation warnings and is replaced with memcpy.
Specifically, which warnings were issued? I am concerned that this could be replacing dubious code with worse. With memcpy(), you should be sure that the source strings is really safe to access up to the specified length. With strncpy(), the copying will stop at the first NUL character in the output, or at the specified length, whichever comes first.
fts_fetch_index_words dfield_get_len() can return UNIV_SQL_NULL (=ULINT32_UNDEFINED) that is too big for used short datatype. Replaced with correct ulint.
Is NULL really an allowed value there? I do not think that the "words" or "tokens" (the keys of the inverted index) should ever be NULL. Likewise, if the entire fulltext-indexed column is NULL, we should not be inserting anything into the inverted index. Please ensure that these cases are covered by test cases in mysql-test-run, and share the detailed results: especially, which pieces of code are handling the NULL values. The data that is being fed to fts_fetch_index_words() comes from this query to the InnoDB internal SQL parser: " SELECT word\n" " FROM $table_name\n" " WHERE word > :word\n" " ORDER BY word;\n" These tables appear to be created in fts_create_one_index_table(), and it seems to me that it is PRIMARY KEY(word,first_doc_id). Strangely, I do not see DATA_NOT_NULL in the column creation, so it is possible that we are reserving NULL flags for the "word" column. But, InnoDB is not prepared to deal with NULL values in the primary key. Lots of things would break if "word" were NULL. Please add assertions to the places where "word" is being written or read in these tables. Here is the call that forgets to declare "word" as NOT NULL: dict_mem_table_add_col(new_table, heap, "word", charset == &my_charset_latin1 ? DATA_VARCHAR : DATA_VARMYSQL, field->col->prtype, FTS_MAX_WORD_LEN_IN_CHAR * unsigned(field->col->mbmaxlen));
row_rename_table_for_mysql Target string was exactly same length as maximum length of copied string.
Fixing this does make sense.
diff --git a/storage/innobase/dict/dict0dict.cc b/storage/innobase/dict/dict0dict.cc index 06c6c3effab..04b14e3ae06 100644 --- a/storage/innobase/dict/dict0dict.cc +++ b/storage/innobase/dict/dict0dict.cc @@ -1845,7 +1845,7 @@ dict_table_rename_in_cache(
ulint db_len; char* old_id; - char old_name_cs_filename[MAX_TABLE_NAME_LEN+20]; + char old_name_cs_filename[MAX_TABLE_NAME_LEN+20]=""; uint errors = 0;
/* All table names are internally stored in charset
What is the initialization needed for? It was not mentioned in the commit comment. Note that this would likely not only initialize the first byte of the string, but all bytes. What kind of machine code is this generating? memcpy()? Or is the compiler smart enough to recognize that the stack variable is going to be zero-filled, and will emit something like memset() instead? Either way, it is not good to write MAX_TABLE_NAME_LEN+20 bytes if only 1 byte (a terminating NUL character at the start of the array) would suffice. And you would have to convince me that this initialization is necessary in the first place. Best regards, Marko Mäkelä Lead Developer InnoDB MariaDB Corporation
participants (2)
-
jan -
Marko Mäkelä