MySQL Bugs: #36997: Possible Overflow in ReadLong

Bug #36997	Possible Overflow in ReadLong
Submitted:	27 May 2008 9:35	Modified:	8 Jul 2008 17:03
Reporter:	Christos Pavlides
Status:	Closed
Category:	Connector/Net	Severity:	S2 (Serious)
Version:	5.3.0.0	OS:	Any
Assigned to:		Target Version:
Triage:	D3 (Medium)

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().