[Bug]: NetworkMessage always increase size even when it's not needed

[ArturKnopik]

By submitting this bug issue, you agree to the following.

• [X] This is a bug in the software that resides in this repository, and not a support matter (use https://otland.net/forums/support.16/ for support)
• [X] This issue is reproducible without changes to the C++ code in this repository
• [X] This bug has not been resolved in master branch
• [X] There is no existing issue for this bug already

Does this bug crash tfs?

no

Server Version

1.7 (Master)

Operation System

all (listed below)

OS Description

No response

Bug description

Adding data to network message always increase it size even if it's not required

Possible Pull Requests which are to blame

N/A

Steps to reproduce

put this code to lua script and execute

local msg = NetworkMessage()
msg:addByte(1)
msg:addByte(2)
msg:addByte(0)
msg:addByte(4)
print(msg:len()) -- will return 4
msg:seek(1) -- set write position
msg:addByte(2) -- already used field
print(msg:len()) -- will return 5

@EDIT updated returned len() values to valid ones

Actual Behavior

Message size increases when not required

Expected Behavior

Message size increases only when required

Backtrace

N/A

[gesior]

local msg = NetworkMessage()
msg:addByte(1)
msg:addByte(2)
msg:addByte(0) -- will be filled in the future
msg:addByte(4)
print(msg:len()) -- will return 3

IDK what you mean. If it returns 3 in print, it means it's bugged. It should return 4. You put 4 bytes into packet, it should return 4. msg:addByte(0) -- will be filled in the future Adding 0 byte does not mean it's not adding byte, it's binary format, every byte matters.

Ignore networkMessage:addByte, read networkMessage:addU32(number) code. If you add 1 as U32 "number", it will add 3 bytes 0 and 1 byte with 1 as it's 32 byte number (31 bytes with 0 and 1 byte with 1 at end). If you need any information about binary communication between server-client/client-server, message me on Discord: gesior.pl

[ArturKnopik]

void addByte(uint8_t value)
{
	if (!canAdd(1)) {
		return;
	}

	buffer[info.position++] = value;
	info.length++;  << why we increment this one when we should not to do this because we are not incrementing network msg size but changes some previously set bytes (msg:addByte(4))
}

We creating NetworkMessage with length of 10, let's set bufferPos to 0, add byte, add byte should override first byte in message but not increment network message size, for now override first byte and increment network size without reasons

IDK what you mean. If it returns 3 in print, it means it's bugged. It should return 4. You put 4 bytes into packet, it should return 4. msg:addByte(0) -- will be filled in the future Adding 0 byte does not mean it's not adding byte, it's binary format, every byte matters.

sorry, copied wrong example, it's for first len() 4 and another len() will return 5, even seek() pos was invalid :p updated issue example

[gesior]

We creating NetworkMessage with length of 10, let's set bufferPos to 0, add byte, add byte should override first byte in message but not increment network message size, for now override first byte and increment network size without reasons

Now I get it (after example update). seek moves to some byte in NetworkMessage, but functions that add bytes to NetworkMessage do not check, if they are currently on last byte of message or if they are overwriting bytes after seek.

I think it can be fixed by removing seek function. It's not used anywhere in C++/Lua code of TFS and it should not be used by anyone, as we use binary protocol in Tibia (read "stream" with "getX").

To fix this and keep seek function in engine, we would need to add IF to every byte added to every NetworkMessage. It would use some CPU (too much CPU!) just to check something, that is not used by any OTS code. I think it should be removed from OTS. If we ever get Tibia protocol that can't be read as "stream" (with getX), we can add it again, but Tibia did not use such protocol for 6.4-14.11 protocols (all I know), so I don't expect them to add it now.

[ArturKnopik]

Agreed! this code is critical nd should be fast!

[nekiro]

I would say the same. If it's not used let's keep it simple and get rid of it.