Duplicate rows in monitor after radiod is restarted

[argilo]

If radiod is restarted while monitor is running, then duplicate rows begin appearing in monitor's table. It looks like this happens because the UDP source port differs, so lookup_session considers the session to be new:

https://github.com/ka9q/ka9q-radio/blob/4c3fcae0901203800faf5aa23369f9275c5323a4/monitor.c#L1087-L1096

Perhaps only the source address should be compared?

Valgrind also complains that uninitialized memory is read here. I expect this happens because sockaddr_storage is padded so as to be large enough to hold any of the various sockaddr structures.

==1526506== Conditional jump or move depends on uninitialised value(s)
==1526506==    at 0x485207E: bcmp (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1526506==    by 0x10F837: lookup_session (monitor.c:1090)
==1526506==    by 0x10CBC2: sockproc (monitor.c:395)
==1526506==    by 0x4B05B42: start_thread (pthread_create.c:442)
==1526506==    by 0x4B96BB3: clone (clone.S:100)

I expect the sockaddr structures will need to be compared more carefully to avoid this.

[ka9q]

On 5/9/22 10:17, Clayton Smith wrote:

If radiod is restarted while monitor is running, then duplicate rows begin appearing in monitor's table. It looks like this happens because the UDP source port differs, so |lookup_session| considers the sessions to be new:

https://github.com/ka9q/ka9q-radio/blob/4c3fcae0901203800faf5aa23369f9275c5323a4/monitor.c#L1087-L1096

Perhaps only the source address should be compared?

Yes, I've long noticed that. I've been trying to decide if the source port should be part of the logical session ID or not.

Valgrind also complains that uninitialized memory is read here. I expect this happens because |sockaddr_storage| is padded so as to be large enough to hold any of the various |sockaddr| structures.

|==1526506== Conditional jump or move depends on uninitialised value(s) ==1526506== at 0x485207E: bcmp (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) ==1526506== by 0x10F837: lookup_session (monitor.c:1090) ==1526506== by 0x10CBC2: sockproc (monitor.c:395) ==1526506== by 0x4B05B42: start_thread (pthread_create.c:442) ==1526506== by 0x4B96BB3: clone (clone.S:100) |

I expect the |sockaddr| will need to be compared more carefully to avoid this.

Thanks. Yes, I should probably be more explicit about what fields in a sockaddr I compare.

Phil

Message ID: @.***>

[ka9q]

OK, I'm working on it. The port number will not be part of the comparison, but it will be updated so that the verbose display will reflect any changes.

Comparing sockaddr structures is tedious enough that I'm creating a new function address_match() to do it. It'll work for IPv4 or IPv6, even though I currently only use IPv4.

On 5/9/22 17:30, Phil Karn wrote:

On 5/9/22 10:17, Clayton Smith wrote:

If radiod is restarted while monitor is running, then duplicate rows begin appearing in monitor's table. It looks like this happens because the UDP source port differs, so |lookup_session| considers the sessions to be new:

https://github.com/ka9q/ka9q-radio/blob/4c3fcae0901203800faf5aa23369f9275c5323a4/monitor.c#L1087-L1096

Perhaps only the source address should be compared?

Yes, I've long noticed that. I've been trying to decide if the source port should be part of the logical session ID or not.

Valgrind also complains that uninitialized memory is read here. I expect this happens because |sockaddr_storage| is padded so as to be large enough to hold any of the various |sockaddr| structures.

|==1526506== Conditional jump or move depends on uninitialised value(s) ==1526506== at 0x485207E: bcmp (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) ==1526506== by 0x10F837: lookup_session (monitor.c:1090) ==1526506== by 0x10CBC2: sockproc (monitor.c:395) ==1526506== by 0x4B05B42: start_thread (pthread_create.c:442) ==1526506== by 0x4B96BB3: clone (clone.S:100) |

I expect the |sockaddr| will need to be compared more carefully to avoid this.

Thanks. Yes, I should probably be more explicit about what fields in a sockaddr I compare.

Phil

Message ID: @.***>

[ka9q]

OK, I just pushed changes to monitor.c, multicast.c and multicast.h to do this. Seems to work fine here. Try it out.

I'll go through all my code and get rid of any other memcmp()'s on socket structures and replace them with calls to address_match() (plus port number comparisons where warranted).

On 5/9/22 20:02, Phil Karn wrote:

OK, I'm working on it. The port number will not be part of the comparison, but it will be updated so that the verbose display will reflect any changes.

Comparing sockaddr structures is tedious enough that I'm creating a new function address_match() to do it. It'll work for IPv4 or IPv6, even though I currently only use IPv4.

On 5/9/22 17:30, Phil Karn wrote:

On 5/9/22 10:17, Clayton Smith wrote:

If radiod is restarted while monitor is running, then duplicate rows begin appearing in monitor's table. It looks like this happens because the UDP source port differs, so |lookup_session| considers the sessions to be new:

https://github.com/ka9q/ka9q-radio/blob/4c3fcae0901203800faf5aa23369f9275c5323a4/monitor.c#L1087-L1096

Perhaps only the source address should be compared?

Yes, I've long noticed that. I've been trying to decide if the source port should be part of the logical session ID or not.

Valgrind also complains that uninitialized memory is read here. I expect this happens because |sockaddr_storage| is padded so as to be large enough to hold any of the various |sockaddr| structures.

|==1526506== Conditional jump or move depends on uninitialised value(s) ==1526506== at 0x485207E: bcmp (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) ==1526506== by 0x10F837: lookup_session (monitor.c:1090) ==1526506== by 0x10CBC2: sockproc (monitor.c:395) ==1526506== by 0x4B05B42: start_thread (pthread_create.c:442) ==1526506== by 0x4B96BB3: clone (clone.S:100) |

I expect the |sockaddr| will need to be compared more carefully to avoid this.

Thanks. Yes, I should probably be more explicit about what fields in a sockaddr I compare.

Phil

Message ID: @.***>

[argilo]

Looks good! No duplicate lines when I restart radiod.

[argilo]

I'll close this off, since the original issue with duplicate rows in monitor is resolved.

[ka9q]

I've gone back to the previous practice of making the source port part of the session ID. This is necessary because one computer might run several instances of radiod, and they might send to the same SSRC. I know it's annoying when radiod restarts with a new port number, but since the previous session mutes without any new data, is this really a problem? You can always delete it with the 'd' key.

I might still find a way to identify the same stream when radiod is restarted. Radiod now beacons status data on each output stream, so clients like 'monitor' can get much more complete metadata. When I enhance 'monitor' to read this metadata, I can do things like display the frequency and mode and merge the same stream when radiod restarts.

[argilo]

I know it's annoying when radiod restarts with a new port number, but since the previous session mutes without any new data, is this really a problem?

It's not a big deal, just a minor annoyance.