Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-18659

Fix gcc-8 compiler warnings produced by -Wstringop-truncation and -Wstringop-overflow for InnoDB

Details

    Attachments

      Activity

        jplindst Jan Lindström (Inactive) added a comment - http://lists.askmonty.org/pipermail/commits/2019-February/013446.html

        I think that this needs some more research. I replied in
        http://lists.askmonty.org/pipermail/commits/2019-February/013448.html

        marko Marko Mäkelä added a comment - I think that this needs some more research. I replied in http://lists.askmonty.org/pipermail/commits/2019-February/013448.html
        jplindst Jan Lindström (Inactive) added a comment - http://lists.askmonty.org/pipermail/commits/2019-February/013451.html

        It looks like two of my review comments were not addressed:

        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.

        My second unaddressed comment was regarding unnecessary zero-initialization of all bytes in a string buffer. The revised change does seem to be touching code that performs such unnecessary zero-initialization.

        marko Marko Mäkelä added a comment - It looks like two of my review comments were not addressed: 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. My second unaddressed comment was regarding unnecessary zero-initialization of all bytes in a string buffer. The revised change does seem to be touching code that performs such unnecessary zero-initialization.

        I tried to reason on commit message why memcpy() is safe on those cases but maybe it was not clear. So we have:

        memcpy(old_name_cs_filename, old_name, MAX_TABLE_NAME_LEN);
        

        instead of strncpy() and used strings are defined on same functions as

        char	old_name[MAX_FULL_NAME_LEN + 1];
        char old_name_cs_filename[MAX_TABLE_NAME_LEN+20];
        

        And #define MAX_FULL_NAME_LEN (MAX_TABLE_NAME_LEN + MAX_DATABASE_NAME_LEN + 14) that
        is larger than MAX_TABLE_NAME_LEN in both cases. To avoid reading uninitialized memory we should
        zero-initialize those strings.

        I did not add any zero-initialization on latest version, they were there already, maybe they are unnecessary. I do not think real string truncation is possible.

        jplindst Jan Lindström (Inactive) added a comment - I tried to reason on commit message why memcpy() is safe on those cases but maybe it was not clear. So we have: memcpy(old_name_cs_filename, old_name, MAX_TABLE_NAME_LEN); instead of strncpy() and used strings are defined on same functions as char old_name[MAX_FULL_NAME_LEN + 1]; char old_name_cs_filename[MAX_TABLE_NAME_LEN+20]; And #define MAX_FULL_NAME_LEN (MAX_TABLE_NAME_LEN + MAX_DATABASE_NAME_LEN + 14) that is larger than MAX_TABLE_NAME_LEN in both cases. To avoid reading uninitialized memory we should zero-initialize those strings. I did not add any zero-initialization on latest version, they were there already, maybe they are unnecessary. I do not think real string truncation is possible.

        I would prefer to avoid unnecessary copying and initialization. That is why I prefer strncmp() to memcpy(). The minimum correctness requirement is that the string buffers be terminated by at least one NUL byte.

        marko Marko Mäkelä added a comment - I would prefer to avoid unnecessary copying and initialization. That is why I prefer strncmp() to memcpy() . The minimum correctness requirement is that the string buffers be terminated by at least one NUL byte.

        I do not see these warnings being emitted for MariaDB 10.2 or later. Could you try to see how the code differs between 10.1 and 10.2, and attempt a minimal backport to 10.1?

        marko Marko Mäkelä added a comment - I do not see these warnings being emitted for MariaDB 10.2 or later. Could you try to see how the code differs between 10.1 and 10.2, and attempt a minimal backport to 10.1?

        I am testing a similar patch that keeps the strncpy() calls except when using strcpy() or memcpy() would lead to functionally equivalent result. I will also fix the warnings in mariabackup.

        marko Marko Mäkelä added a comment - I am testing a similar patch that keeps the strncpy() calls except when using strcpy() or memcpy() would lead to functionally equivalent result. I will also fix the warnings in mariabackup .

        People

          marko Marko Mäkelä
          jplindst Jan Lindström (Inactive)
          Votes:
          0 Vote for this issue
          Watchers:
          2 Start watching this issue

          Dates

            Created:
            Updated:
            Resolved:

            Git Integration

              Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.