FXServer crashes when Mumble Native ‘MUMBLE_SET_PLAYER_MUTED’ is invoked (sporadic)

tens0rfl0w · April 11, 2022, 9:26pm

Environment:

Host: Windows Server 2019 (tested on two separate bare metals)
FXServer Artifact Version: Lasted Build (5473) and Lasted Recommended Build (5402)
Using txAdmin: Yes
Mumble-Server: Integrated (FXServer)
use_fxv2: Tested both on and off
lua54: yes

Description:

We switched to Mumble-Voice today and on our test environment with max. ~20-30 players connected everything went fine before deploying to our live environment. On our live servers however the servers are crashing at around ~180 connected players printing:

=================================================================
	Native Crash Reporting
=================================================================
Got a SIGSEGV while executing native code. This usually indicates
a fatal error in the mono runtime or one of the native libraries 
used by your application.
=================================================================

=================================================================
	Managed Stacktrace:
=================================================================
=================================================================

in the server console.

Additional info:

The full server dump shows a call stack that is related to the server-side player mute functionallity of mumble:

	Name	Lang
	VCRUNTIME140.dll!strstr(const char * str1, const char * str2) Line 226	C
	[Inline Frame] voip-server-mumble.dll!strstr(char * const) Line 520	C++
	voip-server-mumble.dll!Client_set_player_muted(int serverId, bool muted) Line 136	C++
	[Inline Frame] citizen-scripting-core.dll!std::_Func_class<void,fx::ScriptContext &>::operator()(fx::ScriptContext &) Line 968	C++
	citizen-scripting-core.dll!fx::TestScriptHost::InvokeNative(fxNativeContext & context) Line 325	C++
	citizen-scripting-lua54.dll!__Lua_InvokeNative<0>(lua_State * L) Line 636	C++

throwing exception:

Unhandled exception thrown: read access violation.
**str1** was nullptr.

We utilize the ‘MUMBLE_SET_PLAYER_MUTED’ native to allow players to mute themself via inGame hotkeys and to mute players when they are dead, sadly there are no reproduction steps I can provide here as we can’t trigger this on our test env. with ~20-30 players or can somehow trigger this on a certain timing deployed live. Tested on 2 separate Windows Servers with different artifacts, same crash everytime.

Server-side code used to invoke native:

RegisterNetEvent('voice_*****:muteMe', function(mute)
	MumbleSetPlayerMuted(source, mute)
end)

Full Dump

Full dump is too large to upload on here.
Link → 6.12 GB file on MEGA

tens0rfl0w · April 11, 2022, 11:24pm

Seems like this is somehow throwing a nullptr for c->username, no clue why and when this occurs.

github.com

citizenfx/fivem/blob/00a1fb5f1b95b2046113fa713b61612235d79fc2/code/components/voip-server-mumble/src/client.cpp#L136

      
        
            }
            
            
void Client_set_player_muted(int serverId, bool muted)
            {
            	auto convertedServerId = fmt::sprintf("[%d]", serverId);
            	struct dlist *itr, *save;
            	list_iterate_safe(itr, save, &clients)
            	{
            		client_t* c;
            		c = list_get_entry(itr, client_t, node);
            		if (strstr(c->username, convertedServerId.c_str())) {
            			c->mute = muted;
            			break;
            		}
            	}
            }
            
            
void Client_janitor()
            {
            	std::unique_lock<std::recursive_mutex> lock(g_mumbleClientMutex);

nta · April 12, 2022, 7:25pm

This functionality apparently was added by a pull request and I guess I missed this in review:

Will add this to the investigation queue, likely incorrect iteration.

tens0rfl0w · April 12, 2022, 7:53pm

Thank you for the quick response!

Probably the getter function is also affected as it uses the same iteration logic, untested though.

github.com

citizenfx/fivem/blob/d0a2e8659d8ecb8c2356152ae0e102c751ebb022/code/components/voip-server-mumble/src/client.cpp#L113

      
        
            		pollfds[i].fd = c->tcpfd;
            		pollfds[i].events = POLLIN | POLLHUP | POLLERR;
            		if (c->txsize > 0 || c->readBlockedOnWrite) /* Data waiting to be sent? * /
            			pollfds[i].events |= POLLOUT;
            		i++;
            	}
            	return i;*/
            	return 0;
            }
            
            
bool Client_is_player_muted(int serverId)
            {
            	auto convertedServerId = fmt::sprintf("[%d]", serverId);
            	struct dlist *itr, *save;
            	list_iterate_safe(itr, save, &clients)
            	{
            		client_t* c;
            		c = list_get_entry(itr, client_t, node);
            		if (strstr(c->username, convertedServerId.c_str()))
            		{
            			return c->mute;

(was also added in the PR you mentioned)

AvarianKnight · April 12, 2022, 8:07pm

Could this be related to not locking g_mumbleClientMutex

EDIT:

Consistent reproduction for the crash

function mute(source)
    MumbleSetPlayerMuted(source, not MumbleIsPlayerMuted(source))
end

CreateThread(function()
    while true do
        for i = 1, 100000 do
            for source, _ in pairs(GetPlayers()) do
                mute(source)
            end
        end
        Wait(0)
    end
end)

Run this on the server before you start your client, after fully connecting the server will crash

AvarianKnight · April 12, 2022, 9:42pm

Looks like c->username can be null right on client creation which would cause the server to crash if you hit this perfectly

tens0rfl0w · April 14, 2022, 10:06am

Fixed with