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

uint expression is used for the value that is passed as my_off_t for DDL log

    Details

      Description

      Valerii Kravchuk reported the following upstream bug that also seems to effect MariaDB:

      http://bugs.mysql.com/bug.php?id=83708

      We can see the same block of code in MariaDB 10.1:

      https://github.com/MariaDB/server/blob/a98c85bb501e9021c0d8d509b8c040611d4c0c3a/sql/sql_table.cc#L663

      and in MariaDB 10.0:

      https://github.com/MariaDB/server/blob/5569ac00590ba139bbc575c20de4c682919721e5/sql/sql_table.cc#L664

      and in MariaDB 5.5:

      https://github.com/MariaDB/server/blob/19e3597e0c718a4cfdfe8789c7b4b11a4e0ba0c6/sql/sql_table.cc#L677

      Bug description from upstream report:

      Please, check the following code in sql/sql_table.cc:
       
      static bool read_ddl_log_file_entry(uint entry_no)
      {
        bool error= FALSE;
        File file_id= global_ddl_log.file_id;
        uchar *file_entry_buf= (uchar*)global_ddl_log.file_entry_buf;
        uint io_size= global_ddl_log.io_size;
        DBUG_ENTER("read_ddl_log_file_entry");
       
        mysql_mutex_assert_owner(&LOCK_gdl);
        if (mysql_file_pread(file_id, file_entry_buf, io_size, io_size * entry_no,
                             MYF(MY_WME)) != io_size)
          error= TRUE;
        DBUG_RETURN(error);
      }
       
      I see two potential problems with it:
       
      1. We multiply two uint values, io_size (that is typically 4096, see:
       
      include/my_global.h:332:#define IO_SIZE                 4096
       
      etc)
       
      and entry_no, without any type cast. The resulting expression is going to be of uint type (4 bytes long unsigned), but formal parameter of mysql_file_pread (and my_pread that it ends up defined as) is of type my_off_t. This type is 8 bytes long (on 64-bit systems) usually:
       
      #if defined(_WIN32)
      typedef unsigned long long my_off_t;
      typedef unsigned long long os_off_t;
      #else
      typedef off_t os_off_t;
      #if SIZEOF_OFF_T > 4
      typedef ulonglong my_off_t;
      #else
      typedef unsigned long my_off_t;
      #endif
      #endif /*_WIN32*/
       
      So, we may end up passing the value of 4294955008 all the time whenever entry_no > 1048573
       
      Function to write DDL log (static bool write_ddl_log_file_entry(uint entry_no)) is affected in a similar way it seems.
       
      2. We pass MY_WME flag, that is, we just write error to the error log in case of failure, but continue attempts to read in my_pread() indefinitely:
       
      #define MY_FFNF         1       /* Fatal if file not found */
      #define MY_FNABP        2       /* Fatal if not all bytes read/writen */
      #define MY_NABP         4       /* Error if not all bytes read/writen */
      #define MY_FAE          8       /* Fatal if any error */
      #define MY_WME          16      /* Write message on error */
      ...
       
      So, when offset is big enough we start to loop indefinitely, adding messages to error log and holding LOCK_gdl mutex. As a result, any concurrent DDL activity is essentially blocked forever until we get rid of ddl_log.dll file that is 4G in size.
       
      How to repeat:
      Read the code.
       
      There are reasons to think that as soon as more than 1048573 entries (4096 bytes each) are written to the DDL Log (see https://dev.mysql.com/doc/refman/5.7/en/ddl-log.html) so that it becomes 4G in size, there is no way to read the entries from it successfully.
       
      Suggested fix:
      Make sure the value of offset passed to mysql_file_pread() and my_file_pwrite() is of type my_off_t in all cases and is not affected by artificial 4 bytes limits.
       
      Additionally, consider other flags (MY_FNABP?) when I/O happens with some mutex hold.
      

        Attachments

          Activity

            People

            • Assignee:
              wlad Vladislav Vaintroub
              Reporter:
              GeoffMontee Geoff Montee
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: