Newly connected empty clients don't show up on old clients

[ann0see]

Communication started on 24 Jul 2022

This is an excerpt of an E-Mail conversation
 
Since it might be a privacy issue, I'm reporting it per Mail. 
 
Filler for: #2754
 
Description of bug:
 
If two users with an empty name join a server, the first user does not
see the join of the second user on the mixer board. This might be a
privacy issue.
 
*Expected behavior:
 
Both users should see the user without name on their mixer board
 
Screenshots:
 
C1 is left, C2 is right
 

 
To reproduce:
 

• Start a client (c1) (with new ini file) and set name to the empty string in Settings (and everyting else also to none)
• Join a server
• Start another client (c2) with empty name
• Join the same server
• Observe that c1 doesn't see that c2 joined, but c2 sees that c1 is there

As soon as we e.g. set an instrument in c2, it shows up in c1

Operating System:
Debian 11

Additional context:

Observed in: #2738

[ann0see]

@softins can you help out with some potentially network (server) related?

[softins]

@softins can you help out with some potentially network (server) related?

Not until next week. I'm away on holiday until Saturday. You could send details through in the meantime if you want.

[ann0see]

Ok. Great no problem. You should also have received my E-Mail with the logs and a issue description. hoffie could not reproduce the problem, I could on multiple OS.

[ann0see]

