Bugzilla – Bug 1147
SlimServer can cause problems with network firmware devices (for example packet8 VoIP service) because of UDP arp packets to disconnected slimp3 client
Last modified: 2009-09-08 09:22:37 UTC
Summary: SlimServer can cause problems with network firmware devices (for example packet8 VoIP service) because of UDP arp packets to disconnected slimp3 client The SlimServer has exposed a defect in network firmware devices of lesser quality because they are unavailable to handle broadcast UDP arp packets correctly. The SlimServer generates broadcast UDP arp packets when one of its clients are down, then these packets trigger failure on other devices. The DTA310 used by the packet8 VoIP is one of these problem devices. Ideally the network hardware firmware devices should fix their problem, but the slimserver software is easier to change. I have a low impact software code fix that addresses the issue. The packet8 service becomes unusable with the broadcast UDP arp packets (poor sound quality, incoming and outgoing voice breakups, complete phone call disconnects, phone calls dialed not completing). I have also noticed that the UDP broadcast also caused minor problems for both of my cable-modem and netgear gateway because ever since I made the SlimServer code changes all of my third party network devices have had much fewer problems. When the SlimServer UDP arp broadcasts were present I was resetting one or both devices several times a week. I have tested my code fix with version 5.4.0 on a Qube3. I have also quickly verified the existences of the bug and fix in versions 5.4.1, 6.0b1, and 6.0b2 on a Qube3. I have identified two areas in the code that address this issue. The first area is in the hello discovery code in the initialization. The second area is in the general overall networking code that currently does not address a slim device vanishing from the network. I have no way of knowing if these are issues for the squeezbox, but given that the squeezebox code appears to be a TCP/IP based connection; I believe this problem is exclusive to the slimp3. My proposed changes are mainly for the slimp3 and should not change the current behavior of the squeezebox. I have not had a great deal of time to devote to understanding the slimserver code, but I have addressed the Hello discovery code (which continuously looped forever sending UDP arp packets to disconnected slimp3) by simply adding a timeout value to the networking class initialization. I addressed the issue of the slimp3 vanishing from the network by adding a time stamp field to the player record and then updating the time stamping every time any network traffic from a player was received by the slimserver. I also added an internal timer loop to monitor the timestamp and ping the device as necessary. If the timestamp goes out of range the client is removed from the internal list. The URL attached to this bug is a zip file with all of my code modification for this bug for the 5.4.0, 5.4.1, and 6.0b1, 6.0b2 releases. The file diffs are at the end of the bug report as well. I hope you are able to use my codes change directly. There is one minor existing or exposed issue that I have not looked into. If the slimp3 device is in the off state and displaying the time when it is disconnected from the network, when the slimp3 comes back on it returns to the on state with what it was last playing music wise. It appears that the off state is not saved or is not restored from the saved configuration information. The on state of the player is restored without a problem when the device is disconnected from the network and reconnected at a later time. Please contact me for any follow up, if it is needed. Note: this bug occurs in SlimServer versions 5.4.0, 5.4.1, 6.0b1, and 6.0b2 LAN hardware involved --------------------- Qube3 (slimserver) - patched with all the latest updates - used perl 5.4.1 or 5.4.6 Netgear 10/100 switch Netgear WGT624 gateway Zoom cable modem DTA-310 (packet8 VoIP) device latest firmware v1034 Macs & PCs on and off network slimp3 (powered down every night and other times when it is not being used) Comcast ISP Steps to reproduce ------------------ -Network connect slimp3 and qube3 and DTA-310 and gateway to network -Power up gateway and setup -Power up DTA310 and setup -Power on slimp3 -Setup slimserver on Qube -Use slimp3 and verify all the setup is working -Log into Qube and watch UDP network port (tcpdump) -Disconnect the slimp3 from network (wait about 30 seconds) * See the continuous UDP arps and the DTA-310 link light now flashes File Diffs v6.0b1 ----------------- #####Protocol.pm 28a29 > my $SLIMECHO_PORT = 7; # Echo port on the Slim devices 80c81,82 < LocalAddr => $main::localClientNetAddr --- > LocalAddr => $main::localClientNetAddr, > Timeout => Slim::Utils::Prefs::get('remotestreamtimeout') 125,160c127,136 < < # check that it's a message type we know: starts with i r 2 d a or h (but not h followed by 0x00 0x00) < if ($msg =~ /^(?:[ir2a]|h(?!\x00\x00))/) { < my $client = getUdpClient($clientpaddr, $sock, $msg); < < if (!defined($client)) { < return; < } < < if ($SIMULATE_RX_DELAY) { < # simulate rx delay < Slim::Utils::Timers::setTimer($client, Time::HiRes::time() + $SIMULATE_RX_DELAY/1000, < \&Slim::Networking::Protocols::processMessage, $msg); < } else { < processMessage($client, $msg); < } < < } elsif ($msg =~/^d/) { < # Discovery request: note that slimp3 sends deviceid and revision in the discovery < # request, but the revision is wrong (v 2.2 sends revision 1.1). Oops. < # also, it does not send the MAC address until the [h]ello packet. < # Squeezebox sends all fields correctly. < < my ($msgtype, $deviceid, $revision, @mac) = unpack 'axCCxxxxxxxxH2H2H2H2H2H2', $msg; < my $mac = join(':', @mac); < Slim::Network::Discovery::gotDiscoveryRequest($sock, $clientpaddr, $deviceid, $revision, $mac); < < # Playlist::executecommand can be accessed over the UDP port < } elsif ($msg=~/^executecommand\((.*)\)$/) { < my $ecArgs=$1; < my @ecArgs=split(/, ?/, $ecArgs); < $::d_protocol && msg("UDP: executecommand($ecArgs)\n"); < my $clientipport = shift(@ecArgs); < my $client = Slim::Player::Client::getClient($clientipport); < Slim::Control::Command::execute($client, \@ecArgs); < --- > my ($clientport, $clientip) = sockaddr_in($clientpaddr); > > if ($clientport eq Slim::Networking::Slimproto::getSlimProtoPort()) { > # handle the udp packets that came from client the SLIMPROTO_PORT(3483) > processReplyFromSlimPort($sock, $clientpaddr, $clientip, $msg); > > } elsif ($clientport eq $SLIMECHO_PORT) { > # handle the udp packets that came from client the echo port(7) > processReplyFromEchoPort($sock, $clientpaddr, $msg); > 164d139 < my ($clientport, $clientip) = sockaddr_in($clientpaddr); 170a146,233 > } > > sub processReplyFromSlimPort { > my ($sock, $clientpaddr, $clientip, $msg) = @_; > > # check that it's a message type we know: starts with i r 2 d a or h (but not h followed by 0x00 0x00) > if ($msg =~ /^(?:[ir2a]|h(?!\x00\x00))/) { > my $client = getUdpClient($clientpaddr, $sock, $msg); > > if (!defined($client)) { > return; > } > > if ($SIMULATE_RX_DELAY) { > # simulate rx delay > Slim::Utils::Timers::setTimer($client, Time::HiRes::time() + $SIMULATE_RX_DELAY/ 1000, > \&Slim::Networking::Protocols::processMessage, $msg); > } else { > processMessage($client, $msg); > } > $client->lastReplyTime(Time::HiRes::time()); > > } elsif ($msg =~/^d/) { > # Discovery request: note that slimp3 sends deviceid and revision in the discovery > # request, but the revision is wrong (v 2.2 sends revision 1.1). Oops. > # also, it does not send the MAC address until the [h]ello packet. > # Squeezebox sends all fields correctly. > > my ($msgtype, $deviceid, $revision, @mac) = unpack 'axCCxxxxxxxxH2H2H2H2H2H2', $msg; > my $mac = join(':', @mac); > Slim::Network::Discovery::gotDiscoveryRequest($sock, $clientpaddr, $deviceid, $revision, $mac); > > # Playlist::executecommand can be accessed over the UDP port > } elsif ($msg=~/^executecommand\((.*)\)$/) { > my $ecArgs=$1; > my @ecArgs=split(/, ?/, $ecArgs); > $::d_protocol && msg("UDP: executecommand($ecArgs)\n"); > my $clientipport = shift(@ecArgs); > my $client = Slim::Player::Client::getClient($clientipport); > Slim::Control::Command::execute($client, \@ecArgs); > $client->lastReplyTime(Time::HiRes::time()); > > } else { > if ($::d_protocol) { > msg("ignoring Client: ".inet_ntoa($clientip).":slimport that sent bogus message $msg\n"); > } > } > } > > sub processReplyFromEchoPort { > my ($sock, $clientpaddr, $msg) = @_; > my $client; > my @mac; > my $txt; > my $macaddr; > > ( $txt, $mac[0], $mac[1], $mac[2], $mac[3], $mac[4], $mac[5] ) > = unpack ('a4H2H2H2H2H2H2', $msg); > $macaddr = join(':', @mac); > > $client = Slim::Player::Client::getClient($macaddr); > if (defined($client)) { > $client->lastReplyTime(Time::HiRes::time()); > if ($::d_protocol) { > my ($clientport, $clientip) = sockaddr_in($client->paddr); > msg("processReplyFromEchoPort: recv text of '$txt'\n from Client: " > .inet_ntoa($clientip)." lastReplyTime was set\n"); > } > } else { > $::d_protocol && msg("processReplyFromEchoPort: recv text of '$txt'\n"); > } > > } > > sub sendEchoToClient { > my $client = shift; > > my @mac = split(':', $client->macaddress()); > my $paddr = ipaddress2paddr($client->ip.":".$SLIMECHO_PORT); > my $frame = pack 'a4H2H2H2H2H2H2', (' Yo!', > $mac[0], $mac[1], $mac[2], $mac[3], $mac[4], $mac[5] ); > > $udpsock->send( $frame, 0, $paddr); > > if ($::d_protocol) { > my ($clientport, $clientip) = sockaddr_in($client->paddr); > msg("sendEchoToClient sent to Client: ".inet_ntoa($clientip)."\n"); > } #####Slimproto.pm 536a537,540 > sub getSlimProtoPort { > return ($SLIMPROTO_PORT); > } > #####Client.pm 725a726 > $client->[95] = undef; # lastReplyTime 866a868,881 > sub forgetlostClient { > my $id = shift; > my $client = getClient($id); > > if ($client) { > Slim::Web::HTTP::forgetClient($client); > #Slim::Player::Playlist::forgetClient($client); > # trying to play will close out any open files. > Slim::Control::Command::execute($client, ["play"]); > Slim::Utils::Timers::forgetClient($client); > delete $clientHash{$id}; > } > } > 1545a1561,1564 > sub lastReplyTime { > my $r = shift; > @_ ? ($r->[95] = shift) : $r->[95]; > } #####SLIMP3.pm 17a18,24 > use Slim::Utils::Timers; > use Slim::Player::Client; > use Slim::Networking::Protocol; > use Socket; # for sockaddr_in > > my $CLIENTTIMEOUT = 30; # in seconds > my $CHECKCLIENTEVERY = 5; # in seconds 52c59,63 < --- > > # setup timer to keep checking if client is still available > $client->lastReplyTime(Time::HiRes::time()); > Slim::Utils::Timers::setTimer($client, (Time::HiRes::time() + $CHECKCLIENTEVERY), \&checkSLIMP3); > 55a67,93 > sub checkSLIMP3 { > my $client = shift; > my $now; > my $deltaTime; > > $now = Time::HiRes::time(); > $deltaTime = $now-$client->lastReplyTime; > > if ($deltaTime > $CLIENTTIMEOUT ) { > # we have lost the client, so clean up > my ($clientport, $clientip) = sockaddr_in($client->paddr); > msg("Client: ".inet_ntoa($clientip)." last responded $deltaTime seconds ago and is being timed out.\n"); > msg("The client is being forgotten about\n"); > #Slim::Player::Client::forgetClient($client->id()); > Slim::Player::Client::forgetlostClient($client->id()); > > } else { > if (($now-$client->lastReplyTime) > $CHECKCLIENTEVERY ) { > # only send the echo packet if we have not heard from the client anyway > Slim::Networking::Protocol::sendEchoToClient($client); > } > > # Call ourselves again after $CHECKCLIENTEVERY seconds > Slim::Utils::Timers::setTimer($client, ($now + $CHECKCLIENTEVERY), \&checkSLIMP3); > } > } > 175c213,214 < send($client->udpsock, $frame, 0, $client->paddr()); --- > #send($client->udpsock, $frame, 0, $client->paddr()); > $client->udpsock->send($frame, 0, $client->paddr()); 190c229,230 < send($client->udpsock, $frame, 0, $client->paddr()); --- > #send($client->udpsock, $frame, 0, $client->paddr()); > $client->udpsock->send($frame, 0, $client->paddr()); 198c238,239 < send($client->udpsock, '2 '.$data, 0, $client->paddr); --- > #send($client->udpsock, '2 '.$data, 0, $client->paddr); > $client->udpsock->send('2 '.$data, 0, $client->paddr);
Brian - could you include your patch as an attachment in unified diff format? That's diff -u Thanks.
Oh - forgot to mention, against 6.0b2 (or b3). Thanks
*** Bug 1148 has been marked as a duplicate of this bug. ***
Created attachment 353 [details] proposed patch to Protocol.pm v6.0b2 diff -u as requested
Created attachment 354 [details] proposed patch to Client.pm v6.0b2 diff -u as requested
Created attachment 355 [details] proposed patch to SLIMP3.pm v6.0b2 diff -u as requested
Created attachment 356 [details] proposed patch to Slimproto.pm v6.0b2 diff -u as requested
Thanks for the comprehensive patch. We're shutting down on 6.0 right now, let's get this in 6.0.1 as soon as possible.
This is good stuff, but the scope of the change makes me nervous for 6.1. We'll get to it as soon as possible.
Dean - do we want to look at this for 6.3 or 6.5?
cc'ing Dean since he last commented on the benefits of this change
Is there a reason to not fix it?
My impression is the decision was a combination of 1) it doesn't seem to be a problem for anyone else 2) we don't have the resources to update your proposed patch to Slimserver 7 3) Concern was expressed about the riskiness and complexity of the change.