Bug #37968 Prepared statements byte/tinyint causes data corruption.
Submitted: 8 Jul 2008 12:56 Modified: 11 Jul 2008 21:43
Reporter: Peter van Gulik Email Updates:
Status: Closed Impact on me:
None 
Category:Connector / NET Severity:S2 (Serious)
Version:5.1.6 OS:Windows (Server 2003)
Assigned to: CPU Architecture:Any
Tags: byte, C#, NET, prepared statements, Tinyint

[8 Jul 2008 12:56] Peter van Gulik
Description:
The bug manifests its self when you run a prepared insert query that contains a byte (unsigned tinyint) in the parameter list. Te result is that the complete query and data that should be inserted get's messed up no error is being thrown. 

In my case I tried to insert/update 3 byte fields at the start of a table using a insert on duplicate key query.

Running the same query without calling prepare() solves the problem, but obviously the command does not get prepared then. My table consists of +30 fields and contains binary, varchar, char, (unsigned) int fields.

Only the first byte field got inserted correctly.

The table:
CREATE TABLE `p_characters` (
  `guid` int(11) unsigned NOT NULL,
  `corpse` int(11) unsigned NOT NULL,
  `charm` bigint(22) unsigned NOT NULL,
  `name` varchar(255) NOT NULL DEFAULT '',
  `level` int(11) NOT NULL,
  `race` tinyint(3) unsigned NOT NULL,
  `gender` tinyint(3) unsigned NOT NULL,
  `class` tinyint(3) unsigned NOT NULL,
  `exp` int(11) unsigned NOT NULL,
  `money` int(11) unsigned NOT NULL,
  `hp` int(11) unsigned NOT NULL,
  `mana` int(11) unsigned NOT NULL,
  `my_faction` int(11) unsigned NOT NULL,
  `player_skin` int(11) unsigned NOT NULL,
  `player_hair` int(11) unsigned NOT NULL,
  `player_facial` int(11) unsigned NOT NULL,
  `player_head_model` int(11) unsigned NOT NULL DEFAULT '0',
  `base_flags` int(11) unsigned NOT NULL,
  `player_flags` int(11) unsigned NOT NULL,
  `owner` int(22) unsigned NOT NULL DEFAULT '0' COMMENT 'Account that owns this character',
  `position` char(64) NOT NULL DEFAULT '',
  `homebind` char(64) NOT NULL,
  `logout_stamp` bigint(22) NOT NULL DEFAULT '0' COMMENT 'Binary format.',
  `group` int(11) unsigned NOT NULL DEFAULT '0',
  `rank` int(11) NOT NULL DEFAULT '0',
  `mynotes_1` varchar(255) NOT NULL DEFAULT 'None',
  `mynotes_2` varchar(255) NOT NULL DEFAULT 'None',
  `gain_pt` int(11) unsigned NOT NULL,
  `deer` int(11) unsigned NOT NULL,
  `pt_mask` binary(32) NOT NULL,
  `exp_bytes_1` binary(128) NOT NULL,
  `exp_bytes_2` binary(128) NOT NULL,
  `db_flags` int(11) unsigned NOT NULL DEFAULT '0',
  PRIMARY KEY (`guid`),
  UNIQUE KEY `name` (`name`),
  KEY `account` (`owner`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1;

How to repeat:
Create a table containing several columns of which atleast 1 of the unsigned tinyint type. Create a insert query for that table and prepare it using mysql connector/net. Execute it and verify the data inserted.

Suggested fix:
Cast the byte values to int values.
[8 Jul 2008 12:57] Peter van Gulik
My suggested fix is to the people that also have this problem and not a production fix.
[9 Jul 2008 17:26] Reggie Burnett
Peter

Can you show me a snippet of code that reproduces this?  I can't reproduce using 5.0.10 and 5.1.7 (current branch code)
[9 Jul 2008 19:36] Peter van Gulik
This is the snipset is used to reproduce the error, note I connect with the database using namedpipes that might also influence the behavior. You should be able to compile it right away.
--------------------------------------------------------
CREATE TABLE `test` (
  `id` int(11) unsigned NOT NULL,
  `byte1` tinyint(3) unsigned NOT NULL,
  `byte2` tinyint(3) unsigned NOT NULL,
  `stringtest` varchar(255) NOT NULL,
  PRIMARY KEY (`id`)
) ENGINE=MyISAM DEFAULT CHARSET=latin1;
--------------------------------------------------------
using System;
using System.Data;
using System.Data.Common;
using MySql.Data.MySqlClient;

class Program
{
    static void Main(string[] args)
    {
        DbProviderFactory factory = MySqlClientFactory.Instance;
        IDbConnection conn = factory.CreateConnection();

        conn.ConnectionString =
            "protocol=NamedPipe;pooling=True;" +
            "ignore prepare=False;pipe=MySQL;" +
            "maximum pool size=25;user=root;password=root;" +
            "host=localhost;database=wsharp";
        conn.Open();

        uint id = 1;
        byte b1 = 10;
        byte b2 = 5;

        using (IDbCommand cmd = factory.CreateCommand())
        {
            cmd.CommandText = "INSERT INTO `test` (`id`,`byte1`,`byte2`,`stringtest`) VALUES (?id,?b1,?b2,?st)";
            cmd.Parameters.Add(new MySqlParameter("?id", id));
            cmd.Parameters.Add(new MySqlParameter("?b1", b1));
            cmd.Parameters.Add(new MySqlParameter("?b2", b2));
            cmd.Parameters.Add(new MySqlParameter("?st", "init st"));

            cmd.Connection = conn;
            cmd.Prepare();

            for (int i = 1; i < 11; i++)
            {
                ((IDataParameter)cmd.Parameters["?id"]).Value = (uint)(id + i);
                ((IDataParameter)cmd.Parameters["?b1"]).Value = (byte)(b1 * i);
                ((IDataParameter)cmd.Parameters["?b2"]).Value = (byte)(b2 * i);
                ((IDataParameter)cmd.Parameters["?st"]).Value = "This is my string!";

                cmd.ExecuteNonQuery();
            }
        }

        conn.Close();
    }
}
[9 Jul 2008 19:40] Peter van Gulik
If you comment away Prepare() the snipset works as expected. But with Prepare() only the first byte field get's set correctly, the last 2 fields got wrong values, in larger table you can actually see the values in a wrong column.
[9 Jul 2008 19:46] Peter van Gulik
I am running Mysql version 5.1.24.
[10 Jul 2008 21:14] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/49501
[10 Jul 2008 21:15] Reggie Burnett
Fixed in 5.0.10, 5.1.8, and 5.2.3+
[11 Jul 2008 9:25] Peter van Gulik
Thanks for the patch. After examining I think it might be better to scrap the call to GetBytes(), this call casts the byte/sbyte to a short or ushort the you get the first byte in the array. It would be faster to just write the byte and in case of a sbyte cast it to a byte (the hex value will be unchanged while casting from a sbyte to a byte).

This is the new patch.

Modified: branches/5.0/Driver/Source/Types/MySqlByte.cs
===================================================================
--- branches/5.0/Driver/Source/Types/MySqlByte.cs	2008-07-09 16:44:37 UTC (rev 1331)
+++ branches/5.0/Driver/Source/Types/MySqlByte.cs	2008-07-10 21:14:38 UTC (rev 1332)
@@ -82,10 +82,13 @@
 		void IMySqlValue.WriteValue(MySqlStream stream, bool binary, object val, int length)
 		{
 			sbyte v = ((IConvertible)val).ToSByte(null);
-			if (binary)
-				stream.Write(BitConverter.GetBytes(v));
-			else
-				stream.WriteStringNoNull(v.ToString());
+            if (binary)
+                stream.WriteByte((byte)v);
+            else
+                stream.WriteStringNoNull(v.ToString());
 		}
 
 		IMySqlValue IMySqlValue.ReadValue(MySqlStream stream, long length, bool nullVal)

Modified: branches/5.0/Driver/Source/Types/MySqlUByte.cs
===================================================================
--- branches/5.0/Driver/Source/Types/MySqlUByte.cs	2008-07-09 16:44:37 UTC (rev 1331)
+++ branches/5.0/Driver/Source/Types/MySqlUByte.cs	2008-07-10 21:14:38 UTC (rev 1332)
@@ -81,10 +81,13 @@
 		void IMySqlValue.WriteValue(MySqlStream stream, bool binary, object val, int length)
 		{
 			byte v = ((IConvertible)val).ToByte(null);
-			if (binary)
-				stream.Write(BitConverter.GetBytes(v));
-			else
-				stream.WriteStringNoNull(v.ToString());
+            if (binary)
+                stream.WriteByte(v);
+            else
+                stream.WriteStringNoNull(v.ToString());
 		}
 
 		IMySqlValue IMySqlValue.ReadValue(MySqlStream stream, long length, bool nullVal)
[11 Jul 2008 12:12] Tony Bedford
An entry has been added to the 5.0.10, 5.1.8, and 5.2.3 changelogs:

When a prepared insert query is run that contains an UNSIGNED TINYINT in the parameter list, the complete query and data that should be inserted is corrupted and no error is thrown.
[11 Jul 2008 16:35] Reggie Burnett
Peter

It would be great if it were that easy but it isn't.   We can't assume that the user is always handing us byte values.  We want the code to work correctly if the user sets the parameter value to a byte, int, long, ulong, or even a byte[].  By just doing a simple cast to byte you will get cast exceptions if the value is not a true byte value.
[11 Jul 2008 21:41] Peter van Gulik
You already convert it to a byte value by calling the IConvertible interface method ToByte(), by calling GetBytes() the compiler casts your byte to a USHORT or SHORT. I did not assume the value supplied was a byte, if you look at the modified patch code you see only the part within the nested if was changed.

Although the original patch works it means you are just casting a byte to a ushort and back to byte, that does not make any sense (remember that the orignal value was casted to a IConvertible intreface and the value that the method byte v = ((IConvertible)val).ToByte(null)). Summery the original patch casted twice (compiler automatically casts a byte to short or ushort without notice cause bitconverter class does not have a GetBytes() method for a byte value).
[11 Jul 2008 21:43] Peter van Gulik
You tend to look over these details to fast ;).
[11 Jul 2008 22:55] Reggie Burnett
Peter

Thanks for the job performance analysis.  ;)

Sure, sometimes things slip past but that is to be expected when you are a single developer working on a ton of different projects.  

In any event, yes you are right in that the code doesn't make alot of sense.  I'll take care of it.