Voice Targets not applied when channel doesn't exist

[niekschoemaker]

What happened?

When a channel doesn't exist on a mumble server, the call to MUMBLE_ADD_VOICE_TARGET_CHANNEL is ignored (see MumbleClient.cpp#L356).

This causes people not to be able to hear eachother, since the target doesn't get added.

Expected result

The target should be added or there should be a way to validate the result of the target update.

Reproduction steps

1. Add a channel to a voice target.
2. Set channel of other person to the channel just added.
3. Observe that voice target doesn't work.

Importancy

Slight inconvenience

Area(s)

FXServer

Specific version(s)

Server 9073 linux/windows

Additional information

For some context, I internally cache which voice targets I have added, to avoid network overhead, this works fine, except for the case given above. I did think of some possible solutions, which I listed below. I'm willing to implement them myself, I'm just not sure which one would be the "most correct" one.

Possible solution:

1. Add a native to check if a channel exists, this way it can be assured that the channel exists.
2. Another "solution" would be to simply constantly Call MUMBLE_ADD_VOICE_TARGET_CHANNEL, this however would cause a lot of packets to be sent to the server for no reason, and would probably hit the "MAX_VOICE_TARGET_CHANNELS" limit on the mumble server which is 256, since there's no check if the target is already added anywhere as far a I could tell.
3. Don't clear m_pendingVoiceTargetUpdates, but remove each one only if they are actually handled. This however adds the possibility that a voice target is added after it is no longer needed, causing it to get stuck in the voice targets unless manually cleared.
4. Only add the voicetarget update to m_pendingVoiceTargetUpdates if the channel exists and return a boolean from the native, this would allow a plugin to validate if the target is actually going to be added.
   5. Looking at all the code, the check if a channel actually exists doesn't actually seem necessary for correct behavior (see client.cpp#L1011 so removing the check would also fix the issue.

I think option 1 or 4 would be the easiest to implement, but would require some testing to see if there's any implications to this.

[AvarianKnight]

When a channel doesn't exist on a mumble server, the call to MUMBLE_ADD_VOICE_TARGET_CHANNEL is ignored (see MumbleClient.cpp#L356).

This causes people not to be able to hear eachother, since the target doesn't get added.

This seems like intended functionality, if you're expecting other functionality maybe try the BY_SERVER_ID variant

Another "solution" would be to simply constantly Call MUMBLE_ADD_VOICE_TARGET_CHANNEL, this however would cause a lot of packets to be sent to the server for no reason, and would probably hit the "MAX_VOICE_TARGET_CHANNELS" limit on the mumble server which is 256, since there's no check if the target is already added anywhere as far a I could tell.

This is the solution I use in pma-voice currently I eventually ™️ plan to swap to the ByServerId to get around some of the aforementioned issues here, relying on channels in the first place just seems like a bad move. They're pretty inflexible and look to have bad perf issues on higher player counts.

https://github.com/AvarianKnight/pma-voice/blob/b0be960b15811bf817c1461de2884bb7d46c798e/client/init/proximity.lua#L31-L40

Looking at all the code, the check if a channel actually exists doesn't actually seem necessary for correct behavior (see client.cpp#L1011 so removing the check would also fix the issue.

I'm curious on how this is not necessary?

fivem/code/components/voip-server-mumble/src/client.cpp

Lines 1013 to 1018 in e8e1554

	targeted_channels.emplace(ch->id);
	list_iterate(itr, &ch->clients) {
	client_t *c;
	c = list_get_entry(itr, client_t, chan_node);
	listeningUsers.insert(c);
	}

Would this not be a null pointer deref?

[niekschoemaker]

When a channel doesn't exist on a mumble server, the call to MUMBLE_ADD_VOICE_TARGET_CHANNEL is ignored (see MumbleClient.cpp#L356).

This causes people not to be able to hear eachother, since the target doesn't get added.

This seems like intended functionality, if you're expecting other functionality maybe try the BY_SERVER_ID variant

The specific problem I have here is that I have a grid system, in which I add the current grid you're in to the voice target and the neighboring grids, but it's not guaranteed that there's any players in that grid, so the channel might not exist.

This then causes the update to be ignored and the plugin itself thinking it already added the channel so not re-adding it anymore.

