MySQL Bugs: #36997: Possible Overflow in ReadLong

Bug #36997	Possible Overflow in ReadLong
Submitted:	27 May 2008 7:35	Modified:	8 Jul 2008 15:03
Reporter:	Christos Pavlides	Email Updates:
Status:	Closed	Impact on me:	None
Category:	Connector / NET	Severity:	S2 (Serious)
Version:	5.3.0.0	OS:	Any
Assigned to:		CPU Architecture:	Any

Description:
The new functions to manually read/parse the data from the buffer are an excellent addition to the connector, but there are some issues relating to casting of numeric types, namely int/long/uint/ulong.
For most numeric value reading the function ReadLong is being used which returns a long:
 public long ReadLong(int numbytes)
        {
            int value = 0;

            int pos = (int)buffer.Position;
            byte[] bits = buffer.GetBuffer();
            int shift = 0;

            for (int i = 0; i < numbytes; i++)
            {
                value |= (bits[pos++] << shift);
                shift += 8;
            }
            buffer.Position += numbytes;
            return value;
        }

I believe the int value = 0; should be long value = 0; to support long data types.

Thank You
I hope the performance enhancements will keep coming

How to repeat:
The easiest way to spot the difference between this version and the previous ones is to take a look at the previous ReadLong function before the changes:
public ulong ReadLong(int numbytes)
		{
			ulong val = 0;
			int raise = 1;
			for (int x = 0; x < numbytes; x++)
			{
				int b = ReadByte();
				val += (ulong)(b * raise);
				raise *= 256;
			}
			return val;
		}
the function returns a ulong and the val is defined as ulong.

Although I have not tested my hypothesis, it should be easily repeated by defining a column in a table a long or even ulong and setting its value to the maximum allowed value. Then use the connector to do any operation on that table using this column that will require reading or writing to this column

Suggested fix:
This function (with the above change) should be ok for longs and int (since the length of the readint prevents a possible overflow) but for ulongs and uints this will probably not work properly.
It might be a good idea to have separate functions for unsigned numeric values or define the ReadLong functions as public ulong ReadLong(int numbytes), define the value as ulong and do some checking to prevent overflows.
A different function might be better since it will be clearer and the checking will not be necessary. 
Finally making these type of unsafe functions public might cause some misuse.

P.S. The same goes for the function WriteInteger(long ....
which should follow the previous idea (either make it ulong or make two different functions to handle singed/unsigned)

Hi Christos and thanks for your report.

Truly, source reveals this change in latest c/NET sources but I can't find why it was introduced (besides speedup). At least, this change should be documented. Workaround would be to build your own driver with proposed change.

Verified as described.

Hi Tonci,
well I think the latest changes that were made to the trunk are a step in the right direction regarding the performance of the connector. Although I have not tested the performance benefit of the changes yet, just by looking at the changes I am sure they will significantly boost performance. 
The issue is not why they were made, but if they are correct in all usages. Doing "unsafe" parsing/writing of any value like this is definetely a proper optimization to do in such performance sensitive operations, but they have to be made very carefully in order to limit the possibilities of serious problems in the future. Also their scope must be as restrictive as possible to make sure they are only used only when absolutely necessary.

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/47112

Fixed in 5.3

We fixed this by renaming the ReadLong method to ReadULong and using ulong values inside the function.  Also we added an assert inside ReadInteger to catch any cases where that method is called with a num bytes > 4.

Thank you Reggie

An entry has been added to the 5.3.0 Changelog:

Possible overflow in MySqlPacket.ReadLong().