Bug #89422 Dangerous enum-ulong casts in sql_formatter_options
Submitted: 25 Jan 2018 16:56 Modified: 26 Jan 2018 16:14
Reporter: Zsolt Parragi (OCA) Email Updates:
Status: Verified Impact on me:
None 
Category:MySQL Server: mysqldump Command-line Client Severity:S3 (Non-critical)
Version:5.7 OS:Any
Assigned to: CPU Architecture:Any

[25 Jan 2018 16:56] Zsolt Parragi 
Description:
The Sql_formatter_options class ends with the following two members:

enum enum_gtid_purged_mode m_gtid_purged;
const Mysql_chain_element_options* m_mysql_chain_element_options;

Just below this code, there is a function for using the enum with typelib:

   const TYPELIB* get_gtid_purged_mode_typelib()
   {
     static const char *gtid_purged_mode_names[4]= {"OFF", "AUTO", "ON", NullS};
     TYPELIB static gtid_purged_mode_typelib=
       {array_elements(gtid_purged_mode_names) -1, "",
         gtid_purged_mode_names, NULL};
     return &gtid_purged_mode_typelib;
   }

Which is used to read the enum.

Unfortunately during the reading the type information is lost, and an ulong (8 bytes) is read to that address, while enums are usually smaller than that, only 4 bytes.

On 64 bit architectures, this luckily works, as there is 4 byte padding before the m_mysql_chain_element_options pointer. On 32 bit builds, that pointer is overwritten when the enum is read. (because of the error checking of the parameter, on little endiad architectures, always with a 0. On big endian, with 0, 1 or 2)

This already resulted in a bug with big endian architectures, and somebody attempted to fix it. The related commit has a long comment explaining it:

https://github.com/mysql/mysql-server/commit/7c9dfc8b05f1efba1d653524d71dba054aee3852

Unfortunately, that fix doesn't solve the issue that the pointer after the enum is overwritten in 32 bit builds, and the used casts result in undefined behavior according the C++ standard. (resulting in warning with clang 5)

The correct fix for the issue would be reverting the linked commit, and changing the type of m_gtid_purged to ulong.

How to repeat:
Found as a warning when building with clang

Suggested fix:
reverting 7c9dfc8b05f1efba1d653524d71dba054aee3852, and changing the type of Sql_formatter::m_gtid_purged to ulong.
[26 Jan 2018 14:04] MySQL Verification Team 
Hi!

That particular field that you mentioned in the end is in the Sql_formatter_options class and not in Sql_formatter class, as you state. Otherwise your analysis is quite correct.

It should be mentioned that 32 builds will be discontinued in the future, so I truly do not know what will be the final priority of this bug.

Verified.
[26 Jan 2018 16:14] Zsolt Parragi 
While this bug in its current form only could cause issues for 32 bit builds, it is a possible bug source in 64 bit builds too, if somebody changes the code:

For example imagine that somebody adds a data member between the two variables I mentioned, an int, short, or anything which requires at most 4 byte alignment.
In this case, the new variable will occupy the currently unused memory space, and will cause bugs which will be quite hard to debug.
[13 Feb 2018 10:50] Zsolt Parragi 
Changing the enum to an ulong

(*) I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.

Contribution: enum-ulong.diff (text/x-patch), 2.14 KiB.