SVN 187 Probable Overflow error in PacketWriter.cs

[Ohms_Law]

I'm fairly certain that I just discovered a bug in PacketWriter.cs. I'm no expert on this stuff, by any means, so some peer review would be greatly appreciated.

Regardless, the AccountLoginAck() method of the Server.Network.AccountLoginAck class is:

		public AccountLoginAck( ServerInfo[] info ) : base( 0xA8 )
		{
			this.EnsureCapacity( 6 + (info.Length * 40) );

			thisStream.Write( (byte) 0x5D ); // Unknown

			thisStream.Write( (ushort) info.Length );

			for ( int i = 0; i < info.Length; ++i )
			{
				ServerInfo si = info[i];

				thisStream.Write( (ushort) i );
				thisStream.WriteAsciiFixed( si.Name, 32 );
				thisStream.Write( (byte) si.FullPercent );
				thisStream.Write( (sbyte) si.TimeZone );
				thisStream.Write( (int) Utility.GetAddressValue( si.Address.Address ) );
			}
		}

Note the highlighted line. It's calling m_Stream.Write (I changed all of the "m_"'s to "this", simply because I've always hated hungarian notation with a real passion) with the si.TimeZone variable cast to an sbyte. The problem is that System.MemoryStream.WriteByte takes an unsigned byte as it's parameter. Therefore, when the computer running the server is in a TimeZone with a negative value (I'm currently living in the Pacific time zone, which is -7 right now) it's a problem.

Of course, I turned on arithmetic overflow exceptions in my build, so this shouldn't really be an issue for most people... but, that's why this is in the Core Modifications forum rather than the Server forum.

The best (immediate) solution that I can think of is to use Math.Abs like:

				thisStream.Write( (byte) Math.Abs(si.TimeZone) );

but that's obviously a cludge...

[Jeff]

Originally Posted by Ohms_Law

I'm fairly certain that I just discovered a bug in PacketWriter.cs. I'm no expert on this stuff, by any means, so some peer review would be greatly appreciated.

Regardless, the AccountLoginAck() method of the Server.Network.AccountLoginAck class is:

		public AccountLoginAck( ServerInfo[] info ) : base( 0xA8 )
		{
			this.EnsureCapacity( 6 + (info.Length * 40) );

			thisStream.Write( (byte) 0x5D ); // Unknown

			thisStream.Write( (ushort) info.Length );

			for ( int i = 0; i < info.Length; ++i )
			{
				ServerInfo si = info[i];

				thisStream.Write( (ushort) i );
				thisStream.WriteAsciiFixed( si.Name, 32 );
				thisStream.Write( (byte) si.FullPercent );
				thisStream.Write( (sbyte) si.TimeZone );
				thisStream.Write( (int) Utility.GetAddressValue( si.Address.Address ) );
			}
		}

Note the highlighted line. It's calling m_Stream.Write (I changed all of the "m_"'s to "this", simply because I've always hated hungarian notation with a real passion) with the si.TimeZone variable cast to an sbyte. The problem is that System.MemoryStream.WriteByte takes an unsigned byte as it's parameter. Therefore, when the computer running the server is in a TimeZone with a negative value (I'm currently living in the Pacific time zone, which is -7 right now) it's a problem.

Of course, I turned on arithmetic overflow exceptions in my build, so this shouldn't really be an issue for most people... but, that's why this is in the Core Modifications forum rather than the Server forum.

The best (immediate) solution that I can think of is to use Math.Abs like:

				thisStream.Write( (byte) Math.Abs(si.TimeZone) );

but that's obviously a cludge...

PacketWriter.cs turns this back into a byte, I don't think this is an issue.

		/// <summary>
		/// Writes a 1-byte signed integer value to the underlying stream.
		/// </summary>
		public void Write( sbyte value )
		{
			m_Stream.WriteByte( (byte) value );
		}

[Zippy]

Since the client interprets this as a signed value, simply using Math.Abs yields a different and unintended result.

This discussion is really pointless though since:
1) The client doesn't do anything really useful with this value.
2) IIRC RunUO doesn't auto-supply a value for this, and most servers leave it 0.
3) It only causes a problem when you use non-standard compiler settings. IMO the stupidest "feature" of C# is arithmetic overflow exceptions. Especially in this case.
4) The correct solution would be to use the "unchecked()" syntax to instruct C# to ignore arithmetic exceptions for this piece of code, since it is, indeed, working correctly. But, if the language was sane, you wouldn't need to do that since the value was already explicitly cast to an sbyte.

