[Bug]: Potential null reference exception

[tysonstrange]

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

• 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)
• This issue is reproducible without changes to the C++ code in this repository
• This bug has not been resolved in master branch
• There is no existing issue for this bug already

Does this bug crash tfs?

yes

Server Version

1.7 (Master)

Operation System

all (listed below)

OS Description

No response

Bug description

Possible getConnection() null reference exception

Possible Pull Requests which are to blame

• [https://github.com/Minimal HTTP login server #4706] Minimal HTTP login server #4706

Steps to reproduce

1. Compile
2. Login
3. Pray

Actual Behavior

Works for the most part, rare race condition could result in a null reference exception and crash the server.

void ProtocolLogin::onRecvFirstMessage(NetworkMessage& msg) checks if the connection exists auto connection = getConnection(); and then schedules a Task for getCharacterList()

PR #4709 for the HTTP Login server adds reference to getConnection()->getIP().to_string() without first checking if getConnection() returns a valid ptr

This line here:

forgottenserver/src/protocollogin.cpp

Line 121 in 1c68163

db.escapeString(getConnection()->getIP().to_string())))) {

Just like in onRecvFirstMessage(), if it's going to reference it, it should check again since the function is scheduled to run later, even if it's within nano seconds.

Expected Behavior

If you're going to use/reference the connection, make sure it exists first like it does from ProtocolLogin::onRecvFirstMessage

auto connection = getConnection();
	if (!connection) {
		return;
	}

https://github.com/otland/forgottenserver/blob/master/src/protocollogin.cpp#L236

[gesior]

IDK, if ProtocolLogin is used on Tibia 12+ at all. It looks like code reference for old clients (server downgrades), but code is still there, so someone can write a script to connect to OTS using that protocol and crash server.

I CAN CONFIRM THIS BUG

This crash is rare on production servers, because it only occurs on long server lags ex. server save,
but it's very easy to reproduce with 10.98 or older protocol, that uses ProtocolLogin (it won't work on 12+, which uses HTTP to login):

• open 2 clients
• login first client into game
• lag OTS for ~20 seconds ex. using !lua debug talkaction ( gesior@32428da ) with script !lua t = os.time() while(os.time() < t + 20) do end
• try to login to account using second client
• close second client
• when lag ends (20 seconds), server will process getCharacterList and crash

In code used by real login protocol (HTTP for 12+ client), it passes IP as string, not read it from connection, so it's safe:

forgottenserver/src/http/login.cpp

Line 36 in 8fe2872

std::pair<status, json::value> tfs::http::handle_login(const json::object& body, std::string_view ip)

@tysonstrange
Thank you for report.
I added fix for that into my 1.4 branch OTC Proxy protocol:
gesior@8988435