maria-developers team mailing list archive

[Marko Mäkelä <marko.makela@xxxxxxxxxxx>]

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