[Ohms_Law]

All of that is certainly true Zippy, and I have no illusions that I've created this "problem" all by myself... However, I think that I may have a bit of an optimization for you based on this.

In PacketWriter.cs, there are several Write() methods that handle different variables being passed to it. The current method being used is (I'll just post the integer conversion one for an example):

		/// <summary>
		/// Writes a 4-byte signed integer value to the underlying stream.
		/// </summary>
		public void Write( int value )
		{
			m_Buffer[0] = (byte)(value >> 24);
			m_Buffer[1] = (byte)(value >> 16);
			m_Buffer[2] = (byte)(value >>  8);
			m_Buffer[3] = (byte) value;

			m_Stream.Write( m_Buffer, 0, 4 );
		}

what about:

		/// <summary>
		/// Writes a 4-byte unsigned integer value to the underlying stream.
		/// </summary>
		public void Write( uint value )
		{
			m_Buffer = BitConverter.GetBytes(value);

			m_Stream.Write( m_Buffer, 0, 4 );
		}

I haven't actually profiled it or anything, but... I don't think that the BitConverter class does any boxing or unboxing though, which theoretically should make it's use an improvement.

[Zippy]

The bit shifts are done to ensure that the bytes are written in big endian order. BitConverter, AFAIK, is little endian. Thus it is the opposite of what you want.

[Ohms_Law]

Yea, I kind of figured that out last night...


It's been a long time since I've messed around with endian'ness. What a PITA. It took me forever to figure out that that's what the problem was, and I honestly didn't know for sure until I just read your reply. damnit... now I really understand what you were saying about about the overflow check problems.

I'm going to mess around with it some still, just for the heck of it. Thanks for the replies at least, I appreciate it.


edit: in case anyone else is interested (I'm certain that Zippy know this...) here's a little example of the problem that BitConverter causes.

Example program:

using System;
using System.Collections.Generic;
using System.Text;

namespace BitConverter_test
{
    class Program
    {
        static void Main(string[] args)
        {
            int integerTest = 123456;
            byte[] bytearrTest1 = new byte[4];
            byte[] bytearrTest2 = new byte[4];

            bytearrTest1[0] = (byte)(integerTest >> 24);
            bytearrTest1[1] = (byte)(integerTest >> 16);
            bytearrTest1[2] = (byte)(integerTest >> 8);
            bytearrTest1[3] = (byte)integerTest;

            bytearrTest2 = BitConverter.GetBytes(integerTest);

            Console.WriteLine("Byte array 1");
            Console.WriteLine(bytearrTest1[0].ToString());
            Console.WriteLine(bytearrTest1[1].ToString());
            Console.WriteLine(bytearrTest1[2].ToString());
            Console.WriteLine(bytearrTest1[3].ToString());

            Console.WriteLine("Byte Array 2");
            Console.WriteLine(bytearrTest2[0].ToString());
            Console.WriteLine(bytearrTest2[1].ToString());
            Console.WriteLine(bytearrTest2[2].ToString());
            Console.WriteLine(bytearrTest2[3].ToString());

            Console.WriteLine("Integer");
            Console.WriteLine(integerTest.ToString());
            Console.ReadLine();
        }
    }
}

output:2007-07-07_175443.jpg

notice how the two Byte arrays are mirrors of each other.

[Sephur]

All right. I've had the same problem, but I'm not quite as computer-savvy yet. I just want to get my server up and running so that I can focus on things more suited to my level of comprehension.

I'm not horrible with programming, so I can do line-by-line stuff and probably understand it, but big endian, little endian... I'm afraid that means nothing to me. All I know is that it's not writing correctly. I have those places in the core scripts pinpointed with an error log file, if you need to reference them, I can.

Again, any help would be appreciated.

- Sephur

[zenroth]

Disable arithmetic overflow exceptions in your project, or change the code to read something like

for ( int i = 0; i < info.Length; ++i )
{
	ServerInfo si = info[i];
        unchecked
        { 
                thisStream.Write( (ushort) i );
	        thisStream.WriteAsciiFixed( si.Name, 32 );
	        thisStream.Write( (byte) si.FullPercent );
	        thisStream.Write( (sbyte) si.TimeZone );
		thisStream.Write( (int) Utility.GetAddressValue( si.Address.Address ) );
         }
}