Bug when trying to send a really large single id packet

[HeavenIsLost]

Due the max network msg size(24590 bytes) we can't send a big single one packet to the client(a packet larger than 24566) like the store offers message(this packet got too much strings, its why if you try to send a lot of offers it will easily reach 35000~ bytes)

@ 2dfcb49 wont fix that because this issue only happens when you try to send a big large packet with only one message inside. You can't even write this big packet to the NetworkMessage istance because of the 24590 limit and the network code would split the packet and then the client debugs/crash.

TibiaClient/OTclient already got it fixed, there is a need to write the whole packet size in the message header and then send the packet split. Current networking code does not support that.

The only current fix to this issue is to resize this variable on const.h: #define NETWORKMESSAGE_MAXSIZE 35590

The max limit is 65535 bytes. The bad side of this is each NetworkMessage/OutputMessage will need more memory and the allocation/deallocation time may be bigger.

@marksamman @djarek

[marksamman]

The max size should be raised if the clients support it, I'm not worried about larger heap allocations, we have a pool of pre-allocated output messages anyway. I'm more concerned about hitting stack size limits because NetworkMessages are placed on the stack in ProtocolGame send functions.

[HeavenIsLost]

I was ware of the fact about the NetworkMessages problem(because they are placed on the stack). The client does support larger packets size than 24590(i have test with it with 35000~ bytes with 10.93 tibia client, cipsoft is already using big packets).

Can we just increase OutputMessage buffer size and let the NetworkMessage buffer with 24590 bytes size?

[HeavenIsLost]

@marksamman another fact i want to add is we are using a NetworkMessage on protocolgame.cpp that uses 24590 bytes of the stack. On linux with the command ulimit -a i can see "stack size (kbytes, -s) 8192" so do we really got a problem raizing the NETWORKMESSAGE_MAXSIZE?

I got a doubt if 8192 is 8192 kbs or 8192 bytes, if its bytes then it should be raised in limits config file.

[marksamman]

No, OutputMessage is a subclass of NetworkMessage and packets are constructed from NetworkMessages, so if a NetworkMessage can't hold more than 24590 bytes then you can't construct a larger packet anyway.

On Linux the default stack size is 8MB, but there are other platforms with significantly smaller stack sizes (64KB on OpenBSD). It's possible to set a stack size in the linker, but that may only be enough to bypass the soft limit and I don't want to risk introducing security vulnerabilities when running TFS on any platform, so I rather just use a heap allocated buffer. I think we can store the buffer in the Protocol (or ProtocolGame if we don't need it in other protocols) class so that we don't have to re-allocate it every time we need to construct a packet, we just re-use the same buffer.

[HeavenIsLost]

Add something like to protocolgame.h:

NetworkMessage& getNetworkMessage() {
    networkMessage.reset();
    return networkMessage;
}

NetworkMessage networkMessage;

then inside each protocolgame.cpp use of NetworkMessage replace with:

NetworkMessage& msg = getNetworkMessage();

[HeavenIsLost]

@marksamman should i do a pull request with that code i have post above?

[marksamman]

If you've also changed the NetworkMessage class to allocate the buffer on the heap, yes.

[HeavenIsLost]

`NetworkMessage() { reset(); buffer = new uint8_t[NETWORKMESSAGE_MAXSIZE]; }

~NetworkMessage() { delete [] buffer; }`

when i got the time i will do the pull request. Last question, how much we should raise NETWORKMESSAGE_MAXSIZE?

[marksamman]

If the clients can handle packets of size 65535, then raise it to that and we can dynamically grow the buffer in NetworkMessage when it's needed. The default size should be 16383, and it should grow by 8192 at a time up until 65535. Growing the buffer should be handled by canAdd. You can check the implementation of PropWriteStream to see how it should be done.

[djarek]

I suggest using std::vector for the internal buffer - it's much easier to manage resizing that way. Another thing to consider is that OutputMessages are "torn down" (the default destructor is called, which does nothing right now) every time the ref count goes to 0 and their memory is put back to the allocator pool. Without changing the allocator, this MIGHT put considerable pressure on the default memory allocator. For more details see: https://github.com/otland/forgottenserver/blob/master/src/lockfree.h

[HeavenIsLost]

@djarek well its beyond my knowledge :(

[Klonoa88]

Is this fixed yet?