And both solutions in this case (either re-adding it causing duplicates which causes unecessarry bandwidth, or removing and re-adding) are not the cleanest solutions.

Another "solution" would be to simply constantly Call MUMBLE_ADD_VOICE_TARGET_CHANNEL, this however would cause a lot of packets to be sent to the server for no reason, and would probably hit the "MAX_VOICE_TARGET_CHANNELS" limit on the mumble server which is 256, since there's no check if the target is already added anywhere as far a I could tell.

This is the solution I use in pma-voice currently I eventually ™️ plan to swap to the ByServerId to get around some of the aforementioned issues here, relying on channels in the first place just seems like a bad move. They're pretty inflexible and look to have bad perf issues on higher player counts.

I use the channels only for the grid system, since this requires far less voice target updates and in my experience this increased performance quite a bit over only using player based targets. And speaking of player count, used it succesfully for around 700 players so appears to be working fine that way.

https://github.com/AvarianKnight/pma-voice/blob/b0be960b15811bf817c1461de2884bb7d46c798e/client/init/proximity.lua#L31-L40

Looking at all the code, the check if a channel actually exists doesn't actually seem necessary for correct behavior (see client.cpp#L1011 so removing the check would also fix the issue.

I'm curious on how this is not necessary?

fivem/code/components/voip-server-mumble/src/client.cpp

Lines 1013 to 1018 in e8e1554

	targeted_channels.emplace(ch->id);
	list_iterate(itr, &ch->clients) {
	client_t *c;
	c = list_get_entry(itr, client_t, chan_node);
	listeningUsers.insert(c);
	}

Would this not be a null pointer deref?

Not as far as I can tell, it would already cause a null pointer deref anyways if that was the case since channels are all temporary so get deleted when no one is in them.

fivem/code/components/voip-server-mumble/src/client.cpp

Lines 1010 to 1012 in e8e1554

	ch = Chan_fromId(vt->channels[i].channel);
	if (ch == NULL)
	continue;

And as can be seen here it does a null check right there.

A solution I did figure out is simply removing the target (either channel or player) and then re-adding it again, since this doesn't cause any side effects since updates are only sent every 500 ms and are batched, so updating the same voice target twice only causes one update, in which it sends the entire voice target data.

[AvarianKnight]

This then causes the update to be ignored and the plugin itself thinking it already added the channel so not re-adding it anymore.

And both solutions in this case (either re-adding it causing duplicates which causes unnecessary bandwidth, or removing and re-adding) are not the cleanest solutions.

Constantly listening to channels when you don't need to uses a lot more unnecessary bandwidth than re-sending voice target requests, by an order of magnitude. This is the primary reason that pma-voice swapped from grid -> targeting system.

See:

Not as far as I can tell, it would already cause a null pointer deref anyways if that was the case since channels are all temporary so get deleted when no one is in them.

I'm very confused here, in your original post you said

client.cpp#L1011 so removing the check would also fix the issue.

But this gets used on the very next line to add to targeted_channels, which would be a null pointer deref and crash.

With the current way that it is done this will not cause a null pointer deref because its checked, but you're saying to remove this check to fix this issue

[niekschoemaker]

This then causes the update to be ignored and the plugin itself thinking it already added the channel so not re-adding it anymore.

And both solutions in this case (either re-adding it causing duplicates which causes unnecessary bandwidth, or removing and re-adding) are not the cleanest solutions.

Constantly listening to channels when you don't need to uses a lot more unnecessary bandwidth than re-sending voice target requests, by an order of magnitude. This is the primary reason that pma-voice swapped from grid -> targeting system.

See:

Not as far as I can tell, it would already cause a null pointer deref anyways if that was the case since channels are all temporary so get deleted when no one is in them.

I'm very confused here, in your original post you said

client.cpp#L1011 so removing the check would also fix the issue.

But this gets used on the very next line to add to targeted_channels, which would be a null pointer deref and crash.

With the current way that it is done this will not cause a null pointer deref because its checked, but you're saying to remove this check to fix this issue

Yes, I stand corrected at the removing the check, I got the channel ID parameter completely wrong.
So I'll remove that one as an option to begin with.