MySQL Bugs: #61979: MySQL 5.5 and PVS-Studio

Bug #61979	MySQL 5.5 and PVS-Studio
Submitted:	26 Jul 2011 5:41	Modified:	26 Jul 2011 11:51
Reporter:	Andrey Karpov	Email Updates:
Status:	Verified	Impact on me:	None
Category:	MySQL Server: General	Severity:	S3 (Non-critical)
Version:	5.5.16	OS:	Any
Assigned to:		CPU Architecture:	Any

Description:
I checked the MySQL 5.5 project using the PVS-Studio static code analyzer.
I just glanced through the code but managed to find a few obviously odd
fragments. Below I will cite the analyzer-generated messages I have
studied and the corresponding code fragments. I hope this will help to
improve the project a bit.

------------------------------
V501 There are identical sub-expressions to the left and to the right of the '&&' operator: val != end && val != end sql item_timefunc.cc

static date_time_format_types
get_date_time_result_type(const char *format, uint length)
{
  ...
  for (; val != end && val != end; val++)
  ...
}

Strange code.
------------------------------
V511 The sizeof() operator returns size of the pointer, and not of the array, in 'sizeof (buf)' expression. auth_win_client common.cc 489

const char* get_last_error_message(Error_message_buf buf)
{
  int error= GetLastError();

  buf[0]= '\0';
  FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM,
    NULL, error, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
    (LPTSTR)buf, sizeof(buf), NULL );

  return buf;
}

Size of 'buf' must passed to function.
-------------------------
V547 Expression 'str [0] != 'a' || str [0] != 'A'' is always true. Probably the '&&' operator should be used here. clientlib my_time.c 340

enum enum_mysql_timestamp_type
str_to_datetime(...)
{
  ...
  else if (str[0] != 'a' || str[0] != 'A')
    continue; /* Not AM/PM */
  ...
}
------------------------------
V557 Array overrun is possible. The '16' index is pointing beyond array bound. semisync_master semisync_master.h 162

#define BLOCK_TRANX_NODES 16

struct Block {
  Block *next;
  TranxNode nodes[BLOCK_TRANX_NODES];
};

int free_nodes_before(TranxNode* node)
{
  ...
  if (&(block->nodes[0]) <= node &&
      &(block->nodes[BLOCK_TRANX_NODES]) >= node)
  ...
}
------------------------------
V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. innobase ha_innodb.cc 6789

int ha_innobase::create(...)
{
  ...
  if (srv_file_per_table
      && !mysqld_embedded
      && (!create_info->options & HA_LEX_CREATE_TMP_TABLE)) {
  ...
}

Need:
(!(create_info->options & HA_LEX_CREATE_TMP_TABLE))
------------------------------
V519 The 'stats.max_data_file_length' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1597, 1598. archive ha_archive.cc 1598

int ha_archive::info(uint flag)
{
  ...
  stats.max_data_file_length= share->rows_recorded * stats.mean_rec_length;
  stats.max_data_file_length= MAX_FILE_SIZE;
  ...
}

Strange code.
------------------------------
V525 The code containing the collection of similar blocks. Check items '0', '1', '2', '3', '4', '1', '6' in lines 680, 682, 684, 689, 691, 693, 695. sql records.cc 680

static int rr_cmp(uchar *a,uchar *b)
{
  if (a[0] != b[0])
    return (int) a[0] - (int) b[0];
  if (a[1] != b[1])
    return (int) a[1] - (int) b[1];
  if (a[2] != b[2])
    return (int) a[2] - (int) b[2];
  if (a[3] != b[3])
    return (int) a[3] - (int) b[3];
  if (a[4] != b[4])
    return (int) a[4] - (int) b[4];
  if (a[5] != b[5])
    return (int) a[1] - (int) b[5];
  if (a[6] != b[6])
    return (int) a[6] - (int) b[6];
  return (int) a[7] - (int) b[7];
}

CopyPaste error:
return (int) a[1] - (int) b[5];
Need:
return (int) a[5] - (int) b[5];
------------------------------

How to repeat:
You may review other odd fragments by downloading PVS-Studio from here:
http://www.viva64.com/en/pvs-studio-download/

I can also give you a registration key for some time.
You are welcome to ask questions here:
http://www.viva64.com/en/about-feedback/

Verified with current mysql-5.5 from bzr by code review. Indeed, at least some of the code fragments found look logically incorrect, like this one ("sql-common/my_time.c" line 340 of 1257):

          if (str[0] == 'p' || str[0] == 'P')
            add_hours= 12;
          else if (str[0] != 'a' || str[0] != 'A')
            continue;                           /* Not AM/PM */

I became curious how do the pointed out issues stand five years later and looked into them:

1. Issue fixed in 8.0 by

commit 3c270a5f78e8625ad5110b72f7a578a035443f5c
Author: Jon Olav Hauglid <jon.hauglid@oracle.com>
Date:   Wed May 4 09:31:26 2016 +0200

    Bug#23195845: FIX COMPILE WARNINGS REPORTED BY GCC 6.1
    
    Fix compile warnings reported by recently released GCC 6.1:
    
    -Werror=misleading-indentation
    libbinlogevents/src/uuid.cpp
    rapid/plugin/x/mysqlxtest_src/mysqlx.cc
    sql/sql_update.cc
    This error had to be disabled for files using Boost.
    
    -Werror=nonnull-compare
    storage/innobase/include/dict0dict.ic
    
    -Werror=logical-op
    mysys/mf_dirname.cc
    sql/item_timefunc.cc
    sql/mysqld.cc
    vio/viosocket.cc
    
    Also fix problem in mysys/my_sync.cc where we were using
    __linux rather than the correct __linux__ symbol.

2. The code is still the same

3. Issue fixed in 8.0 by

commit eff9bbedd4ccb20eff352f3b3ea8f7f2319e909a
Author: Jon Olav Hauglid <jon.hauglid@oracle.com>
Date:   Mon Jun 29 08:28:01 2015 +0100

    Bug#21285604: ENABLE -WMISSING-FORMAT-ATTRIBUTE
                  AND -WLOGICAL-OP WARNINGS
    
    This patch adds -Wmissing-format-attribute and -Wlogical-op
    to the list of warnings reported by GCC. The patch also fixes
    a number of code issues/bugs that were revealed by these
    warnings. The mistaken use of || rather than && in a few
    boolean expressions and wrong number/type of arguments to
    printf-style functions.
    
    The patch also removes some redundant declarations found
    with -Wredundant-decls

4. The code is still the same

5. Issue fixed in 5.6 by

commit c028d88f46a301edeb2b56bd2eefad311af705a5
Author: kevin.lewis@oracle.com <>
Date:   Wed Jun 6 11:29:11 2012 -0500

    Bug#13809713-SUSPICIOUS CHECK IN HA_INNOBASE::CREATE()
    Approved by Marco on IM

6. The code is still the same

7. Issue fixed in 5.5 by

commit 0a8ae683297e494703c50d362e211d3ed1f3f5c3
Author: Tor Didriksen <tor.didriksen@oracle.com>
Date:   Tue Jun 5 15:53:39 2012 +0200

    Bug#14051002 VALGRIND: CONDITIONAL JUMP OR MOVE IN RR_CMP / MY_QSORT
    
    Patch for 5.1 and 5.5: fix typo in byte comparison in rr_cmp().

To sum up, out of 7 original bugs, one has been fixed in 5.5+, one in 5.6+, two in 8.0, and three still stand.