GamePacketGenerator's sendChatPacket tries to create too large a packet

[dugwyler]

arrayOutOfBounds error still occurs, particularly with chatbot. Oddly, the error is only with the very largest sorts of strings, which, looking over the code, makes not much sense ... Sort of a note to self, unless someone else wants to check it out.

[Maverick]

Hmm got a stacktrace?
I think this is caused because it tries to send the chat packet in one packet and not over multiple packets if necessary.

Probably just needs to send the chat packet using the sendMassiveChunkPacket() method, however sendMassiveChunkPacket() doesn't check if the packet needs to be send over multiple packets at all.

The GamePacketGenerator class probably needs a method to send packets which checks if packets are too big to use sendMassiveChunkPacket() and if not use sendReliableMessage(). All other methods using sendReliableMessage() need to use this new method to prevent problems like this in the future.

[Maverick]

Hmm looking through the code a bit

/**
     * Sends a chat message.
     * @param messageType Type of message being sent -- public, error, etc. (0x00 to 0x09)
     * @param soundCode Number of the sound attached to this message, if any
     * @param userID For message types 0x04 and 0x05, player ID of target
     * @param message Text to send
     */
    public void sendChatPacket( byte messageType, byte soundCode, short userID, String message ){
        if( message.length() > 243 )
                message = message.substring(0, 242);            // (hack) Don't send more than SS can handle

        int            size = message.length() + 6;
        ByteArray      bytearray = new ByteArray( size );

        bytearray.addByte( 0x06 );          // Type byte
        bytearray.addByte( messageType );
        bytearray.addByte( soundCode );
        bytearray.addLittleEndianShort( userID );
        bytearray.addString( message );
        bytearray.addByte( 0x00 );          // Terminator

        composePacket( bytearray );
    }

It shouldn't go haywire with too large strings since it just cuts them down. Also composePacket() should be used instead of sendReliableMessage() I guess.

[dugwyler]

The exception is thrown by ByteArray -- an IndexOutOfBounds exception on the addString method. This suggests that the space allocated is not sufficient to take the entire String ... but, if the String is of length X, wouldn't the space needed always be length + 6? (In other words, why is the formula "length of string + 6" sufficient when length < ~240, but not later? ByteArray is just an array of bytes with methods to add common datatypes onto its tail for easy packet construction. I would have expected an exception somewhere else, but not here ... does the String contain an extra byte or two than the sum of all its characters when it gets over a certain size?)

I guess the out of bounds exception in ByteArray is just not where I would expect this error to be -- I would think that the packet spec would have something to do with it ... the protocol becoming irritated by too large a packet ... and not just our storage medium which in the end has nothing to do with the packet. Confusing!

[milosh]

Is this error still showing up?

[dugwyler]

Think it does from time to time, yes. Nothing was done to fix it as far as I know.

[Maverick]

Indeed. You can reproduce the bug by making a maximum-length size message on the devchat while the chatbots are on.

[milosh]

Going for consistency.

[milosh]

I can't seem to reproduce this bug. I tried both with TWScript and the chatbots but it seems the String is being cut down properly. Due to that observation I'm going to close this ticket, but if you can reproduce the bug please reopen this and post a stack trace.

[dugwyler]

Check the exception logs (a good source for work anyway) -- pretty sure it still comes up from time to time. Don't think anything was changed between now and when it was giving the exception, either.

[Maverick]

When working on the connection logging I noticed that the string for the chat packets were being cut down with a .substring() method. Maybe this has solved it already? (I don't know who put it in though.)

[dugwyler]

I put that hack in a few years back, but it didn't seem to help any from what I could tell. It's strange, because if you look at the chat packet, the size allocation appears correct. Not sure why the array allocation isn't large enough, unless it has something to do with special characters needing more than one byte, trailing mystery characters, or something along those lines.

[milosh]

java.lang.ArrayIndexOutOfBoundsException
	at java.lang.System.arraycopy(Native Method)
	at twcore.core.util.ByteArray.addString(Unknown Source)
	at twcore.core.net.GamePacketGenerator.sendChatPacket(Unknown Source)
	at twcore.core.BotAction.sendChatMessage(Unknown Source)
	at twcore.core.BotAction.sendChatMessage(Unknown Source)
	at twcore.bots.staffbot.staffbot_serverwarningecho.handleEvent(staffbot_serverwarningecho.java:18)
	at twcore.bots.Module.handleEvent(Module.java:62)
	at twcore.bots.ModuleHandler.handleEvent(ModuleHandler.java:170)
	at twcore.bots.staffbot.staffbot.handleEvent(staffbot.java:126)
	at twcore.core.net.GamePacketInterpreter.handleChatMessage(Unknown Source)
	at twcore.core.net.GamePacketInterpreter.translateNormalPacket(Unknown Source)
	at twcore.core.net.GamePacketInterpreter.translateGamePacket(Unknown Source)
	at twcore.core.net.ReliablePacketHandler.handleReliableMessage(Unknown Source)
	at twcore.core.net.GamePacketInterpreter.translateSpecialPacket(Unknown Source)
	at twcore.core.net.GamePacketInterpreter.translateGamePacket(Unknown Source)
	at twcore.core.Session.run(Unknown Source)

[milosh]

Line 242 from ByteArray.java

System.arraycopy( bytearr, 0, m_array, m_pointer, bytearr.length );

void java.lang.System.arraycopy(Object src, int srcPos, Object dest, int destPos, int length)

If any of the following is true, an IndexOutOfBoundsException is thrown and the destination is not modified:

• The srcPos argument is negative.
• The destPos argument is negative.
• The length argument is negative.
• srcPos+length is greater than src.length, the length of the source array.
• destPos+length is greater than dest.length, the length of the destination array.

[Maverick]

milosh, what did you do to reproduce the bug?

If it's a scenario you can always reproduce then it's easy to debug at the line where System.arraycopy is called and figure what is going wrong.

[milosh]

I didn't reproduce it; I just found the stack trace in the log. I assumed that either you or Qan reproduced it; however, it looks to be caused by a server warning echo from staffbot(hard to reproduce).

[milosh]

Found two more of the same above mentioned stack trace and also this one which is slightly off topic but since we're already talking about staffbot...

java.lang.NullPointerException
	at twcore.bots.staffbot.staffbot_savelog.handleEvent(staffbot_savelog.java:112)
	at twcore.bots.Module.handleEvent(Module.java:62)
	at twcore.bots.ModuleHandler.handleEvent(ModuleHandler.java:170)
	at twcore.bots.staffbot.staffbot.handleEvent(staffbot.java:126)
	at twcore.core.net.GamePacketInterpreter.handleChatMessage(Unknown Source)
	at twcore.core.net.GamePacketInterpreter.translateNormalPacket(Unknown Source)
	at twcore.core.net.GamePacketInterpreter.translateGamePacket(Unknown Source)
	at twcore.core.net.ReliablePacketHandler.handleReliableMessage(Unknown Source)
	at twcore.core.net.GamePacketInterpreter.translateSpecialPacket(Unknown Source)
	at twcore.core.net.GamePacketInterpreter.translateGamePacket(Unknown Source)
	at twcore.core.Session.run(Unknown Source)

[Maverick]

That NPE I already fixed, milosh. I just didn't update Staffbot yet.

The NPE was caused on !getautologtime because the TimerTask getLogTask is null (because the logging has been disabled). However, this wasn't being checked before.
I tried this yesterday and figured I still haven't updated Staffbot yet :-P

[milosh]

I'm getting these errors a lot on the new elim bot; however, the messages causing the exception are no where near long enough to be too big. o.O It happens both with an arena message spam and a simple smart private message.