Data race on writeToOutputBuffer when called from network thread

[ramon-bernardo]

There is a potential data race when writeToOutputBuffer (and by extension methods like sendStoreBalance) is called directly from the network thread. The output buffer is only safe to access from the dispatcher thread, and calling it from the network thread can lead to race conditions and corrupted msgs.

void ProtocolGame::parseMarketCancelOffer(NetworkMessage& msg)
{
    uint32_t timestamp = msg.get<uint32_t>();
    uint16_t counter = msg.get<uint16_t>();

    g_dispatcher.addTask(
        [=, playerID = player->getID()]() {
            g_game.playerCancelMarketOffer(playerID, timestamp, counter);
        });

    // ❌ This is called from the network thread
    // It writes to the output buffer directly
    sendStoreBalance();
}

g_dispatcher.addTask(
    [=, playerID = player->getID(), self = shared_from_this()]() {
        g_game.playerCancelMarketOffer(playerID, timestamp, counter);
        self->sendStoreBalance(); // ✅ Now, called in dispatcher thread
    });
}

[MillhioreBT]

Interesting, these two methods access unprotected resources:
The Items list which in theory is immutable since there's no direct API to change the state unless you do a /reload items, plus writing to the buffer as you mention.

parseMarketCreateOffer
parseMarketCancelOffer

Another case would be the parsePackets method itself which accesses the global gameState variable. Shouldn't this variable be atomic at least? It's not common to change the state of this variable, but I've seen it can happen if we close the server, or if we open it again, or if the server is cleaning up or simply do it deliberately through the API via scripts.

[kygov]

Maybe use SayiansKing idea and move whole parsePacket method to dispatcher thread (together with resumeWork method so network thread wont modify message buffer untill parsePacket finish). It would avoid future mistakes in parsePacket

https://github.com/otland/forgottenserver/blob/master/src/protocol.cpp#L83
https://github.com/opentibiabr/optimized_forgottenserver/blob/master/src/protocol.cpp#L117

[ramon-bernardo]

@MillhioreBT exactly, there are some methods accessing outputBuffer from outside the dispatcher thread. I'm rewriting the way connection/protocol handling works,and see this issue. There’s no easy fix.

@kygov although it’s a good idea, believe there would be a small overhead, because entire cost of msg.get<T> currently happens on network thread. This would shift that cost to the dispatcher.

I’m not sure how heavy that cost actually is, but it doesn’t look significant. resumeWork shouldn’t be on the dispatcher; we can let the network thread always push tasks into the dispatcher, even if it hasn’t fully finished yet. We already have a packets/s limiter in place.

[kygov]

@MillhioreBT exactly, there are some methods accessing outputBuffer from outside the dispatcher thread. I'm rewriting the way connection/protocol handling works,and see this issue. There’s no easy fix.

@kygov although it’s a good idea, believe there would be a small overhead, because entire cost of msg.get<T> currently happens on network thread. This would shift that cost to the dispatcher.

I’m not sure how heavy that cost actually is, but it doesn’t look significant. resumeWork shouldn’t be on the dispatcher; we can let the network thread always push tasks into the dispatcher, even if it hasn’t fully finished yet. We already have a packets/s limiter in place.

I think there is some cost but its not big enough to worry about. msg is passed by reference so if network thread processes next packet, it can corrupt data while parsePacket on dispatcher is still processing it, its important, but can be solved in different way ofc (copying instead reference?)