@pljones Softins has a proposed fix (sent via mail). Could you please look over it too (if you’re ok with it?

[pljones]

Not received... and not marked as spam.

[ann0see]

Ok. Will forward it

[ann0see]

Excerpt: [Privacy issue]: Newly connected empty clients don't show up on old clients

Please note that the conversation needs to be read from the bottom to the top!

---

pljones (DC/Matrix)

My only concern is why this hasn't had a very noticeable effect. Why is it an edge case? I think we need to have a fully documented scenario saying how to reproduce the behaviour and why this change must (logically) be the fix. The code, as code, looks okay. The code, as code, before the change, looked okay, too.

As of 3.8.0 we don't have a connection until the client info comes in. So the Reset happens on disconnect. If no connect happens (with an emit now always), the channel should just sit empty unused.
So it would be a last gasp "the last change that happened here was I got reset" emit

I'd rather the emit happened and wasn't needed than it didn't happen and was needed...
I'm not sure what would be listening for the signal, though.
Well the emphasis on low latency isn't

Some chat on Matrix/Discord with pljones followed. Excerpts are following

ann0see (Matrix/Discord)
I'd like to merge the fix by would you be ok with that?

softins

Thanks !

All, I have pushed my fix to my own repo at https://github.com/softins/jamulus/tree/fix-new-empty-chaninfo if anyone wants to try it.

Let me know when I should raise a PR (although after today, I'm probably not free again until Sunday).

Cheers

(Cc direct to , since he might not be receiving the team emails?)

hoffie

Thanks for spotting and fixing this, and ! The proposed fix sounds good to me.
The privacy-specific severity sounds little enough for me to go the standard PR way and do a release sometime soon (even if it is some weeks from now, but I'd be fine with an immediate release in a few days as well).

<...>

ann0see:

Yes, that looks like a possible fix. I’d like to have Peter‘s OK here too (and maybe do a local test) then you can open a PR and Admin merge it.
Afterwards we should update the issue, give a short summary and publish a nightly.

softins:

I have committed the fix to my local repo, but not yet pushed to my github repo:

<redacted>:~/jamulus $ git show
commit 369f1091754842d91cd88712436af44be1288a7d (HEAD -> fix-new-empty-chaninfo)
Author:  <redacted>
Date:   Tue Aug 2 21:20:25 2022 +0100
    Fix new channel with no info not being displayed.
    A new connection must always cause a channel update signal to be emitted,
    even if the channel info for the new channel is empty.
    Fixes #2754
diff --git a/src/channel.cpp b/src/channel.cpp
index b078348a..c6df1f39 100644
--- a/src/channel.cpp
+++ b/src/channel.cpp
@@ -334,11 +334,11 @@ float CChannel::GetPan ( const int iChanID )
void CChannel::SetChanInfo ( const CChannelCoreInfo& NChanInf )
{
-    bIsIdentified = true; // Indicate we have received channel info
-
-    // apply value (if different from previous one)
-    if ( ChannelInfo != NChanInf )
+    // apply value (if a new channel or different from previous one)
+    if ( !bIsIdentified || ChannelInfo != NChanInf )
     {
+        bIsIdentified = true; // Indicate we have received channel info
+
         ChannelInfo = NChanInf;
         // fire message that the channel info has changed
<redacted>:~/jamulus $

Please let me know how best to proceed.

softins

My new idea is much simpler, does not need any magic in CChannelCoreInfo, and addresses the problem right where it occurs.
Using bIsIdentified, we can tell that this is a new channel, and can force the update and emitting of the signal even if the value of ChannelInfo is unchanged. Here is the new version of CChannel::SetChanInfo:

void CChannel::SetChanInfo ( const CChannelCoreInfo& NChanInf )
{
    // apply value (if a new channel or different from previous one)
    if ( !bIsIdentified || ChannelInfo != NChanInf )
    {
        bIsIdentified = true; // Indicate we have received channel info
        ChannelInfo = NChanInf;
        // fire message that the channel info has changed
        emit ChanInfoHasChanged();
    }
}

I have built a server with this change, and unnamed clients are now all displayed as they should be.
Cheers

softins

OK, I have a better idea, which I will test first and then describe.
Personally, I don’t think it is severe, but rather niche. It should be fixed though, for 3.9.1
Cheers
softins

ann0see

Hi ,
After thinking some more, strName == "" isn't clean either. I sometimes saw empty client names in the wild (although that's not good practice). What would that mean? Could it be abused in any way (e.g. to send a lot of unneeded messages if a client with empty name is connected)? 
Also, we should agree on how severe this is. Currently not much changed on master, and a bug fix release should be fast.
Yours,

softins

Just to follow this up, it probably makes most sense to specify that an empty name always compares not equal, since this is the least likely to happen in practice. Also, we would need to test both operands, so that the != operator was commutative:

    bool operator!= ( const CChannelCoreInfo& CompChanInfo )
    {
        return ( ( strName == "" ) || ( CompChanInfo.strName == "" ) || ( CompChanInfo.strName != strName ) ||
                 ( CompChanInfo.eCountry != eCountry ) || ( CompChanInfo.strCity != strCity ) || ( CompChanInfo.iInstrument != iInstrument ) ||
                 ( CompChanInfo.eSkillLevel != eSkillLevel ) );
    }

I’ll test this change and if the team are happy with the approach, I can raise a PR.
 
Cheers

 
softins

 
OK, I believe I now understand this issue, and there are two alternative ways to fix it.
 
The commit that stopped it working is 726cc6fa, specifically the removal of the compatibility call to CreateAndSendChanListForAllConChannels() on line 594 of the previous version. As Volker indicated, that call should not be necessary. The intention is that when a client connects, in CServer::OnNewConnection() there is a call to vecChannels[iChID].CreateReqChanInfoMes(), which tells the new client to send its channel info to the server. When the server receives this, in CChannel::SetChanInfo(), it emits ChanInfoHasChanged(), which itself is connected to CreateAndSendChanListForAllConChannels() and therefore updates all the other connected clients.
 
However, in CChannel::SetChanInfo() there is an optimisation that only emits the signal if the new channel info differs from the old:

void CChannel::SetChanInfo ( const CChannelCoreInfo& NChanInf )
{
    bIsIdentified = true; // Indicate we have received channel info
 
    // apply value (if different from previous one)
    if ( ChannelInfo != NChanInf )
    {
        ChannelInfo = NChanInf;
 
        // fire message that the channel info has changed
        emit ChanInfoHasChanged();
    }
}

The variable CChannel::ChannelInfo is of type CChannelCoreInfo, which is defined in src/util.h, and initialised according to the constructor there.
 
The problem is that if the client has no name and no language, the channel info it sends is the same as the newly-initialised ChannelInfo, so the comparison fails and the signal is not emitted.
 
So there are two ways it can be fixed:
 
1.       Remove the comparison between NChanInf and ChannelInfo, and always emit the signal anyway.
2.       In src/util.h, class CChannelCoreInfo, either:
a.       initialise a new instance with some value that can never be sent by a client, so that operator!= would always succeed on a newly-initialised instance. It would be sufficient to make only one of the fields initilaised to a value that would never match. However, it would be necessary to ensure such an initialised value was never used as a protocol source. Or alternatively:
b.      a newly-initialised instance would always compare not equal to any other instance. Only one of the members needs to be tested to ensure this.
 
Maybe 2.b would be the most compatible way to go, so that operator!= would become:

    // compare operator
    bool operator!= ( const CChannelCoreInfo& CompChanInfo )
    {
        return ( ( eSkillLevel == SL_NOT_SET ) ||  ( CompChanInfo.strName != strName ) || ( CompChanInfo.eCountry != eCountry ) || ( CompChanInfo.strCity != strCity ) ||
                 ( CompChanInfo.iInstrument != iInstrument ) || ( CompChanInfo.eSkillLevel != eSkillLevel ) );
    }

The test ( eSkillLevel == SL_NOT_SET ) could just as easily be one of the other members, depending on which is considered most appropriate:
 
-          ( strName == "" )
-          ( eCountry == QLocale::AnyCountry )
-          ( strCity == "" )
-          ( iInstrument == CInstPictures::GetNotUsedInstrument() )
 
Only one of these is needed to ensure a newly-initialised CChannelCoreInfo always compares not equal to any other instance.
 
Comments?
 
Cheers

 
softins
 
OK, I’ve finished a git bisect. This is what it says:

726cc6facfc8f075d084684d3aaffa589f257fd7 is the first bad commit
commit 726cc6facfc8f075d084684d3aaffa589f257fd7
Author: Volker Fischer <corrados@users.noreply.github.com>
Date:   Tue May 26 15:43:00 2020 +0200
 
    clean up -> COMPATIBILITY ISSUE can be safely removed, ResetTimeOutCounter is not necessary since it is already done in the channel
 
src/server.cpp | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)

so won’t be able to look at it till tomorrow. If anyone else wants to have a look, feel free!
 
Cheers

 
softins
 
I’ve found that server 3.5.4 does not display the issue, but server 3.5.5 does display it. Now to narrow it down further.
 
softins
 
I’ve been having an initial look at this issue this afternoon.
 
I suppose it could conceivably be a privacy issue, but would consider it more just a long-standing bug.
 
Here’s what I have found:
 
On my Pi, I initially tested with the 3.8.2 release. I opened three shell windows and gave the following commands, one in each:

Jamulus -s -n 
Jamulus -i /dev/null
Jamulus -i /dev/null

I cleared out the default “No Name” from the client name, but initially didn’t change the country from the default United States. I could not reproduce the problem. Both clients showed both channels. I then changed the country from United States to None, and could then reproduce the problem:
 
Client 1 connects and sees its channel.
Client 2 connects and sees both channels, but client 1 does not see the client 2 channel.
 
After disconnecting, I set a country on one of the clients only and repeated the test:
 
Connecting the client with no country first, it sees itself as before. Then connecting the client with a country, both clients display both channels.
Connecting the client with a country first, it sees itself as expected. Then connecting the client with no country, it sees both channels, but the first client does not see the second appear.
 
I then tried the clients with no country but a default “No Name”. The problem did not occur.
 
So it is when a client with no country and no name connects, it is visible to itself, but not to the other client.
 
I then tried connecting to my private AWS server, also running 3.8.2, and the same occurred. So it was nothing to do with the clients and server being on the same machine.
 
I then looked for a public server running an earlier version, and tried “feeels.JAM.world” in Munich, running 3.6.2, still using my 3.8.2 clients. The same wrong behaviour was observed.
 
I then compiled 3.6.2 and tested with clients of that version. The same was observed again.
 
So I conclude this is a long-standing bug that has existed since at least 3.6.2 and probably earlier versions, and is related to the version of server, not client.
 
I then repeated the test using 3.8.2 clients again, connecting to the server “PhilJam”, which is on 3.5.3git, and it did NOT show the problem: both clients with empty names and countries showed both channels. Interesting.
 
Then I tried “CMJamServer” on 3.5.11git. It displayed the problem.
 
Next, “Czechia262” on 3.5.6git. It displayed the problem.
 
Next, “XavsJamulusServer” on 3.5.5git also displayed the problem.
 
Next, “Anders”, running 3.5.3git. Both channels displayed correctly.
 
So it seems to have been introduced somewhere between 3.5.2 and 3.5.5 of the server. I’ll try building those early versions of server locally to try and narrow it down to some change between them.
 
Cheers

 
ann0see
 
I‘m answering that based on another test I made:
 

 Does it still happen as described if the two clients are on different machines?
 How does it behave if there was already a named client connected to the server first?
How does it behave if, while in the anomalous state described, a named client then connects?
 
I haven’t tested these questions specif However, if I connect a client with empty name to a server with a jam going on, before anyone else joins, I can’t observe that anyone muted me.

As soon as I request the welcome message via explorer or someone joins, I see the muted symbol. Therefore I would conclude that the empty client only shows up as soon as the client list gets resent (I assume it doesn’t get sent on the join of an empty client although it gets sound).
 

Does it happen in the same way with different versions of server software? There are plenty of old versions of public server that could be tried, visible in Explorer.

Not yet tested, I assume it’s been like this for many versions. 
 
Yours,
 

softins

Hi all,

As I said on the placeholder issue, I can’t look properly <...>. But I had a few initial thoughts:

• Does it still happen as described if the two clients are on different machines?

• Does it happen in the same way with different versions of server software? There are plenty of old versions of public server that could be tried, visible in Explorer.

• How does it behave if there was already a named client connected to the server first?

• How does it behave if, while in the anomalous state described, a named client then connects?

Cheers

[ann0see]

Re-opened as there's still post discussion needed. @pljones ?

[pljones]

Also adding the following here as it's pertinent:

<IP source addr/dest addr> {
  <UDP source port/dest port> {
    <Jamulus protocol message> {
      <Audio frame> {
        If IP source/UDP source not in active channels {
          activate a new channel
          emit channel list changed // does this happen?
        }
        ... continue with audio frame processing ...
      }
      <Profile Info message> {
        Find IP source/UDP source in active channels (and, yes, drop the message if it's not there as we require audio to create the channel: I think this happens for all CM messages, before they're emitted by the protocol handler)
        If details have changed {
          update details
          emit channel list changed
        }
      }
      // *and* emit channel list changed when deactivating the channel on audio frame "time out" -- does this happen?
    }
  }
}

What this fix does is somehow avoid emit channel list changed when a previously unknown client appears (i.e. when its first audio frame is received and the channel has been created) by making that "create" appear to be an "update" to an existing channel. This seems ... unclean and not addressing the root cause of the problem.

Based on "standard" operation "create"/"update"/"read"/"delete", we want to trigger sending the full channel list whenever any operation on a single channel happens, other than "read". What this change has done is overload "update" by adding an extra field (bIsIdentified) and forcing that to be updated. Whilst it's reliable, it does change the data model (adding a new value just to work around a bug).

[pljones]

Re-opened as there's still post discussion needed. @pljones ?

I'm only unhappy because, whilst it might work, there are high level design issues...

[softins]

I'm only unhappy because, whilst it might work, there are high level design issues...

That may or may not be the case. The PR #2774 fixing the current issue is not intended to make any design changes; it is just making sure the existing design continues to work under one particular edge case.

To outline the current design as intended by Volker:

1. When a connected client makes a change to its profile info, it sends a Channel Info message to the server.
2. When the server receives this channel info message, it checks whether the supplied info differs from the existing info it holds for that particular client. If it is different, it emits a “Channel Info Changed” signal. If it did not differ, it does not emit the signal, as there is no change to inform other clients of.
3. The “Channel Info Changed” signal is connected to a function that sends the complete list of connected clients to every connected client.
4. When a new client connects, the server sends a message to that client requesting that it send its client info to the server.
5. When the client receives that request, it responds with a channel info message as in 1 above. This causes step 2 above to be executed, and that is how all the other clients get updated to display the new client in their mixer board.

Now, in 99.999% of cases, the above operations work correctly. After 3.9.0 was released, it was discovered that if a client had been configured with an empty name and a country of “None” in its profile (and ONLY in this case), the initial channel info it sends in step 5 failed the “info has changed” check in step 2 above, because it was identical to a newly-initialised ChannelInfo structure. Consequently, the other connected clients did not get informed about the new channel list.

So the PR uses the existing bIsIdentified flag (which is initialised to false for a new channel) to notice when the channel info message from the client is the FIRST channel info received from the new client, and ensures the signal is emitted even if that info if identical to the newly-initialised ChannelInfo object.

That means there is no edge case, and the current design above now works correctly in all cases. The other clients are all informed of the new channel list.

If the underlying design can be improved, that should be the subject of separate discussion and does not negate the value and necessity of the current fix at this moment.

The reason the behaviour was not observed when connecting to a server older than 3.5.5 was because at 3.5.5 (commit 726cc6f) a backward-compatibility call was removed by Volker as no longer necessary, which had directly called the function mentioned in 3 above.

[ann0see]

Just to add: I've tagged a nightly. We should decide how to proceed (publish a new release, publish an announcement for recommended updating,...).

[pljones]

publish an announcement for recommended updating,...

I'd recommend updating to anyone operating a post-3.5.5 server that's open to the public. (Not that most registered servers will update - they've been on the same version since they were installed - but for the few that are maintained, it's worth the announcement.)

[hoffie]

So I guess by now it's clear that this fix will be part of a shortly released 3.9.1. I've already added a reminder to the release tracker. I'm therefore closing this issue for now.